Bug 1259486 - Review Request: libglvnd - The GL Vendor-Neutral Dispatch library
Review Request: libglvnd - The GL Vendor-Neutral Dispatch library
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Zbigniew Jędrzejewski-Szmek
Fedora Extras Quality Assurance
:
: 1412764 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2015-09-02 13:25 EDT by Nicolas Chauvet (kwizart)
Modified: 2017-01-12 16:14 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-10-06 04:51:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
zbyszek: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nicolas Chauvet (kwizart) 2015-09-02 13:25:01 EDT
Spec URL: http://dl.kwizart.net/review/libglvnd.spec
SRPM URL: http://dl.kwizart.net/review/libglvnd-0.0.0-3git20150901.fc22.src.rpm
Description:
This is a work-in-progress implementation of the vendor-neutral dispatch layer
for arbitrating OpenGL API calls between multiple vendors on a per-screen
basis, as described by Andy Ritger's OpenGL ABI proposal.

Currently, only the GLX window-system API and OpenGL are supported, but in the
future this library may support EGL and OpenGL ES as well.
Fedora Account System Username: kwizart

This package is currently only supported on x86_64 i386 and armhfp
Comment 1 Upstream Release Monitoring 2015-11-21 10:32:51 EST
kwizart's scratch build of libglvnd-0.0.0-5git20151121.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11937356
Comment 2 Nicolas Chauvet (kwizart) 2015-11-21 10:42:40 EST
Spec URL: http://dl.kwizart.net/review/libglvnd.spec
SRPM URL: http://dl.kwizart.net/review/libglvnd-0.0.0-5git20151121.fc23.src.rpm
Description:
This is a work-in-progress implementation of the vendor-neutral dispatch layer
for arbitrating OpenGL API calls between multiple vendors on a per-screen
basis, as described by Andy Ritger's OpenGL ABI proposal.
Comment 3 Upstream Release Monitoring 2015-11-21 11:05:45 EST
kwizart's scratch build of libglvnd-0.0.0-5git20151121.fc23.src.rpm for f23 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11937775
Comment 4 Upstream Release Monitoring 2015-11-21 11:28:51 EST
kwizart's scratch build of libglvnd-0.0.0-5git20151121.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11937984
Comment 5 Zbigniew Jędrzejewski-Szmek 2016-01-08 21:26:48 EST
Why not use a tarball directly from github, and run autogen.sh during %build? In general it is a good idea to regenerate everything from pristine sources, and in this case it will avoid the need for manual steps and simplify maintenance and allow automatic version bumps.

[1] in the %description doesn't point anywhere.

%description should end in a dot.

make %{?_smp_mflags} V=1 → %make_build V=1

find %{buildroot} -name '*.la' -exec rm -f {} ';' → find %{buildroot} -name '*.la' -delete

(-f is bad, because it means "ignore errors", and if an error happened here, you actually want the build to fail. Anyway -delete is nicer.)

No %license file?

No %check?
Comment 6 Knut J BJuland 2016-01-09 07:11:20 EST
The latest NVIDIA beta driver now inlcudes open GL Vendor-Neutral Driver (GLVND) 
https://devtalk.nvidia.com/default/topic/908423. Fedora should include this as well.
Comment 7 Nicolas Chauvet (kwizart) 2016-01-14 08:31:53 EST
(In reply to Zbigniew Jędrzejewski-Szmek from comment #5)
First thx for your comment, I will try to fix some next week.

> Why not use a tarball directly from github, and run autogen.sh during
There is no tagged release yet, so I'm re-using the usual snapshot method instead.

> make %{?_smp_mflags} V=1 → %make_build V=1
What the main interest of this change ?

> find %{buildroot} -name '*.la' -exec rm -f {} ';' → find %{buildroot} -name
> '*.la' -delete
> 
> (-f is bad, because it means "ignore errors", and if an error happened here,
> you actually want the build to fail. Anyway -delete is nicer.)
We don't want to deal with installed .la files. And they won't disappear. But either they are here or not by default, it doesn't matter: we want them out! Also, we don't want the build to fail for any reason related to la files. So I think it's on purpose.
 
> No %license file?
> No %check?
I will check.

Do you have any others comments? Is this a full review? Can you take-over as a reviewer ?
Comment 8 Zbigniew Jędrzejewski-Szmek 2016-01-14 10:45:47 EST
(In reply to Nicolas Chauvet (kwizart) from comment #7)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #5)
> First thx for your comment, I will try to fix some next week.
> 
> > Why not use a tarball directly from github, and run autogen.sh during
> There is no tagged release yet, so I'm re-using the usual snapshot method
> instead.
Please see https://fedoraproject.org/wiki/Packaging:SourceURL#Commit_Revision.
Making snapshots by hand is mostly obsolete, and the guidelines
say you should use the automatically generated tarball.
 
> > make %{?_smp_mflags} V=1 → %make_build V=1
> What the main interest of this change ?
Using standard macros to make packages more similar. Making the spec file simpler.

> > find %{buildroot} -name '*.la' -exec rm -f {} ';' → find %{buildroot} -name
> > '*.la' -delete
> > 
> > (-f is bad, because it means "ignore errors", and if an error happened here,
> > you actually want the build to fail. Anyway -delete is nicer.)
> We don't want to deal with installed .la files. And they won't disappear.
> But either they are here or not by default, it doesn't matter: we want them
> out! Also, we don't want the build to fail for any reason related to la
> files. So I think it's on purpose.
No, you want the build to fail if the .la file cannot be deleted for some
reason (e.g. because of some access mode screw up). But that's mostly
hypothetical, the version you have works fine, and it's not my role as a reviewer
to force it. The reason I suggested the change is because the internal
-delete switch is simpler and does not fork anything, but if you don't like
it, it's OK.
  
> > No %license file?
> > No %check?
> I will check.
> 
> Do you have any others comments?
The %description for xorg-x11-glvnd is too terse. I'm missing a sentence that
starts with "This allows Xserver to ...".

"#This library only target few architectures use case"
This comment is hard to parse.

The license tag appears to be wrong.

Note: the License tag specifies the *effective* license of the *binary* package [1, 2]. So anything that is not part of the binary rpm (like install scripts) doesn't matter at all. If you combine LGPL with BSD or MIT, the effective license is LGPL. So, if a file in the binary rpm has at least one LGPL licensed file, that file is LGPL. If a file only had sources with more permissive licenses, than that file would have some other license. So you need to determine the effective license of all files in the binary rpm and put the result in License. Each of your binary subpackages effectively is one .so file (apart from READMEs and links), so this shouldn't be too complicated.

[1] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License:_field
[2] https://fedoraproject.org/wiki/Licensing:FAQ?rd=Licensing/FAQ#What_is_.22effective_license.22_and_do_I_need_to_know_that_for_the_License:_tag.3F

> Is this a full review? Can you take-over as
> a reviewer ?
Almost. Apart from Source tag and %description the package looks fine.
I don't know too much about the GLVN, but I can check the packaging side.
Comment 9 Zbigniew Jędrzejewski-Szmek 2016-02-20 17:32:25 EST
?
Comment 10 Nicolas Chauvet (kwizart) 2016-08-30 13:05:38 EDT
Sorry for the late answer, package improved in between.

Scratch build of the updated package:
f24: http://koji.fedoraproject.org/koji/taskinfo?taskID=15438557
rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=15438638

Spec URL: http://dl.kwizart.net/review/libglvnd.spec
SRPM URL: http://dl.kwizart.net/review/libglvnd-0.1.1-1.gitf7fbc4b.fc24.src.rpm
Comment 11 Zbigniew Jędrzejewski-Szmek 2016-09-01 12:06:54 EDT
libglvnd.x86_64: W: self-obsoletion xorg-x11-glvnd < 0.1.0 obsoletes xorg-x11-glvnd = 0.0.0-8

This looks wrong. I guess that the goal is for this package to be a replacement for xorg-x11-glvnd. Following https://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages, this should look like:

Provides: xorg-x11-glvnd = %{version}-%{release}
Obsoletes: xorg-x11-glvnd < 0.1.0


The pkg-config file looks bogus: Libs line is empty, Version is wrong. You might want to bug upstream about that.


%license does not need LGPLv2, afaict. The %license tag applies to the binary rpm, and the parts under GPL are build system components, irrelevant to the licensing of the binary rpm [https://fedoraproject.org/wiki/Licensing:FAQ?rd=Licensing/FAQ#Does_the_License:_tag_cover_the_SRPM_or_the_binary_RPM.3F].

Looks good otherwise.
Comment 12 leigh scott 2016-09-09 04:50:01 EDT
(In reply to Zbigniew Jędrzejewski-Szmek from comment #11)
> libglvnd.x86_64: W: self-obsoletion xorg-x11-glvnd < 0.1.0 obsoletes
> xorg-x11-glvnd = 0.0.0-8
> 
> This looks wrong. I guess that the goal is for this package to be a
> replacement for xorg-x11-glvnd. Following
> https://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.
> 2FReplacing_Existing_Packages, this should look like:
> 
> Provides: xorg-x11-glvnd = %{version}-%{release}
> Obsoletes: xorg-x11-glvnd < 0.1.0
> 
> 
> The pkg-config file looks bogus: Libs line is empty, Version is wrong. You
> might want to bug upstream about that.
> 
> 
> %license does not need LGPLv2, afaict. The %license tag applies to the
> binary rpm, and the parts under GPL are build system components, irrelevant
> to the licensing of the binary rpm
> [https://fedoraproject.org/wiki/Licensing:FAQ?rd=Licensing/
> FAQ#Does_the_License:_tag_cover_the_SRPM_or_the_binary_RPM.3F].
> 
> Looks good otherwise.

It looks like kwizart updated the spec and srpm in comment #10

# Introduced in f23
Provides: xorg-x11-glvnd = 0.0.0-8
Obsoletes: xorg-x11-glvnd < 0.1.0

Can you proceed with the review please?
Comment 13 Nicolas Chauvet (kwizart) 2016-09-09 07:36:11 EDT
(In reply to leigh scott from comment #12)
..
> It looks like kwizart updated the spec and srpm in comment #10
> 
> # Introduced in f23
> Provides: xorg-x11-glvnd = 0.0.0-8
> Obsoletes: xorg-x11-glvnd < 0.1.0
Yes, but this was bogus as mentioned earlier (self obsoletes)

Fixed locally with:
Provides: xorg-x11-glvnd = 0.1.0
Obsoletes: xorg-x11-glvnd < 0.1.0


Also fixed the Licences fied to:
License:        LGPLv2+


About, pkg-config Libs might have -lGLX at most, but I will need to check the patches from targeted to mesa about this. It's possible that there is nothing to link.
The version in pkg-config is accurate. Instead, I should probably bump to 0.1.999 as a package version.
Comment 14 Zbigniew Jędrzejewski-Szmek 2016-09-11 17:38:27 EDT
OK, can you post the updated spec file? The review is pretty hard without something concrete to base the review on ;)
Comment 15 Nicolas Chauvet (kwizart) 2016-09-15 05:59:45 EDT
Spec URL: http://dl.kwizart.net/review/libglvnd.spec
SRPM URL: http://dl.kwizart.net/review/libglvnd-0.2.999-1.git14f6283.fc24.src.rpm

koji scratch build for f26:
http://koji.fedoraproject.org/koji/taskinfo?taskID=15640225

Changelog:
- Update to 2.999 version
- Add EGL
Comment 16 Nicolas Chauvet (kwizart) 2016-10-05 04:35:42 EDT
Any news from reviewer ?
Thx
Comment 17 Zbigniew Jędrzejewski-Szmek 2016-10-05 08:01:35 EDT
Can you reply the comment about license in comment #c11?
Comment 18 leigh scott 2016-10-05 08:38:34 EDT
Debian used BSD-3-clause license for libglvnd

https://anonscm.debian.org/cgit/pkg-nvidia/libglvnd.git/tree/debian/copyright

To me the license looks like it should be MIT and BSD

https://paste.fedoraproject.org/444066/14756709/raw/
Comment 19 Nicolas Chauvet (kwizart) 2016-10-05 08:43:29 EDT
(In reply to Zbigniew Jędrzejewski-Szmek from comment #17)
> Can you reply the comment about license in comment #c11?

Thx for your reply.
I think I got your comment wrong and assumed AX_PTHREAD was something about code, so I've assumed LGPLv2+ (should have been LGPLv3+).
Also the 3-clause BSD is related to build tool, so not to take into account.
Now, indeed, as build tools are not involved in license computation, the result is that libglvnd is actually MIT (only).

Thx for your notice.
Comment 20 Nicolas Chauvet (kwizart) 2016-10-05 09:00:54 EDT
Spec URL: http://dl.kwizart.net/review/libglvnd.spec
SRPM URL: http://dl.kwizart.net/review/libglvnd-0.2.999-2.git14f6283.fc24.src.rpm


Changelog:
- Add Correct License: MIT
Comment 21 Zbigniew Jędrzejewski-Szmek 2016-10-05 14:46:50 EDT
+ package name is OK
+ license is acceptable (MIT)
+ license is specified correctly (afaict)
+ provides/requires look sane
+ scriptlets are OK
+ latest vesion
+ builds and install OK

Package is APPROVED.
Comment 22 Gwyn Ciesla 2016-10-05 17:29:06 EDT
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/libglvnd
Comment 23 Nicolas Chauvet (kwizart) 2016-10-06 04:51:39 EDT
package built.
thx for the review.
Comment 24 Björn "besser82" Esser 2017-01-12 16:14:36 EST
*** Bug 1412764 has been marked as a duplicate of this bug. ***

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