COPR: https://copr.fedorainfracloud.org/coprs/avigne/libgenht Description: Generic hash table C library. This lib. is a foundation for future set of packages, delivering scripting facility in pcb-rnd. http://repo.hu/projects/pcb-rnd/ http://repo.hu/cgi-bin/pool.cgi?cmd=show&node=script_pkg The Mageia COPR build failed due to gcc-c++ ??? I explicitly BuildRequire gcc only ! Fedora Account System Username: avigne
I can review your package.
I just noticed this now - can you add the links to the .spec and SRPM file in a comment, like they are present in the review template? The automated review tool looks for them in the initial bug text and subsequent comments. It should look like this: Spec URL: paste URL here SRPM URL: paste URL here Since you already have a working COPR build, getting those links would be as easy as opening one of the output/chroot directories of your build (e.g. clicking "fedora-rawhide-x86_64" in the table at the bottom of the build details page), and copying-and-pasting the hyperlinks to the .spec and .src.rpm files.
Spec URL: https://copr-be.cloud.fedoraproject.org/results/avigne/libgenht/fedora-rawhide-x86_64/00826178-libgenht/libgenht.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/avigne/libgenht/fedora-rawhide-x86_64/00826178-libgenht/libgenht-1.0.1-1.fc30.src.rpm Description: Generic hash table C library Fedora Account System Username: avigne
Initial comments: 1) Package name Did you consider naming the main package just "genht", and adding a "-libs" sub-package? The upstream project is called "genht", after all. That would also make defining "generic_name" unnecessary, since you could just use "%{name}" everywhere, instead. 2) Build system problems a) The upstream project seems to have some isses with their build system. I don't think it honors build flags from the environment (e.g. CFLAGS, LDFLAGS) - but you didn't set them in the .spec file, either. b) You should use the "%set_build_flags" macro (see [0]) at the top of %build (it sets CFLAGSm LDFLAGS, etc. to the fedora defaults), and change "make" to "%make_build", and "make install" to "%make_install" - those macros set some important flags for fedora builds (including DESTDIR). c) The upstream build system also doesn't define an SONAME for the library ("-Wl,-soname,libfoo.so.1" flag) - that's missing from the upstream Makefile as well, and will throw RPM off - right now, it renders the "-devel" package uninstallable. If upstream won't introduce an SONAME, you will have to add one for the fedora build. d) Don't include the static library in your package (and drop the "-static" sub-package), unless you are sure that other packages will want to use it, and have a good reason to do so. e) The Makefile also defines a binary, is that not used for anything? I'm curious if you could work with the upstream project to add support for a proper build system ("porting" the Makefile to meson could probably be done within an hour, and it would make it more portable than a crusty Makefile). 3) .spec issues You don't need to use macros in summary, description, etc. It just makes the .spec less readable, IMO. But that's a matter of taste, I guess. I also think you can just remove all the comments. They are pretty redundant with the code below them. If you want to keep them, move the ones above %prep and %install into the sections, otherwise RPM will probably treat them as belonging to the previous section. a) By using a glob for the shared library ("%{_libdir}/libgenht.so.*") in line 54, which conceals the library version, you increase the likelihood of unintended breakage because of SONAME bumps. Use "%{_libdir}/libgenht.so.1*") instead, as documented in the Guidelines. b) The summary should not start with "A" and not end with ".". Change it to: "Simple generic hash table implementation in C". c) The "-devel" sub-package must depend on the package containing the shared library (right now, libgenht; if you decide to change the package name to "genht", then "genht-libs") like this: Requires: %{name}%{?_isa} = %{version}-%{release} # for libgenht or this: Requires: %{name}-libs%{?_isa} = %{version}-%{release} # for genht-libs d) What does the comment "# Untar the correct tar.gz file" mean? I think you can just drop it. (The benefit of renaming the main package to "genht" would be that you can drop the "-n" argument from the "%autosetup" call.) e) The following entries in %files lists are equivalent: %dir %{_includedir}/%{generic_name} %{_includedir}/%{generic_name}/* and %{_includedir}/%{generic_name}/ This is probably just a stylistic issue, so you decide which one you'd like to use. [0]: https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/master/f/buildflags.md
Thanks a lot Fabio for this detailed feed-back. I will use your numbering to answer. 1) Originally, I named it genht.spec, and developed the spec file as you suggested (it was easier :). But when I submitted to the main developer, he said Debian was already packaging the lib as "libgenht1", under https://packages.debian.org/sid/amd64/libgenht1 and he prefers the package name to be "libgenht". I have the genht.spec file. Who will have the last word ? 2a, 2b) I should investigate and implement your proposal... 2c) I create a .so symlink from the .spec file. The ln-s command is in between pushd/popd directives. 2d) Again, originally I did not have this -static subpackage. But the main developer wanted to distribute the archive lib. too. He plans to use this library for other applications. I think, by proper packaging the other applications, we can avoid this static lib. Correct ? 2e) The sole binary is a "trivial" example of how to use the lib. Not needed in the distro. I am used to work with the main developer. Never, ever he would accept meson, because it is Python. He is C89 to the bare. 3) You suggest to use the "genht" text iso a macro. For a "final" spec file (close to "approved" state), I agree. I think I will simplify that. About comments, I prefer to keep them, but did not realize this "section" subtle thing. I will simplify and move them around. 3a) You are right. Have to change that. 3b) Agree 3c) I have to implement your suggestion 3d) Will change the comment, as I think the name "libgenht" will be kept. 3e) %dir %{_includedir}/%{generic_name} is to own the directory, while %{_includedir}/%{generic_name}/* describes all the files in the dir. It seems not equivalent to me ? Thank you again for your time. As soon as I can, I will experiment those changes and update my COPR. I'll then post a comment here, linking to the new proposal.
(In reply to Alain V. from comment #5) > Thanks a lot Fabio for this detailed feed-back. I will use your numbering to > answer. That was the idea. ;) I find it makes it a lot easier to structure suggestions and answers. > 1) Originally, I named it genht.spec, and developed the spec file as you > suggested (it was easier :). > But when I submitted to the main developer, he said Debian was already > packaging the lib as "libgenht1", under > https://packages.debian.org/sid/amd64/libgenht1 > and he prefers the package name to be "libgenht". > I have the genht.spec file. Who will have the last word ? Well, you have the last word, as the maintainer of this package. I also know that debian package naming has different heuristics than fedora's. Applying the heuristics from the fedora Naming Guidelines [0] would lead to "genht" name (since it's what the upstream project is called, and it's what the tarball is named). But since the upstream developer would prefer libgenht, that's usually respected, so I would stick to "libgenht". In fedora, the "upstream name" is per convention usually defined as "srcname", if it is different than %{name} (like this: "%global srcname genht"). > 2a, 2b) I should investigate and implement your proposal... You just need to add the line "%set_build_flags" as the first line after "%build". It sets all required environment variables. After doing that, you only have to check if "make" respects those variables. If not, you will have to patch the Makefile to do so. Replacing "make" with "%make_build" and "make install" with "%make_install" is also easy. > 2c) I create a .so symlink from the .spec file. The ln-s command is in > between pushd/popd directives. That's not the issue. The library's ELF metadata is incomplete, because the Makefile is missing those linker flags. I'd notify the upstream developer about that. The missing entry in the LDFLAGS would probably look like this: "-Wl,-soname,$(LIBSO)" > 2d) Again, originally I did not have this -static subpackage. But the main > developer wanted to distribute the archive lib. too. > He plans to use this library for other applications. > I think, by proper packaging the other applications, we can avoid this > static lib. Correct ? Yes. Usually the static library isn't needed. So for now I'd drop the -static sub-package. If it *really* will be needed in the future, you can always add it back. > 2e) The sole binary is a "trivial" example of how to use the lib. Not needed > in the distro. Good. Just checking :) > I am used to work with the main developer. Never, ever he would accept > meson, because it is Python. He is C89 to the bare. Well, that doesn't make things easier for you as a packager ... > 3) > You suggest to use the "genht" text iso a macro. For a "final" spec file > (close to "approved" state), I agree. I think I will simplify that. What I meant to say here was: You don't need to replace every occurrence of "genht" with a macro. For example, it doesn't really help in summaries and descriptions, but makes the .spec file less readable for a human. > About comments, I prefer to keep them, but did not realize this "section" > subtle thing. I will simplify and move them around. Alright, that's your decision. And yeah, that's probably a really subtle difference. It's good to know that the %prep, %build, %install, %check, etc. sections are just parsed as "sh" scripts, though. > 3d) Will change the comment, as I think the name "libgenht" will be kept. You're right. > 3e) > %dir %{_includedir}/%{generic_name} > is to own the directory, while > %{_includedir}/%{generic_name}/* > describes all the files in the dir. It seems not equivalent to me ? But it is equivalent. Both - %{_includedir}/%{srcname} - %{_includedir}/%{srcname}/ indicate the same thing: This package owns this directory and everything below it (although the first option makes it less clear that it's a directory). I prefer the second option. On the other hand, %dir %{_includedir}/%{srcname} indicates that this package only owns the "empty" directory, and nothing below it (which can be useful for empty directories!). That's why you have to add the second line %{_includedir}/%{srcname}/* for the package to own the actual directory contents, too. > > Thank you again for your time. > As soon as I can, I will experiment those changes and update my COPR. I'll > then post a comment here, linking to the new proposal. Sure! I hope my comments are helpful. I know that starting with packaging can be a bit intimidating, and has a steep learning curve. Don't hesitate to ask if you have more questions. [0]: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/
Created attachment 1508379 [details] RPM creation fails
I need to patch src/Makefile ? How to do that, and handle this action 1. Locally 2. in a COPR 3. in the git allocated for Fedora ? How do I know what %make_install is doing in this case ? Thanks for your help
(In reply to Alain V. from comment #8) > I need to patch src/Makefile ? Yes. > How to do that, and handle this action > 1. Locally > 2. in a COPR > 3. in the git allocated for Fedora ? I'll write up the required steps. Give me a minute. > How do I know what %make_install is doing in this case ? You can always see what macros are expanded to by running: rpm --eval "%make_install" > Thanks for your help
To create a patch, I usually do it with a temporary git repository (it makes generating the patches easier than plain "diff"): 1) Extract the upstream tarball 2) Initialise a git repository with pristine contents: $ git init && git add . && git commit -m import 3) Make my modifications 4) Generate the patch (I tend to number mine from 00 upwards): $ git diff --patch > $HOME/rpmbuild/SOURCES/00-location.patch 5) Include the patch in the .spec file: Patch0: 00-location.patch 6) Tell autosetup which patch level to apply: %autosetup -n %{srcname}-%{version} -p1 7) Proceed as usual (patches are included in the .src.rpm file when running rpmbuild -bs *.spec). I've added a patch for the Makefile and a .spec file with the necessary modifications for you to inspect in the attached .src.rpm file.
Created attachment 1508381 [details] libgenht-1.0.1-2.fc29.src.rpm
I forgot to mention: Patches are mostly just treated just like sources for package builds. - When using plain rpmbuild, the live in the rpmbuilds/SOURCES directory. - Uploading the generated .src.rpm files to COPR just works (because they bundle the necessary patches). - fedpkg repositories contain them too, the only difference between source tarballs and patches here is that sources are uploaded to a lookaside cache seperately, and patches are just committed to the git repository.
Thank you very much Fabio for these detailed explanations. I was able to make sense of them, reproduce at my end, and build another version of this package in COPR: Spec URL: https://copr-be.cloud.fedoraproject.org/results/avigne/libgenht/fedora-rawhide-x86_64/00829352-libgenht/libgenht.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/avigne/libgenht/fedora-rawhide-x86_64/00829352-libgenht/libgenht-1.0.1-2.fc30.src.rpm My questions/remarks: A. Why COPR Mageia build is failing ? B. I kept the static subpackage and would consider this as an option (available to who wants to install the archive lib.) C. As I need to patch the Makefile, why not inserting a Makefile rule to create the lib.so file, instead of linking it in the .spec file ? Let me know your views on this new attempt... BR, Alain.
A) The mageia build is failing due to a network connectivity error, which you can see at the bottom of "root.log". You can safely ignore this. B) I think you should reconsider dropping the static library. Maybe the relevant section of the Packaging Guidelines can help clear some confusion? [0] C) That's certainly possible. If you want to modify my patch to a rule to create the additional symlink, go ahead. By the way, I saw you asked some questions on the -devel mailing list; and yes, the wiki is still the authoritative source for most(=) fedora documentation. Only some parts have moved over to the new website (including the Packaging Guidelines), but not others (for example, the Update Process Guidelines, etc.). [0]: https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-libraries Final review: Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed ===== ISSUES ===== [!]: License file installed when any subpackage combination is installed. => Add the %license and %doc lines from the main package to the -static sub-package, if you decide to keep it. [?]: Patches link to upstream bugs/comments/lists or are otherwise justified. => Ask the upstream developer if they want to include the missing linker flag in the upstream Makefile. Otherwise the package looks good now (see details below). ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. [x]: Development (unversioned) .so files in -devel subpackage, if present. Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. [!]: License file installed when any subpackage combination is installed. [x]: Package does not own files or directories owned by other packages. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [x]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 10240 bytes in 1 files. [?]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: Static libraries in -static or -devel subpackage, providing -devel if present. Note: Package has .a files: libgenht-static. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [x]: Fully versioned dependency in subpackages if applicable. [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [?]: Patches link to upstream bugs/comments/lists or are otherwise justified. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: Package should compile and build into binary rpms on all supported architectures. [-]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). [x]: Rpmlint is run on all installed packages. [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: libgenht-1.0.1-2.fc30.x86_64.rpm libgenht-devel-1.0.1-2.fc30.x86_64.rpm libgenht-static-1.0.1-2.fc30.x86_64.rpm libgenht-debuginfo-1.0.1-2.fc30.x86_64.rpm libgenht-debugsource-1.0.1-2.fc30.x86_64.rpm libgenht-1.0.1-2.fc30.src.rpm libgenht.x86_64: W: spelling-error %description -l en_US genht -> gent, gen ht, gen-ht libgenht-devel.x86_64: W: no-documentation libgenht-static.x86_64: W: spelling-error Summary(en_US) genht -> gent, gen ht, gen-ht libgenht-static.x86_64: W: spelling-error %description -l en_US genht -> gent, gen ht, gen-ht libgenht-static.x86_64: W: no-documentation libgenht.src: W: spelling-error %description -l en_US genht -> gent, gen ht, gen-ht 6 packages and 0 specfiles checked; 0 errors, 6 warnings. Rpmlint (debuginfo) ------------------- Checking: libgenht-debuginfo-1.0.1-2.fc30.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- sh: /usr/bin/python: No such file or directory libgenht-debugsource.x86_64: W: invalid-url URL: http://repo.hu/projects/genht <urlopen error [Errno -2] Name or service not known> libgenht-debuginfo.x86_64: W: invalid-url URL: http://repo.hu/projects/genht <urlopen error [Errno -2] Name or service not known> libgenht-devel.x86_64: W: invalid-url URL: http://repo.hu/projects/genht <urlopen error [Errno -2] Name or service not known> libgenht-devel.x86_64: W: no-documentation libgenht-static.x86_64: W: spelling-error Summary(en_US) genht -> gent, gen ht, gen-ht libgenht-static.x86_64: W: spelling-error %description -l en_US genht -> gent, gen ht, gen-ht libgenht-static.x86_64: W: invalid-url URL: http://repo.hu/projects/genht <urlopen error [Errno -2] Name or service not known> libgenht-static.x86_64: W: no-documentation libgenht.x86_64: W: spelling-error %description -l en_US genht -> gent, gen ht, gen-ht libgenht.x86_64: W: invalid-url URL: http://repo.hu/projects/genht <urlopen error [Errno -2] Name or service not known> 5 packages and 0 specfiles checked; 0 errors, 10 warnings. Requires -------- libgenht-debugsource (rpmlib, GLIBC filtered): libgenht-debuginfo (rpmlib, GLIBC filtered): libgenht-devel (rpmlib, GLIBC filtered): libgenht(x86-64) libgenht.so.1.0.1()(64bit) libgenht-static (rpmlib, GLIBC filtered): libgenht(x86-64) libgenht (rpmlib, GLIBC filtered): libc.so.6()(64bit) rtld(GNU_HASH) Provides -------- libgenht-debugsource: libgenht-debugsource libgenht-debugsource(x86-64) libgenht-debuginfo: debuginfo(build-id) libgenht-debuginfo libgenht-debuginfo(x86-64) libgenht-devel: libgenht-devel libgenht-devel(x86-64) libgenht-static: libgenht-static libgenht-static(x86-64) libgenht: libgenht libgenht(x86-64) libgenht.so.1.0.1()(64bit) Source checksums ---------------- http://repo.hu/projects/genht/releases/genht-1.0.1.tar.gz : CHECKSUM(SHA256) this package : dddfe6b093e365f24cab0a642a15d23aebb0680c9f7a8e7d60ed6d79adc54f40 CHECKSUM(SHA256) upstream package : dddfe6b093e365f24cab0a642a15d23aebb0680c9f7a8e7d60ed6d79adc54f40 Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02 Command line :/usr/bin/fedora-review -b 1650987 -m fedora-rawhide-x86_64 Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, Shell-api, C/C++ Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6
Created attachment 1510365 [details] Remove the static subpackage
Thank you so much Fabio for the review/advises. I understand Fedora policy, to not include .a archive libs. I list only 7 such files on my system... $ ls -1 /usr/lib64/*.a|wc -l 7 To simplify the situation, I propose to drop the static sub-package and such, I patched even more the Makefile : Spec URL: https://copr-be.cloud.fedoraproject.org/results/avigne/libgenht/fedora-rawhide-x86_64/00832049-libgenht/libgenht.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/avigne/libgenht/fedora-rawhide-x86_64/00832049-libgenht/libgenht-1.0.1-3.fc30.src.rpm The mageia-cauldron is failing again, and looking at root.log.gz, I don't see the reason why. Somebody should report something somewhere ? Concerning -Wl,-soname,$(LIBSO) , upstream Makefile will NOT include this statement, as it is a gcc flag only, and upstream is "compiler agnostic". I hope what I propose makes sense ? Alain
(In reply to Alain V. from comment #15) > Created attachment 1510365 [details] > Remove the static subpackage Wrong file, this should be removed...
(In reply to Alain V. from comment #16) > Thank you so much Fabio for the review/advises. > > I understand Fedora policy, to not include .a archive libs. I list only 7 > such files on my system... > $ ls -1 /usr/lib64/*.a|wc -l > 7 Yeah, and those 7 probably shouldn't be there either. > To simplify the situation, I propose to drop the static sub-package and > such, I patched even more the Makefile : > > Spec URL: > https://copr-be.cloud.fedoraproject.org/results/avigne/libgenht/fedora- > rawhide-x86_64/00832049-libgenht/libgenht.spec > SRPM URL: > https://copr-be.cloud.fedoraproject.org/results/avigne/libgenht/fedora- > rawhide-x86_64/00832049-libgenht/libgenht-1.0.1-3.fc30.src.rpm > > The mageia-cauldron is failing again, and looking at root.log.gz, I don't > see the reason why. > Somebody should report something somewhere ? It's a known issue in mock / COPR, I think - failed downloads due to network issues aren't retried. > Concerning -Wl,-soname,$(LIBSO) , upstream Makefile will NOT include this > statement, as it is a gcc flag only, and upstream is "compiler agnostic". I don't think that's true, since the "-Wl," flag only passes through the following arguments to the linker. But alright, if upstream doesn't want to add it, keep the flag in your patch for the Makefile. > I hope what I propose makes sense ? > Alain Yes, it does, the package looks good now!
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/libgenht
It looks like you've successfully built this package on rawhide. Please don't forget to close review requests after they are concluded.