https://bugzilla.redhat.com/show_bug.cgi?id=1259486 Zbigniew Jędrzejewski-Szmek <zbyszek@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@xxxxxxxxxxxxxxxxx |zbyszek@xxxxxxxxx Flags| |fedora-review? --- Comment #8 from Zbigniew Jędrzejewski-Szmek <zbyszek@xxxxxxxxx> --- (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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review