Message ID | f19329dc2f36a10f3fdc9985887cb2cf66dce094.1651594312.git.philip@philipmcgrath.com |
---|---|
State | Accepted |
Headers | show |
Series | gnu: Update Racket to 8.5 and Chez Scheme to 9.5.8. | expand |
Context | Check | Description |
---|---|---|
cbaines/comparison | success | View comparision |
cbaines/git branch | success | View Git branch |
cbaines/applying patch | success | View Laminar job |
cbaines/issue | success | View issue |
cbaines/comparison | success | View comparision |
cbaines/git branch | success | View Git branch |
cbaines/applying patch | success | View Laminar job |
cbaines/issue | success | View issue |
Philip McGrath schreef op di 03-05-2022 om 14:33 [-0400]: > - ;; help with debugging, but it confuses `install-license-files`. [...] > + ;; workaround for install-license-files > + (lambda* (#:key out-of-source? #:allow-other-keys) > + (when out-of-source? > + (with-directory-excursion ".." > + (symlink "src" > + (package-name->name+version > + (strip-store-file-name #$output)))))))))) Surely we could fix this bug/limitation of install-license-files upstream (in this case, upstream=Guix)? Greetings, Maxime.
Hi, On 5/4/22 05:29, Maxime Devos wrote: > Philip McGrath schreef op di 03-05-2022 om 14:33 [-0400]: >> - ;; help with debugging, but it confuses `install-license-files`. > [...] >> + ;; workaround for install-license-files >> + (lambda* (#:key out-of-source? #:allow-other-keys) >> + (when out-of-source? >> + (with-directory-excursion ".." >> + (symlink "src" >> + (package-name->name+version >> + (strip-store-file-name #$output)))))))))) > > Surely we could fix this bug/limitation of install-license-files > upstream (in this case, upstream=Guix)? > I'd certainly be in favor of that! I've never edited '(guix build gnu-build-system)' but I assume that would be a 'core-updates' change. I'm also not sure what the best solution would be in general. In Racket's specific case, the source directory for the Racket VM is (relative to the repository root), "racket/src/", where other notable directories present include "racket/collects/" and "pkgs/". An out-of-source build ends up happening in "racket/build/". The license files are in the source directory, which is what install-license-files expects, but the find-source-directory helper function fails to guess the location of the source directory: > (define (find-source-directory package) > ;; For an out-of-source build, guess the source directory location > ;; relative to the current directory. Return #f on failure. > (match (scandir ".." > (lambda (file) > (and (not (member file '("." ".." "build"))) > (file-is-directory? > (string-append "../" file))))) > (() ;hmm, no source > #f) > ((source) ;only one other file > (string-append "../" source)) > ((directories ...) ;pick the most likely one > ;; This happens for example with libstdc++, which lives within the GCC > ;; source tree. > (any (lambda (directory) > (and (string-prefix? package directory) > (string-append "../" directory))) > directories)))) Some possibilities I can imagine: * We could add a case to find-source-directory specifically for "src". * We could change configure to communicate the location of the source directory, e.g. via an environment variable, rather than trying to guess. * We could change install-license-files to do more searching in general, e.g. trying multiple directories or preferring a directory that contains at least one match for #:license-file-regexp. We'd have to consider the potential for false positives, too (e.g. sibling directories with projects under different licenses.) * We could add an argument to install-license-files to specify the directory explicitly, as a complement to #:license-file-regexp. Another scenario that might be worth considering is projects that follow the REUSE specification[1], which I don't think would be handled by the current install-license-files. But actually, for Racket, I forgot that a patch I'd sent upstream to handle this in `make install` should be in 8.5, so we may not need this patch at all: I'll confirm before I send v2. -Philip [1]: https://reuse.software/ [2]: https://github.com/racket/racket/pull/4127
Am Donnerstag, dem 05.05.2022 um 14:53 -0400 schrieb Philip McGrath: > Hi, > > On 5/4/22 05:29, Maxime Devos wrote: > > Philip McGrath schreef op di 03-05-2022 om 14:33 [-0400]: > > > - ;; help with debugging, but it confuses `install-license- > > > files`. > > [...] > > > + ;; workaround for install-license-files > > > + (lambda* (#:key out-of-source? #:allow-other-keys) > > > + (when out-of-source? > > > + (with-directory-excursion ".." > > > + (symlink "src" > > > + (package-name->name+version > > > + (strip-store-file-name > > > #$output)))))))))) > > > > Surely we could fix this bug/limitation of install-license-files > > upstream (in this case, upstream=Guix)? > > > > I'd certainly be in favor of that! I've never edited '(guix build > gnu-build-system)' but I assume that would be a 'core-updates' change. > It would. > I'm also not sure what the best solution would be in general. > > In Racket's specific case, the source directory for the Racket VM is > (relative to the repository root), "racket/src/", where other notable > directories present include "racket/collects/" and "pkgs/". An > out-of-source build ends up happening in "racket/build/". The license > files are in the source directory, which is what install-license-files > expects, but the find-source-directory helper function fails to guess > the location of the source directory: I think the issue here is that we're in a chdir into the source, where the typical assumption that there's only a single directory besides it fails. > > (define (find-source-directory package) > > ;; For an out-of-source build, guess the source directory > > location > > ;; relative to the current directory. Return #f on failure. > > (match (scandir ".." > > (lambda (file) > > (and (not (member file '("." ".." "build"))) > > (file-is-directory? > > (string-append "../" file))))) > > (() ;hmm, no source > > #f) > > ((source) ;only one other > > file > > (string-append "../" source)) > > ((directories ...) ;pick the most > > likely one > > ;; This happens for example with libstdc++, which lives > > within the GCC > > ;; source tree. > > (any (lambda (directory) > > (and (string-prefix? package directory) > > (string-append "../" directory))) > > directories)))) I don't think (lambda (directory) (and (string-prefix? package directory) (string-append "../" directory))) does what we think it does. Even so, it's debatable whether that works for Racket. > Some possibilities I can imagine: > > * We could add a case to find-source-directory specifically for > "src". Not a fan personally. > * We could change configure to communicate the location of the > source directory, e.g. via an environment variable, rather than > trying to guess. That would work, but gratuitous environment variables could have unexpected consequences. However, note that the build systems themselves already need to capture the information of where the source directory lives! For gnu-build-system, you could check for abs_srcdir in the Makefile for instance. If this ever becomes necessary (reading ahead it doesn't seem to be, but let that if be an if), I'd suggest splitting install-license-files into a helper procedure that assumes we're in the correct directory and a top level procedure, that performs the guess. This top level procedure would need to be implemented once per build system, while the helper could be inherited. > * We could change install-license-files to do more searching in > general, e.g. trying multiple directories or preferring a > directory that contains at least one match for > #:license-file-regexp. We'd have to consider the potential for > false positives, too (e.g. sibling directories with projects > under different licenses.) > * We could add an argument to install-license-files to specify the > directory explicitly, as a complement to #:license-file-regexp. I would not do either. An alternative to adding another phase is wrapping install-license-files in a directory excursion within this package. That'd be less code which achieves the same thing. > Another scenario that might be worth considering is projects that > follow the REUSE specification[1], which I don't think would be > handled by the current install-license-files. I don't think we can rely on projects consistently following them, sadly. > But actually, for Racket, I forgot that a patch I'd sent upstream to > handle this in `make install` should be in 8.5, so we may not need > this patch at all: I'll confirm before I send v2. That is of course the best solution :)
Philip McGrath schreef op do 05-05-2022 om 14:53 [-0400]: > Another scenario that might be worth considering is projects that follow > the REUSE specification[1], which I don't think would be handled by the > current install-license-files. It doesn't copy .reuse/dep5 and LICENSES yet, though that can be implemented: <https://issues.guix.gnu.org/54234#4>. I don't think that helps for this particular package though, given that Racket doesn't ship those files. Greetings, Maxime.
Liliana Marie Prikler schreef op do 05-05-2022 om 21:52 [+0200]: > > Another scenario that might be worth considering is projects that > > follow the REUSE specification[1], which I don't think would be > > handled by the current install-license-files. > I don't think we can rely on projects consistently following them, > sadly. There's plenty of projects that don't follow REUSE (e.g., Guix itself), and there might be some that have the .reuse/dep5 and LICENSES but aren't doing things 100% according to the spec, but I don't think it matters much to Guix if SPDX-License-Identifier is present in every file and such, given that all Guix has to do here is copy license files, so Guix could be taught to copy .reuse/dep5 and LICENSES (in addition to LICENSE, COPYING, ...). Greetings, Maxime
On 5/5/22 16:33, Maxime Devos wrote: > Philip McGrath schreef op do 05-05-2022 om 14:53 [-0400]: >> Another scenario that might be worth considering is projects that follow >> the REUSE specification[1], which I don't think would be handled by the >> current install-license-files. > > It doesn't copy .reuse/dep5 and LICENSES yet, though that can be > implemented: <https://issues.guix.gnu.org/54234#4>. I don't think that > helps for this particular package though, given that Racket doesn't > ship those files. > Right, it wouldn't help Racket, it was just another example of a case not handled by the current implementation. (Though I think it would be much more justifiable to add a special case for REUSE than for Racket.) Glad people are already thinking about it! -Philip
diff --git a/gnu/packages/racket.scm b/gnu/packages/racket.scm index adf3ccfd74..2f4f7cebd8 100644 --- a/gnu/packages/racket.scm +++ b/gnu/packages/racket.scm @@ -272,8 +272,8 @@ (define-public racket-vm-cgc ;; main-distribution-test that aren't part of the main ;; distribution. #:tests? #f - ;; Upstream recommends #:out-of-source?, and it does - ;; help with debugging, but it confuses `install-license-files`. + ;; Upstream recommends #:out-of-source?, and helps a lot with debugging + #:out-of-source? #t #:modules '((ice-9 match) (ice-9 regex) (guix build gnu-build-system) @@ -310,7 +310,15 @@ (define-public racket-vm-cgc #f))))))) (add-before 'configure 'chdir (lambda _ - (chdir "racket/src")))))) + (chdir "racket/src"))) + (add-after 'chdir 'symlink-src + ;; workaround for install-license-files + (lambda* (#:key out-of-source? #:allow-other-keys) + (when out-of-source? + (with-directory-excursion ".." + (symlink "src" + (package-name->name+version + (strip-store-file-name #$output)))))))))) (home-page "https://racket-lang.org") (synopsis "Old Racket implementation used for bootstrapping") (description "This variant of the Racket BC (``before Chez'' or