https://bugzilla.redhat.com/show_bug.cgi?id=1259486
Bug ID: 1259486 Summary: Review Request: libglvnd - The GL Vendor-Neutral Dispatch library Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: kwizart@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
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
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #1 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- kwizart's scratch build of libglvnd-0.0.0-5git20151121.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11937356
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #2 from Nicolas Chauvet (kwizart) kwizart@gmail.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #3 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- kwizart's scratch build of libglvnd-0.0.0-5git20151121.fc23.src.rpm for f23 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11937775
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #4 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- kwizart's scratch build of libglvnd-0.0.0-5git20151121.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11937984
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |zbyszek@in.waw.pl
--- Comment #5 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- 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?
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #6 from Knut J BJuland knutjbj@online.no --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #7 from Nicolas Chauvet (kwizart) kwizart@gmail.com --- (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 ?
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |zbyszek@in.waw.pl Flags| |fedora-review?
--- Comment #8 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- (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_.22eff...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #9 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- ?
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #10 from Nicolas Chauvet (kwizart) kwizart@gmail.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #11 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- 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_Exi..., 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_Licen...].
Looks good otherwise.
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #12 from leigh scott leigh123linux@googlemail.com --- (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?
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #13 from Nicolas Chauvet (kwizart) kwizart@gmail.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #14 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- OK, can you post the updated spec file? The review is pretty hard without something concrete to base the review on ;)
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #15 from Nicolas Chauvet (kwizart) kwizart@gmail.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
Nicolas Chauvet (kwizart) kwizart@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(zbyszek@in.waw.pl | |)
--- Comment #16 from Nicolas Chauvet (kwizart) kwizart@gmail.com --- Any news from reviewer ? Thx
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(zbyszek@in.waw.pl | |) |
--- Comment #17 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- Can you reply the comment about license in comment #c11?
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #18 from leigh scott leigh123linux@googlemail.com --- Debian used BSD-3-clause license for libglvnd
https://anonscm.debian.org/cgit/pkg-nvidia/libglvnd.git/tree/debian/copyrigh...
To me the license looks like it should be MIT and BSD
https://paste.fedoraproject.org/444066/14756709/raw/
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #19 from Nicolas Chauvet (kwizart) kwizart@gmail.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #20 from Nicolas Chauvet (kwizart) kwizart@gmail.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+
--- Comment #21 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- + 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
--- Comment #22 from Jon Ciesla limburgher@gmail.com --- Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/libglvnd
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
Nicolas Chauvet (kwizart) kwizart@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |CURRENTRELEASE Last Closed| |2016-10-06 04:51:39
--- Comment #23 from Nicolas Chauvet (kwizart) kwizart@gmail.com --- package built. thx for the review.
https://bugzilla.redhat.com/show_bug.cgi?id=1259486
Björn "besser82" Esser bjoern.esser@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |hdegoede@redhat.com
--- Comment #24 from Björn "besser82" Esser bjoern.esser@gmail.com --- *** Bug 1412764 has been marked as a duplicate of this bug. ***
package-review@lists.fedoraproject.org