[Bug 1983601] Review Request: guile3 - A GNU implementation of Scheme for application extensibility
https://bugzilla.redhat.com/show_bug.cgi?id=1983601 Aleksei Bavshin changed: What|Removed |Added CC||alebast...@gmail.com --- Comment #5 from Aleksei Bavshin --- Apologies for intruding, this review made me genuinely curious about the versioning of guile in Fedora. Once this is approved, we'll have 3 packages: - guile (unversioned) - legacy and unmaintained since 2017 2.0 branch, provides unversioned /usr/bin/guile - guile22 - legacy, but still maintained 2.2.x branch, provides /usr/bin/guile2.2 - guile3 - latest release, provides /usr/bin/guile3.0 Q1: This seems kind of backwards to me. Guidelines do not require unversioned package to be the latest one (I just confirmed with the text), but I've seen that being advised on devel@. It's also confusing for me as a user, since I expect `dnf install guile` to get a current package instead of the deprecated one (but that's not as important). Somewhat related: is our gdb package built against guile 2.0 _only_ because it's unversioned and is assumed to be the right version to use by default? Gdb supports all the versions up to 3.0. Q2: Guile upstream seems to have a solid versioning strategy: each minor release is a new version of the language incompatible with a previous one. 2.0, 2.2, 3.0,.. 3.2? In regards to that, guile3.0 could be more appropriate name for the package. If the upstream is alive enough to release 3.2 in however many years that'll take, things will get a bit confusing :) -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1983601] Review Request: guile3 - A GNU implementation of Scheme for application extensibility
https://bugzilla.redhat.com/show_bug.cgi?id=1983601
--- Comment #4 from Christopher Engelhard ---
Looking good!
Just two more things:
- the %files section is missing %dir %{_datadir}/guile/site/%{mver}
(/usr/share/guile/site/3.0 still has no owner)
- your patch for the chroot thing seems to be replacing the chroot() call with
chdir() instead of doing chdir-then-chroot, is that intended? It would also be
OK to leave it unpatched for now and just report the issue upstream.
Regarding the tests, it did some more runs and it is all a bit inconsitent for
me as well, there were also irreproducible issues with the stack overflow tests
that were most likely caused by that test just eating way too much memory.
Could be that that also happens for the guile-compile test. Anyway, disabling
those inconsistently failing tests if it happens again is perfectly fine.
--
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@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
Do not reply to spam on the list, report it:
https://pagure.io/fedora-infrastructure
[Bug 1983601] Review Request: guile3 - A GNU implementation of Scheme for application extensibility
https://bugzilla.redhat.com/show_bug.cgi?id=1983601 Tomas Korbar changed: What|Removed |Added Flags|needinfo?(tkor...@redhat.co | |m) | --- Comment #3 from Tomas Korbar --- Hi Christopher, thanks for your review. I think i managed to address all of the issues One question from my side: Is /usr/lib64/guile/3.0/extensions/guile-readline.so* something that is supposed to be generally available or is that guile-internal like the stuff in .../ccache ? - Internal IMO, it is guiles module Package Review == Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated Issues: === - [MUST] If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. Please add BuildRequires: gcc - Done - [MUST] There are some directories that should be owned by this package but aren't: /usr/share/guile/site/3.0, /usr/share/guile/3.0/scheme, /usr/share/guile/site Please add them to %files - Done - [MUST] Texinfo files are installed using install-info in %post and %preun if package has .info files. - Done. - [MUST] Some parts of the code seem to be MIT/GPL/PublicDomain licensed, rather than LGPL, please check if these end up in the package MIT License: - guile-3.0.7/doc/ref/sxml-match.texi - guile-3.0.7/module/ice-9/quasisyntax.scm - guile-3.0.7/module/srfi/srfi-38.scm - guile-3.0.7/module/srfi/srfi-41.scm - guile-3.0.7/module/srfi/srfi-45.scm - guile-3.0.7/module/srfi/srfi-64/testing.scm - guile-3.0.7/module/srfi/srfi-67/compare.scm - guile-3.0.7/module/srfi/srfi-71.scm - guile-3.0.7/module/sxml/sxml-match.ss Public domain - guile-3.0.7/module/ice-9/match.upstream.scm GPL - guile-3.0.7/guile-readline/ice-9/readline.scm GPLv3+ - guile-3.0.7/guile-readline/readline.c - guile-3.0.7/guile-readline/readline.h - guile-3.0.7/libguile/libguile-3.0-gdb.scm - guile-3.0.7/module/language/elisp/compile-tree-il.scm - These files are included so i added licenses. It is unfortunate - [SHOULD] Please add dependencies on pkg-config and change (build)requires to pkgconfig(). See https://docs.fedoraproject.org/en-US/packaging-guidelines/PkgConfigBuildRequires/ - Done - [SHOULD] Please add comment with the rationale for the bundled provide, e.g. that guile ships a patched version of a git checkout of localcharset.[h,c] from gnulib, as intended by gnulib upstream. - Done - [SHOULD] Please also add the version of the bundled gnulib. Brief look at upstream seems to indicate that it is v0.1-1157-gb03f418, but that doesn't really line up with the numbering scheme the fedora package uses (just Version: 0 coupled to a Release: with git suffix) or with the way gnulib works in general I'd suggest just adding the Version as a comment. Git history of the file is here: https://git.savannah.gnu.org/gitweb/?p=guile.git;a=commit;h=a91b95cca2d397c84f8b9bbd602d40209a7092ce - Done - [SHOULD] Please list libguile version explicitly in %files, rather than via glob (i.e as libguile*.so..X.Y.Z or at least .X*) to protect against accidental library version bumps. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_shared_libraries - Done - [SHOULD] Please add brief comments and/or upstream links to patches to explain what they do. - Done - [SHOULD] Doesn't build on aarch64 due to test fail: error: 'guild compile' failed to remove 't-guild-compile-100826.go.64hinA' FAIL: test-guild-compile wrote `/builddir/build/BUILD/guile-3.0.7/cache/guile/ccache/3.0-LE-8-4.5/builddir/build/BUILD/guile-3.0.7/test-suite/standalone/test-signal-fork.go' parent: child: 100919101058 - I did not encounter any such issue during scratch builds. It seems that some of the tests are not really stable. If this test will keep failing in the future, then i will disable it. - [EXTRA] [RPMLINT] [PATCH] missing-call-to-chdir-with-chroot is most likely triggered by libguile/posix.c, and for once seems to me to not be a false positive. Please check (I'm not that much of a C expert) and consider adding a patch that calls chdir() before chrooting, as this is a potential security issue. - Done - [EXTRA] [PATCH] configure.ac contains AC_PROG_LIBTOOL, which is deprecated. Maybe add a patch to replace it with LT_INIT? No further changes should be necessary. See https://www.gnu.org/software/libtool/manual/html_node/LT_005fINIT.html - Done - [EXTRA] [RPMLINT] [PATCH] Fix FSF address. - Upstream has been notified but i am not comfortable with downstream patching license. - [NON-ISSUE] Further (informative, non-issue) comments inline below, enclosed in . Updated specfile and srpm are in the same place, the originals were.
[Bug 1983601] Review Request: guile3 - A GNU implementation of Scheme for application extensibility
https://bugzilla.redhat.com/show_bug.cgi?id=1983601 --- Comment #2 from Christopher Engelhard --- I've also tested the s390x and ppc64le builds now. - Build on s390x failed during the libguile compilation when trying to assemble documentation. - ppc64le worked, but you might run into problems on the builders there during the %check phase, on my machine test-stack-overflow took hours to complete while consuming practically all available memory. -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1983601] Review Request: guile3 - A GNU implementation of Scheme for application extensibility
https://bugzilla.redhat.com/show_bug.cgi?id=1983601 Christopher Engelhard changed: What|Removed |Added Flags||needinfo?(tkor...@redhat.co ||m) --- Comment #1 from Christopher Engelhard --- Full review below, please let me know if you have any questions. The package is generally OK, apart from the issues listed below. One question from my side: Is /usr/lib64/guile/3.0/extensions/guile-readline.so* something that is supposed to be generally available or is that guile-internal like the stuff in .../ccache ? Package Review == Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated Issues: === - [MUST] If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. Please add BuildRequires: gcc - [MUST] There are some directories that should be owned by this package but aren't: /usr/share/guile/site/3.0, /usr/share/guile/3.0/scheme, /usr/share/guile/site Please add them to %files - [MUST] Texinfo files are installed using install-info in %post and %preun if package has .info files. - [MUST] Some parts of the code seem to be MIT/GPL/PublicDomain licensed, rather than LGPL, please check if these end up in the package MIT License: - guile-3.0.7/doc/ref/sxml-match.texi - guile-3.0.7/module/ice-9/quasisyntax.scm - guile-3.0.7/module/srfi/srfi-38.scm - guile-3.0.7/module/srfi/srfi-41.scm - guile-3.0.7/module/srfi/srfi-45.scm - guile-3.0.7/module/srfi/srfi-64/testing.scm - guile-3.0.7/module/srfi/srfi-67/compare.scm - guile-3.0.7/module/srfi/srfi-71.scm - guile-3.0.7/module/sxml/sxml-match.ss Public domain - guile-3.0.7/module/ice-9/match.upstream.scm GPL - guile-3.0.7/guile-readline/ice-9/readline.scm GPLv3+ - guile-3.0.7/guile-readline/readline.c - guile-3.0.7/guile-readline/readline.h - guile-3.0.7/libguile/libguile-3.0-gdb.scm - guile-3.0.7/module/language/elisp/compile-tree-il.scm - [SHOULD] Please add dependencies on pkg-config and change (build)requires to pkgconfig(). See https://docs.fedoraproject.org/en-US/packaging-guidelines/PkgConfigBuildRequires/ - [SHOULD] Please add comment with the rationale for the bundled provide, e.g. that guile ships a patched version of a git checkout of localcharset.[h,c] from gnulib, as intended by gnulib upstream. - [SHOULD] Please also add the version of the bundled gnulib. Brief look at upstream seems to indicate that it is v0.1-1157-gb03f418, but that doesn't really line up with the numbering scheme the fedora package uses (just Version: 0 coupled to a Release: with git suffix) or with the way gnulib works in general I'd suggest just adding the Version as a comment. Git history of the file is here: https://git.savannah.gnu.org/gitweb/?p=guile.git;a=commit;h=a91b95cca2d397c84f8b9bbd602d40209a7092ce - [SHOULD] Please list libguile version explicitly in %files, rather than via glob (i.e as libguile*.so..X.Y.Z or at least .X*) to protect against accidental library version bumps. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_shared_libraries - [SHOULD] Please add brief comments and/or upstream links to patches to explain what they do. - [SHOULD] Doesn't build on aarch64 due to test fail: error: 'guild compile' failed to remove 't-guild-compile-100826.go.64hinA' FAIL: test-guild-compile wrote `/builddir/build/BUILD/guile-3.0.7/cache/guile/ccache/3.0-LE-8-4.5/builddir/build/BUILD/guile-3.0.7/test-suite/standalone/test-signal-fork.go' parent: child: 100919101058 - [EXTRA] [RPMLINT] [PATCH] missing-call-to-chdir-with-chroot is most likely triggered by libguile/posix.c, and for once seems to me to not be a false positive. Please check (I'm not that much of a C expert) and consider adding a patch that calls chdir() before chrooting, as this is a potential security issue. - [EXTRA] [PATCH] configure.ac contains AC_PROG_LIBTOOL, which is deprecated. Maybe add a patch to replace it with LT_INIT? No further changes should be necessary. See https://www.gnu.org/software/libtool/manual/html_node/LT_005fINIT.html - [EXTRA] [RPMLINT] [PATCH] Fix FSF address. - [NON-ISSUE] Further (informative, non-issue) comments inline below, enclosed in . = MUST items = C/C++: [X]: Package does not contain kernel modules. [X]: Package contains no static executables. [X]: Development (unversioned) .so files in -devel subpackage, if present. Shared libraries in libdir are guile-specific, not in LDPATH and not even normal libraries [x]: Provides: bundled(gnulib) in place as required. [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only
[Bug 1983601] Review Request: guile3 - A GNU implementation of Scheme for application extensibility
https://bugzilla.redhat.com/show_bug.cgi?id=1983601 Christopher Engelhard changed: What|Removed |Added CC||c...@lcts.de Assignee|nob...@fedoraproject.org|c...@lcts.de -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
