Bug 470703 - Review Request: links - text mode browser with graphics
Summary: Review Request: links - text mode browser with graphics
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Matěj Cepl
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 490152 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-11-09 02:26 UTC by John F
Modified: 2018-04-11 11:20 UTC (History)
10 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-04-14 18:52:04 UTC
Type: ---
Embargoed:
mcepl: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
screenshot of Google with links -g (64.64 KB, image/png)
2009-04-10 16:22 UTC, Matěj Cepl
no flags Details
/var/log/Xorg.0.log (49.72 KB, text/plain)
2009-04-10 16:56 UTC, Matěj Cepl
no flags Details

Description John F 2008-11-09 02:26:57 UTC
Spec URL: http://matrix.senecac.on.ca/~jhford/fedora/links.spec
SRPM URL: http://matrix.senecac.on.ca/~jhford/fedora/links-2.2-1.fc10.src.rpm
Description: 
This is a new version of the Links graphical browser.  It is distinct from the elinks browser which is currently included because it offers an X11 or Linux framebuffer graphical mode.  

There only issue is that the elinks package is using /usr/bin/links even though it isn't actually called links.  I would be willing to do the modifications to elinks depending on what the outcome is

Comment 1 John F 2008-11-09 02:28:36 UTC
Depending on ario is because I am waiting to be sponsored through ario.

Comment 2 John F 2008-11-10 18:27:15 UTC
http://matrix.senecac.on.ca/~jhford/fedora/links.spec
http://matrix.senecac.on.ca/~jhford/fedora/links-2.2-2.fc10.src.rpm

Added a missing dependency.  I have also modified %build to ensure that static libraries are built

Comment 3 Fabian Affolter 2008-11-10 19:11:06 UTC
Just some quick comments on your spec file:

- iconv -f ISO-8859-1 -t UTF-8 %{_builddir}/%{name}-%{version}/AUTHORS -o  \
%{_builddir}/%{name}-%{version}/AUTHORS
 You didn't preserve the time stamp 
 https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

- desktop-file-install --vendor="fedora"
  Vendor tag 'fedora' is no longer used, see
https://fedoraproject.org/wiki/TomCallaway/DesktopFileVendor

- gzip $RPM_BUILD_ROOT/%{_mandir}/man1/links.1
  This is not necessary, install only the non-gzipped man page (as links.1), it
is compressed automatically during rpm build process.

Comment 4 John F 2008-11-11 05:09:42 UTC
Hi Fabian,

Just a couple questions:

1. I notice that in a lot of documentation it states that timestamps should be preserved.  Is this for auditing reasons mainly?

2. The documentation that I found for dealing with desktop files still says to use fedora as a vendor.  Is it appropriate for me to update this page on the wiki?  (https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files)

I have made the changes as you recomended and have uploaded a new specfile and srpm at:

http://matrix.senecac.on.ca/~jhford/fedora/links.spec
http://matrix.senecac.on.ca/~jhford/fedora/links-2.2-3.fc10.src.rpm

Thank you very much for your time!

John

Comment 5 Susi Lehtola 2008-11-11 09:05:31 UTC
(In reply to comment #0)
> There only issue is that the elinks package is using /usr/bin/links even though
> it isn't actually called links.  I would be willing to do the modifications to
> elinks depending on what the outcome is

This is a bit tricky since a lot of people are used to using links in the console.

I'd prefer this package and binary to be named links2, which also avoids the name clash with elinks.

Comment 6 John F 2008-11-11 13:48:48 UTC
The issue is that this is the successor to the original links.  Elinks is a fork where this is just version 2.  In the elinks spec file the 'links' symlink is done by the package maintainer in the following code from %install:

ln -s elinks $RPM_BUILD_ROOT%{_bindir}/links
ln -s elinks.1 $RPM_BUILD_ROOT%{_mandir}/man1/links.1

I also noticed that in https://fedoraproject.org/wiki/Packaging/NamingGuidelines#General_Naming the package name should match upstream tarball where technically possible.  

Maybe this is a situation where alternatives should be used?

Comment 7 manuel wolfshant 2008-11-11 14:00:59 UTC
To me alternatives looks like a nice answer. Did you try to talk with elinks's maintainer ?

Comment 8 Patrice Dumas 2008-11-11 14:15:51 UTC
alternative could be used if elinks and links command-line syntax are compatible. However the link for elinks to links should certaainly be undone when this package is put in fedora, such that elink is elinks and links is links.

The elinks package is very broken when it comes to obsoletes and provides, however, and the elinks maintainer should contacted such that this is fixed. Unfortunately, the review request has been accepted without fixing those issues:

https://bugzilla.redhat.com/show_bug.cgi?id=225725

Comment 9 John F 2008-11-11 15:31:35 UTC
I guess I should try to talk with the elinks maintainer, that makes a lot of sense!  Should I contact them directly or use the fedora-devel list?

Comment 10 Patrice Dumas 2008-11-11 17:47:55 UTC
I think that you should try to contact the maintainer directly, and if things doesn't move, go to the mailing list.

Comment 11 Ondrej Vasik 2008-11-13 08:35:20 UTC
Just as a note - links2 is used as name of binary of original links on other Linux distros like Gentoo as well. So with versioned obsoletes in elinks everything would be fine and packages could live together without troubles.

Comment 12 John F 2008-11-13 13:29:13 UTC
I noticed that with Ubuntu and Debian, they do use links2 but that is because they also have links 1 which uses links.  Their elinks package doesn't use the 'links' symlink.

Comment 13 Ondrej Vasik 2008-11-13 13:53:29 UTC
Yes, they have links v1.00 package, but links v1.00 package doesn't have graphic, so it could fully replace elinks - so I'll be completely ok to remove symlinks to links for hypothetical links v1.00 package if accepted on fedora-devel-list (although the output could differ a bit in some cases), but not for links v2.X package with graphics ... I really would prefer to use links2 binary name for links with graphic as it is used on other distros that way.

Comment 14 Patrice Dumas 2009-01-14 13:52:07 UTC
Ok for me with using links2. There is a wiki page with those kind of name changes to coordinate with other distros, but I can't find it.

Comment 15 Matěj Cepl 2009-03-14 07:32:00 UTC
*** Bug 490152 has been marked as a duplicate of this bug. ***

Comment 16 Lubomir Rintel 2009-03-17 13:46:36 UTC
0.) links or links2?

#Issue:
#  There is a symlink in /usr/bin/links to elinks if that package is installed
#  Should this package use links2 or should I modify elinks to remove the links
#  symlink from elinks

mv $RPM_BUILD_ROOT/%{_bindir}/links $RPM_BUILD_ROOT/%{_bindir}/links2
mv $RPM_BUILD_ROOT/%{_mandir}/man1/links.1 $RPM_BUILD_ROOT/%{_mandir}/man1/links2.1

Upstream decided to call this "links", not "links2" and changing it needs careful consideration. I'm all for fixing elinks package not to call its binary "links" but "elinks", but given this might be controversial and you've discussed it before I won't block the review for this, but leave the choice up to you.

When it comes to consistency with other distributors, all operating systems I've used called the links or links 2 binary "links" (well, Slackware and NetBSD/pkgsrc).

1.) Sources

Source2:        %{name}.png

Isn't links_32x32.xpm included in the distribution tarball sufficient?  If it really isn't, include full URL here, or comment on how did you obtain this file.

2.) Summary contains application name

Summary:        Links is a web browser running in both graphics and text mode

Please remove "Links is a".

3.) Description.

The focus on intuitive usability makes it suitable as a web 
browser for low-end terminals in libraries, Internet cafes etc

This is a joke, right? :)
Seriously, would you install links in an Internet cafe?
Please try to make description more helpful to casual user.

4.) Don't silence make. Its output is useful.

make %{?_smp_mflags} -s

Remove -s

5.) Redundant arguments to %configure

--bindir=%{_bindir} --mandir=%{_mandir}

These are useless, %configure already expands to them. See:

rpm --eval %configure

6.) Icon directory

%files
%{_datadir}/icons/hicolor/48x48/apps

Why do you put the icon here, and own just this directory?
You don't seem to depend on anything that would own %{_datadir}/icons.
Wouldn't /usr/share/pixmaps be a better choice?

7.) Features

You're missing certain features lot of people will find useful:

SSL support:            NO

Links 2 looks for OpenSSL, but openssl-devel is not BuildRequired.
Given Links is distributed under the terms of GPLv2+, you will need exception from GPL for this from Links upstream. Alternatively, you can use nss_compat_ossl (patch is already in elinks repository, you
may want to adapt it). See: http://www.gnome.org/~markmc/openssl-and-the-gpl.html

Graphics drivers:       FB

No X? I'd say X11 is quite popular these days :) Please BuildRequire libX11-devel

8.) Compile-time warnings

You get a _LOT_ of "pointer targets differ in signedness" warnings.  Please notify upstream.

9.) Desktop entry

Name=glinks

Please replace this with something more meanungful, such as "Links Web Browser"

Comment=Text mode browser with graphics

This is self-contradictory.

Comment 17 Ondrej Vasik 2009-03-17 14:13:15 UTC
Ad 0) I would say the best way would be to have links2 binary as the default in that package and elinks/links2.X/(possibly links v1.X in future) could share for /usr/bin/links (elinks has it's binary named elinks, /usr/bin/links is just compat symlink) via alternatives (to make it easier for users)

Comment 18 Lubomir Rintel 2009-03-21 20:56:35 UTC
Ping?

Comment 19 Ondrej Vasik 2009-03-25 13:02:21 UTC
Just FYI ... I modified elinks to use alternatives for links . Elinks priority chosen as 90, I'm ok with anything lower for links2 and even something higher for possible links1 in future.

Comment 20 Lubomir Rintel 2009-03-30 19:31:38 UTC
John: Ping?

Comment 21 Lubomir Rintel 2009-04-02 21:46:47 UTC
SPECS: http://v3.sk/~lkundrak/SPECS/links.spec
SRPMS: http://v3.sk/~lkundrak/SRPMS/links-2.2-4.fc11.src.rpm

Most issues addressed, apart from SSL support (which is in works) and warnings. Anyone please continue this, since the submitter doesn't seem to respond.

Comment 22 Lubomir Rintel 2009-04-03 06:26:40 UTC
SPECS: http://v3.sk/~lkundrak/SPECS/links.spec
SRPMS: http://v3.sk/~lkundrak/SRPMS/links-2.2-5.fc11.src.rpm

+ NSS support

Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1274783
Anyone to review this?

Comment 23 Ondrej Vasik 2009-04-03 09:12:28 UTC
Ok, just quick check first...

1) 
#Issue:
#  There is a symlink in /usr/bin/links to elinks if that package is installed
#  Should this package use links2 or should I modify elinks to remove the links
#  symlink from elinks
- this comment in spec should be removed as elinks/links2 now uses alternatives.

2)
Many warnings "pointer targets in passing argument <N> of <variable> differ in signedness" in build.log still ... upstream should address those...  maybe just adding Mikulas to that review could be ok.

The rest of the spec file looks sane to me (except one trailing space in build section - line with mv converted.AUTHORS ). 

Question: Shouldn't be that NSS support enabled via configure option (like in elinks?). This should be easier to get into upstream ...

Comment 24 Lubomir Rintel 2009-04-03 09:42:48 UTC
(In reply to comment #23)
> Ok, just quick check first...

If I understand correctly, you're planning to do a more detailed review later, right? In that case, I'll wait with updating the package until then.

> 1) 
> #Issue:
> #  There is a symlink in /usr/bin/links to elinks if that package is installed
> #  Should this package use links2 or should I modify elinks to remove the links
> #  symlink from elinks
> - this comment in spec should be removed as elinks/links2 now uses
> alternatives.

Will remove.

> 2)
> Many warnings "pointer targets in passing argument <N> of <variable> differ in
> signedness" in build.log still ... upstream should address those...  maybe just
> adding Mikulas to that review could be ok.

Mikulas? Which one? An upstream developer?

> The rest of the spec file looks sane to me (except one trailing space in build
> section - line with mv converted.AUTHORS ).

Will fix.

> Question: Shouldn't be that NSS support enabled via configure option (like in
> elinks?). This should be easier to get into upstream ...  

It should. In fact, I have little motivation to fix configure scripts gotten obviously wrong -- see use of random include directories in openssl detection routine below.

So my plan is to throw this upstream and rewrite it (and the openssl detection) only once they complain. I don't really care about user choosing between NSS and OpenSSL.

Comment 25 Ondrej Vasik 2009-04-03 10:10:21 UTC
(In reply to comment #24)
> > Ok, just quick check first...
> If I understand correctly, you're planning to do a more detailed review later,
> right? In that case, I'll wait with updating the package until then.

Yep, I do plan to check&fill review approval template I have step by step... Hopefully today ;)

> > 2)
> > Many warnings "pointer targets in passing argument <N> of <variable> differ in
> > signedness" in build.log still ... upstream should address those...  maybe just
> > adding Mikulas to that review could be ok.
> 
> Mikulas? Which one? An upstream developer?

Exactly... adding him to CC...


> > Question: Shouldn't be that NSS support enabled via configure option (like in
> > elinks?). This should be easier to get into upstream ...  
> 
> It should. In fact, I have little motivation to fix configure scripts gotten
> obviously wrong -- see use of random include directories in openssl detection
> routine below.
> 
> So my plan is to throw this upstream and rewrite it (and the openssl detection)
> only once they complain. I don't really care about user choosing between NSS
> and OpenSSL.  

In general - I'm ok with that - that patch is not very complicated and NSS compat could be full replacement for OpenSSL in that case. I guess that's not review blocker anyway - any maintainer/comaintainer could improve that configure script later to give user a chance to choose which library will be used.

Comment 26 Matěj Cepl 2009-04-03 10:16:36 UTC
or what about his Red Hat address :)

Comment 27 Ondrej Vasik 2009-04-07 12:32:34 UTC
Sorry for few days delay...

+ package builds in mock (development i386).
+ rpmlint is NOT silent for SRPM 
 links.src:37: W: unversioned-explicit-provides webclient
 links.src:38: W: unversioned-explicit-provides text-www-browser
   - common for those provides in existing packages - could be ignored
 links.src: W: mixed-use-of-spaces-and-tabs (spaces: line 5, tab: line 37)
   - should be fixed
+ rpmlint is NOT silent for RPM.
  links.i586: W: non-executable-in-bin /usr/bin/links 0644
   - I guess this is just rpmlint noise, as /usr/bin/links is only via 
     alternatives and should be ignored
  links.i586: E: invalid-desktopfile /usr/share/applications/links.desktop
   - links.desktop: error: required key "Encoding" not found
+ source files match upstream.
bf5b20529a2a811701c5af52b28ebdd4  links-2.2.tar.bz2
+ package meets naming and packaging guidelines.
+ specfile is properly named, is cleanly written
+ Spec file is written in American English.
+ Spec file is legible.
+ dist tag is present.
+ build root is correct.
+ license is open source-compatible.
- License text is NOT included in package.
  - COPYING file should be shipped
+ %doc files present.
+ BuildRequires are proper.
+ defattr usage is correct. 
+ %clean is present.
+ package installed properly.
+ Macro use appears rather consistent.
+ Package contains code.
+ no static libraries.
+ no .pc files.
+ no .la files.
+ menu translations are available in binary
+ Does own the directories it creates (none special created)
+ no duplicates in %files.
+ file permissions are appropriate (ignoring false-positive rpmlint warning about ghosted touched file/future alternative symlink).
+ GUI app with desktop file (expecting addition of Encoding line) and icon  

Required to fix:
- Add Encoding=UTF-8 line to desktop file
- Do ship COPYING in %doc
- fix mixed tab/spaces and previously mentioned things in spec file

Comment 28 Mikuláš Patočka 2009-04-08 12:15:42 UTC
I don't know anything about NSS, I thought that it is used by Mozilla and I didn't even know that other projects could be compiled against it. Dost the conversion mean only a recompile and relink? Or do I have to rewrite some of the code?

I may try to compile Links with NSS, but even if it succeeds, you won't get good testing with NSS soon, Links always used OpenSSL and it is long-term tested with it, so if you want something proven stable, use OpenSSL.

BTW. why do you want NSS instead of OpenSSL?

Regarding that UTF-8 --- I have written most of it already for Links-2.3pre1, just have not released it yet. I could release it soon.

Regarding the config script --- there is a bug that it selects hardcoded directories first and the user directory last. Easy to fix. Just keep in mind that the configure script must be generated with autoconf 2.13 --- that was current version 10 years ago when Links project was started and support for all the operating systems is tested with it.

Comment 29 Lubomir Rintel 2009-04-10 06:59:53 UTC
(In reply to comment #28)
> I don't know anything about NSS, I thought that it is used by Mozilla and I
> didn't even know that other projects could be compiled against it. Dost the
> conversion mean only a recompile and relink? Or do I have to rewrite some of
> the code?

NSS provides and OpenSSL compat library which implements most (though definitely all) of OpenSSL API. See the patch included in the source package (most of it is just configure script modifications and like two ifdefs or so)

> I may try to compile Links with NSS, but even if it succeeds, you won't get
> good testing with NSS soon, Links always used OpenSSL and it is long-term
> tested with it, so if you want something proven stable, use OpenSSL.

It will get testing in Fedora. I believe there's not that much to test in SSL/TLS support anyways.

> BTW. why do you want NSS instead of OpenSSL?

Your license forbids use of OpenSSL.
See: http://www.gnome.org/~markmc/openssl-and-the-gpl.html

Besides that, NSS is used most of core SSL functionality in Fedora and pushing the redundant oddly-licensed library away as much as we can is indeed good, especially when it comes to cryptography where redundancy implies increased risk of serious flaws.

> Regarding the config script --- there is a bug that it selects hardcoded
> directories first and the user directory last. Easy to fix. Just keep in mind
> that the configure script must be generated with autoconf 2.13 --- that was
> current version 10 years ago when Links project was started and support for all
> the operating systems is tested with it.  

Yup. If you have time, please take a look at the patch for NSS compatibility I bundled with the package. As Ondrej suggested, you may not be satisfied with it, but it's trivial enough for anyone to fit it their needs.

(In reply to comment #23)
> 2)
> Many warnings "pointer targets in passing argument <N> of <variable> differ in
> signedness" in build.log still ... upstream should address those...  maybe just
> adding Mikulas to that review could be ok.

Mikulas, any thoughts about these?

Comment 30 Matěj Cepl 2009-04-10 07:24:48 UTC
(In reply to comment #28)
> I don't know anything about NSS, I thought that it is used by Mozilla and I
> didn't even know that other projects could be compiled against it. Dost the
> conversion mean only a recompile and relink? Or do I have to rewrite some of
> the code?

See https://fedoraproject.org/wiki/FedoraCryptoConsolidation for more explanation ... it's nice to have a patch and Fedora package should certainly use it when it is there, but let me just emphasize that this is certainly not issue which would break Fedora package review.

Comment 31 Lubomir Rintel 2009-04-10 10:40:53 UTC
(In reply to comment #27)
> Required to fix:
> - Add Encoding=UTF-8 line to desktop file
> - Do ship COPYING in %doc
> - fix mixed tab/spaces and previously mentioned things in spec file  

All three are addressed here:

SPECS: http://v3.sk/~lkundrak/SPECS/links.spec
SRPMS: http://v3.sk/~lkundrak/SRPMS/links-2.2-6.fc11.src.rpm

Comment 32 Ondrej Vasik 2009-04-10 10:57:31 UTC
Looks sane for me now, APPROVED as the discussion with upstream maintainer about warnings/NSS is not review blocker...

Comment 33 Lubomir Rintel 2009-04-10 13:09:41 UTC
Thanks for the review.

New Package CVS Request
=======================
Package Name: links
Short Description: Web browser running in both graphics and text mode
Owners: lkundrak
Branches: EL-5 F-10

Comment 34 Matěj Cepl 2009-04-10 16:07:40 UTC
This:

# Incompatible with GPL
BuildConflicts: openssl-devel

is not good. Whatever conflicts between openssl-devel and GPL, the package should be buildable on computer where openssl-devel has been installed. You have to fix ./configure options, or patch the source code (if necessary) to avoid compiling in openssl, but this is not acceptable.

Comment 35 Lubomir Rintel 2009-04-10 16:21:09 UTC
Matej: Though this is mostly a pre-nss-patch relic (current version of the nss patch would not look for OpenSSL once NSS compat library is present) I believe this is completely legitimate use of BuildConflicts. Purpose of autoconf is to automatically configure package's features based on what software is available, and once such feature implies linking that should not be allowed it makes perfect sense to enforce that the offending piece of software is not present. (I am aware that --without-* is also a valid option).

Seriously, if you believe this is a review blocker, please re-read the guidelines and elaborate on what use of BuildConflicts seems legitimate to you.

Also, thank you for bringing this up once the review was finished, even though this was present in the SPEC file since my first revision.

Comment 36 Matěj Cepl 2009-04-10 16:22:11 UTC
Created attachment 339108 [details]
screenshot of Google with links -g

... and certainly it is not packaging review issue, but are you sure, that this is how Google should look like? (see the lines in the top left corner)?

Mikuláš?

Comment 37 Lubomir Rintel 2009-04-10 16:30:47 UTC
Thanks for the bug report.  We have reviewed the information you have provided
above, and there is some additional information we require that will be helpful
in our diagnosis of this issue.

Please attach your X server config file (/etc/X11/xorg.conf) and X server log
file (/var/log/Xorg.*.log) to the bug report as individual uncompressed file
attachments using the bugzilla file attachment link below.

We will review this issue again once you've had a chance to attach this
information.

Thanks in advance.

Comment 38 Matěj Cepl 2009-04-10 16:48:14 UTC
(In reply to comment #35)
> Matej: Though this is mostly a pre-nss-patch relic (current version of the nss
> patch would not look for OpenSSL once NSS compat library is present) I believe
> this is completely legitimate use of BuildConflicts.

No, it is not ... the fact that I have three versions of one library installed on my computer doesn't mean that you have to one particular of them (and if you want me to uninstall openssl, then please fix mysql, postifx, opal, nash, and dovecot ;-)). Either select the right one (which is what you do in your patch anyway), or add some ./configure option for it (--with-crypto={openssl,nss,gnutls} or something of that kind).

And of course more important is that this line is there only to annoy people who would actually want to build this package on their own computer ... it doesn't make any sense for building in mock/koji, and it is totally unnecessary -- when I remove it and rebuild package, resulting package doesn't require openssl anyway:

[matej@viklef redhat]$ rpm -qR links
/bin/sh  
/bin/sh  
/bin/sh  
/usr/sbin/alternatives  
/usr/sbin/alternatives  
/usr/sbin/alternatives  
libX11.so.6()(64bit)  
libbz2.so.1()(64bit)  
libc.so.6()(64bit)  
libc.so.6(GLIBC_2.2.5)(64bit)  
libc.so.6(GLIBC_2.3)(64bit)  
libc.so.6(GLIBC_2.3.4)(64bit)  
libc.so.6(GLIBC_2.4)(64bit)  
libdl.so.2()(64bit)  
libgpm.so.2()(64bit)  
libjpeg.so.62()(64bit)  
libm.so.6()(64bit)  
libm.so.6(GLIBC_2.2.5)(64bit)  
libnss_compat_ossl.so.0()(64bit)  
libpng12.so.0()(64bit)  
libpng12.so.0(PNG12_0)(64bit)  
libtiff.so.3()(64bit)  
libz.so.1()(64bit)  
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rtld(GNU_HASH)  
[matej@viklef redhat]$ 

> Seriously, if you believe this is a review blocker, please re-read the
> guidelines and elaborate on what use of BuildConflicts seems legitimate to you.

I have no clue what BuildConflicts is good for, and I have never seen it to be used before, but this is certainly not the case (see above).

> Also, thank you for bringing this up once the review was finished, even though
> this was present in the SPEC file since my first revision.  

I don't think there is any duty to be silent when I see stupidity in a package which is soon to come to Fedora -- I haven't thought it is necessary to review this package because I believed that ovasik will do proper job on it as usually he does. Only when I tried to rebuild this package on my computer I got hit by this, and so I jumped here before the damage would be hard to fix.

Comment 39 Matěj Cepl 2009-04-10 16:56:10 UTC
Created attachment 339109 [details]
/var/log/Xorg.0.log

here you are :) although I quite sure, it doesn't help you to fix this.

Comment 40 Mikuláš Patočka 2009-04-11 04:18:04 UTC
Google with links -g:

Google in Links should really look like that. Links doesn't support CSS and google adds textarea field to the page and hides it with CSS. So without CSS, the field is visible.

Comment 41 Mikuláš Patočka 2009-04-11 04:48:17 UTC
> # Incompatible with GPL
> BuildConflicts: openssl-devel

Could anyone please explain, why so you think that it is illegal to dynamically link GPL and NON-GPL code?

My point of view:

1. there is not any single legal case where it was found that dynamic linking is considered as creating derivate works. If it were viewed as creating derivate works, then loading browser plugins or running windows media player would be illegal activity, because it involves linking several dynamic libraries from different vendors with different licenses that do not allow creating derivate works (even normal commercial software could not have plugins, regardless of GPL).

2. even if dynamic linking were viewed as creating derivate works, the derivate work is created by the USER when he RUNS the program. Not by the packager who distributes the binary (Links binary package contains none of OpenSSL code, it contains only symbols names, and symbols names are not copyrightable).

3. GPL doesn't prevent anyone from creating derivate work of GPL programs and GPL-incompatible programs. It only prevents distributing such works. You can mix GPL code with whatever you like as long as you don't distribute it. So even if dynamic linking was viewed as creating derivate works, the only effective thing is that the user cannot distribute the core dump of the program (which contains both GPL-covered and GPL-incompatible code). He can definitelly run the program and let the dynamic linker create the "derivate work".

When I said these arguments to Debian people, they basically couldn't say anything against them, but didn't really want to drop their policy anyway and asked me to add a paragraph to COPYING file. So I did. But I still don't understand WHY is it wrong to dynamically link GPL and non-GPL code?

It almost looks like someone said it and people are repeating it like parrots. Is there any trustworthy legal opinion on this issue?

Comment 42 Mikuláš Patočka 2009-04-11 04:57:52 UTC
"Many warnings "pointer targets in passing argument <N> of <variable> differ in> signedness" in build.log"

Use -Wno-pointer-sign. I think it's easier to use it than to add many needless "unsigned char * -> char *" casts. Links is written with the policy that all characters are unsigned ... to avoid hard-to-detect bugs coming from the fact that different systems have different char signedness.

Comment 43 Lubomir Rintel 2009-04-11 06:48:26 UTC
(In reply to comment #38)
> (In reply to comment #35)
> > Matej: Though this is mostly a pre-nss-patch relic (current version of the nss
> > patch would not look for OpenSSL once NSS compat library is present)

> -- when I remove it and rebuild package, resulting package doesn't require
> openssl anyway:

If you bothered to read the above, I'm sure you would understand you are just re-stating what was already said.

> > Seriously, if you believe this is a review blocker, please re-read the
> > guidelines and elaborate on what use of BuildConflicts seems legitimate to you.
> 
> I have no clue what BuildConflicts is good for, and I have never seen it to be
> used before, but this is certainly not the case (see above).

Thank you for answering the review blocker part. Now that we're clear, could you please return the review flag back? I'm not going to reroll the package at this point, and am going to remove the BuildConflict upon import.

> 
> > Also, thank you for bringing this up once the review was finished, even though
> > this was present in the SPEC file since my first revision.  
> 
> I don't think there is any duty to be silent when I see stupidity in a package
> which is soon to come to Fedora -- I haven't thought it is necessary to review
> this package because I believed that ovasik will do proper job on it as usually
> he does. Only when I tried to rebuild this package on my computer I got hit by
> this, and so I jumped here before the damage would be hard to fix.  

You're completely right, there's no other term that would describe one extra BuildConflict better than "unfixable damage". I think your colleague will also be glad that to hear that you saved the mankind from unfixable damage caused by his improper review he wasted time on.

(In reply to comment #41)
> > # Incompatible with GPL
> > BuildConflicts: openssl-devel
> 
> Could anyone please explain, why so you think that it is illegal to dynamically
> link GPL and NON-GPL code?
> 
> My point of view:

If you believe this is the right place to troll about this (ever heard about the fedora-legal-list?), here you go: I believe that whether dynamic linking is creating derivative works was never tried in court and general consensus is that it's equivalent to static linking once it was done at build time (as opposed to say, browser plugins). Therefore links codebase linked with openssl is derivative work of the gpl and openssl license adds the advertisement clause which makes the license of the resulting product incompatible with GPL and thus is not allowed. IANAL, and I may be completly wrong and I reserve the right to "parrot" the decision of the lawyers instead of imposing my own uninformed decisions.

Comment 44 Lubomir Rintel 2009-04-11 06:52:55 UTC
(In reply to comment #42)
> "Many warnings "pointer targets in passing argument <N> of <variable> differ
> in> signedness" in build.log"
> 
> Use -Wno-pointer-sign. I think it's easier to use it than to add many needless
> "unsigned char * -> char *" casts. Links is written with the policy that all
> characters are unsigned ... to avoid hard-to-detect bugs coming from the fact
> that different systems have different char signedness.  

Makes sense (was bitten in the nose by such bug in powerpc KinoSearch just a week ago). I'll add the compiler flag upon import.

Comment 45 Mikuláš Patočka 2009-04-11 08:00:08 UTC
> "it's equivalent to static linking once it was done at build time (as
> opposed to say, browser plugins)."

And what if you create "dummy-openssl" library that contains nothing but empty function declarations and you license it under GPL and you build Links against it ... and then the user takes the Links binary built this way and loads it into memory with real OpenSSL --- is it license violation or not and who is doing the violation?

> "IANAL, and I may be completly wrong and I reserve the right to
> "parrot" the decision of the lawyers"

--- and where is the real "unparrotted" analysis?


I tried to install NSS --- it doesn't contain any general openssl functions, such as SSL_read, in /usr/include/nss. Am I doing something wrong ... do I need some other package? Which?

I managed to build Links against gnutls, but I'm getting runtime SSL errors. It needs some more debugging.

Comment 46 Lubomir Rintel 2009-04-11 08:18:39 UTC
(In reply to comment #45)
> > "it's equivalent to static linking once it was done at build time (as
> > opposed to say, browser plugins)."
> 
> And what if you create "dummy-openssl" 
[... blah blah blah blah...]

Did I say I am a lawyer? What makes you believe the court decisions always have sane technical basis and this report is here to discuss your licensing worries?

> > "IANAL, and I may be completly wrong and I reserve the right to
> > "parrot" the decision of the lawyers"
> 
> --- and where is the real "unparrotted" analysis?

Would not fedora-legal-list be more suitable for this kind of question?

I'm simply following the licensing guidelines?
http://fedoraproject.org/wiki/Licensing

> I tried to install NSS --- it doesn't contain any general openssl functions,
> such as SSL_read, in /usr/include/nss. Am I doing something wrong ... do I need
> some other package? Which?

Have you bothered looking at the BuildRequires? Or at least reading my comments? You need nss_compat_ossl.

Comment 47 Matěj Cepl 2009-04-11 22:43:59 UTC
(In reply to comment #43)
> If you bothered to read the above, I'm sure you would understand you are just
> re-stating what was already said.

OK, I may be stupid and you are über-smart, but could you please bow down to my stupidity and explain me why in the world you haven't removed that BuildConflicts then, when it is so obvious that it is not needed?

> Thank you for answering the review blocker part. Now that we're clear, could
> you please return the review flag back? I'm not going to reroll the package at
> this point, and am going to remove the BuildConflict upon import.

I will mark this package as correct, when I think it is correct. Really, is running

rpmdev-bumpspec -v -c '- removing unnecessary BuildConflicts' \
    -u 'Lubomir Rintel <lkundrak@...>' SPECS/links.spec
sed -e '/^BuildConflicts/d' SPECS/links.spec
rpmbuild -ba SPECS/links.spec
<however you call rsync to publish your packages>

too much to ask?

Comment 48 Lubomir Rintel 2009-04-12 17:15:46 UTC
here you go..

SPECS: http://v3.sk/~lkundrak/SPECS/links.spec
SRPMS: http://v3.sk/~lkundrak/SRPMS/links-2.2-7.fc11.src.rpm

Comment 49 Matěj Cepl 2009-04-12 19:45:02 UTC
one more non-review note for Mikuláš:

+ autoreconf -i
autoheader: WARNING: Using auxiliary files such as `acconfig.h', `config.h.bot'
autoheader: WARNING: and `config.h.top', to define templates for `config.h.in'
autoheader: WARNING: is deprecated and discouraged.
autoheader: 
autoheader: WARNING: Using the third argument of `AC_DEFINE' and
autoheader: WARNING: `AC_DEFINE_UNQUOTED' allows one to define a template without
autoheader: WARNING: `acconfig.h':
autoheader: 
autoheader: WARNING:   AC_DEFINE([NEED_FUNC_MAIN], 1,
autoheader: 		[Define if a function `main' is needed.])
autoheader: 
autoheader: WARNING: More sophisticated templates can also be produced, see the
autoheader: WARNING: documentation.

> Just keep in mind that the configure script must be generated with autoconf
> 2.13 --- that was current version 10 years ago when Links project was started
> and support for all the operating systems is tested with it.

I really don't want to do much about this (I am not programmer myself and I don't claim to have any more knowledge about autofoo than sitting in our office to one its maintainers), but just I am confused:

a) shouldn't be autofoo run only on the machine of the guy who creates the release tarball? So support on other operating systems shouldn't be that crucial, right? (I don't know why we do autoreconf here, and I don't care)
b) 2.13 is REALLY old. Which platform does really need it these days, which wouldn't have the later one?

Just questions, nothing to do with the package review.

Comment 50 Matěj Cepl 2009-04-12 19:56:52 UTC
[matej@viklef redhat]$ rpmlint -i SRPMS/links-2.2-7.fc11.src.rpm 
links.src:30: W: unversioned-explicit-provides webclient
The specfile contains an unversioned Provides: token, which will match all
older, equal, and newer versions of the provided thing.  This may cause update
problems and will make versioned dependencies, obsoletions and conflicts on
the provided thing useless -- make the Provides versioned if possible.

links.src:31: W: unversioned-explicit-provides text-www-browser
The specfile contains an unversioned Provides: token, which will match all
older, equal, and newer versions of the provided thing.  This may cause update
problems and will make versioned dependencies, obsoletions and conflicts on
the provided thing useless -- make the Provides versioned if possible.

1 packages and 0 specfiles checked; 0 errors, 2 warnings.

Wouldn't it be better to at least fake some version of those Provides so that we can obsolete them later (e.g., Provides: text-www-browser 2.0, Obsoletes: text-www-browser 1.0)?

Anyway, that's certainly not something which would make me not approve the package.

APPROVED.

Comment 51 Matěj Cepl 2009-04-12 19:57:13 UTC
New Package CVS Request
=======================
Package Name: links
Short Description: Web browser running in both graphics and text mode
Owners: lkundrak
Branches: EL-5 F-10

Comment 52 Kevin Fenzi 2009-04-13 15:45:16 UTC
cvs done.

Comment 53 Lubomir Rintel 2009-04-14 18:52:04 UTC
Imported and built.

Comment 54 Mikuláš Patočka 2009-04-14 20:01:25 UTC
"b) 2.13 is REALLY old. Which platform does really need it these days, which
wouldn't have the later one?"

I don't know. That is exactly the problem that it is impossible to check all the systems. Links runs on many systems that I have no longer access to (BeOS, AtheOS) or systems that I had never access to (RiscOS, AIX, Playstation, someone even run it on some Satellite receiver with Linux). Someone even found an old SunOS system without memmove, strstr or other, so Links contains definitions of these functions.

So I don't upgrade the autoconf version. There is no benefit from doing it and it might break systems silently.

BTW. do the autoconf developers have some QA test farm? I.e. a museum of old computers or emulators and testing the autoconf-generated script on all of them? I know that the configure script is supposed to run even on UnixV7 on PDP-11, the question is just how often someone tries it.

Comment 55 Mikuláš Patočka 2009-04-21 02:42:56 UTC
Regarding alternate crypto routines --- I couldn't get them working:

gnutls returns SSL_ERROR_ZERO_RETURN from SSL_get_error(c->ssl, SSL_connect(c->ssl))

I don't know why, gnutls says that their OpenSSL implementation is imcomplete.

nss+nss_compat_ossl-0.9.4 gets a segfault in SSL_new(context) in
s = SSL_ImportFD(templ_s, s);

So if someone wants to try to make it compile, let he does (and I would accept Links patches for that), for me it doesn't work and I don't care about it much.

Comment 56 Ondrej Vasik 2009-04-21 08:37:08 UTC
NSS SSL_new segfault already reported as https://bugzilla.redhat.com/show_bug.cgi?id=496643 and was recently fixed (and submitted for updates). Several others possible segfaults were fixed as well...

Comment 57 Mikuláš Patočka 2009-05-19 23:14:24 UTC
I am using links compiled with NSS for some time and I got connection error to this server: https://lug.wsu.edu/

NSS-compiled Links works for other https servers (like this bugzilla or mail at centrum.cz), but on that one it doesn't. Definitely, the transition won't be so easy...

Comment 58 Mikuláš Patočka 2009-05-19 23:15:26 UTC
(OpenSSL Links connects to https://lug.wsu.edu/ just fine)

Comment 59 Lubomir Rintel 2009-05-20 05:59:59 UTC
(In reply to comment #57)
> I am using links compiled with NSS for some time and I got connection error to
> this server: https://lug.wsu.edu/

May I dare to ask how does this relate to a closed review request? (And no, this question doesn't need an answer here.) If there's an issue that bothers you, please open a separate bug report.


Note You need to log in before you can comment on or make changes to this bug.