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
kwizart's scratch build of libglvnd-0.0.0-5git20151121.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11937356
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.
kwizart's scratch build of libglvnd-0.0.0-5git20151121.fc23.src.rpm for f23 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11937775
kwizart's scratch build of libglvnd-0.0.0-5git20151121.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11937984
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?
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.
(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 ?
(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.
?
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
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.
(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?
(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.
OK, can you post the updated spec file? The review is pretty hard without something concrete to base the review on ;)
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
Any news from reviewer ? Thx
Can you reply the comment about license in comment #c11?
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/
(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.
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
+ 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.
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/libglvnd
package built. thx for the review.
*** Bug 1412764 has been marked as a duplicate of this bug. ***