Message ID | 20210315225302.6597-1-philip@philipmcgrath.com |
---|---|
State | Accepted |
Headers | show |
Series | [bug#47153,v2,1/3] gnu: chez-scheme: Update nanopass to 1.9.2. | 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 |
Hi, Am Montag, den 15.03.2021, 18:53 -0400 schrieb Philip McGrath: > - (sha256 (base32 > "1synadgaycca39jfx525975ss9y0lkl516sdrc62wrrllamm8n21")) > + (sha256 > "16vjsik9rrzbabbhbxbaha51ppi3f9n8rk59pc6zdyffs0vziy4i") You're inadvertently stripping away base32. > - (sha256 (base32 > "1q5i8pf4cdfjsj6r2k1rih7ljbfggyxdng2p2fvsgarzihpsin2i")) > + (sha256 > "01jnvw8qw33gnpzwrakwhsr05h6b609lm180jnspcrb7lds2p23d") Same here. > - (commit (string-append "v" version)))) > + ;; This commit includes a fix for which we would > + ;; otherwise want to use a snippet. > + ;; When there's a new tagged release, > + ;; go back to using (string-append "v" version) > + (commit "54051494434a197772bf6ca5b4e6cf6be55f39a5"))) Could we then not cherry-pick this commit as a patch? Or is there more needed? > + ;; pre-built bootfiles for unsupported > systems: > + "boot/a6nt" > + "boot/a6osx" > + "boot/i3nt" > + "boot/i3osx" > + "boot/ta6nt" > + "boot/ta6osx" > + "boot/ti3nt" > + "boot/ti3osx"))))))) What about pre-built bootfiles for supported systems? Do we still need those? If we so, I don't think it is right to delete anything in boot; if not we should delete it altogether. > + ;; put these where configure expects them to be > + (add-after 'unpack 'unpack-nanopass+stex > + (lambda* (#:key native-inputs inputs #:allow-other-keys) > + (let ((patch-source-shebangs > (assoc-ref %standard-phases 'patch-source- > shebangs))) I don't think you need patch-source-shebangs directly after unpack. Those shebangs would be patched in their phase anyway – the only reason we needed it before was because we delayed unpacking until configure. > + (setenv "CC" "gcc") This should be determined via cc-for-target. > + (apply invoke > + "./configure" > + flags) This can very likely be one line. Also, it should probably be done via configure-flags. > [outputs]: Add "stex" I don't think an stex output is justified in and of itself, or is it needed for chez to function? (Even if it was, it would then need to be part of the "out" output.) I personally doubt this; if you need stex itself for your purposes, you can try packaging it later. > + (flags (if (assoc-ref inputs "ncurses") > + flags > + (cons "--disable-curses" > + flags))) > + (flags (if (assoc-ref inputs "libx11") > + flags > + (cons "--disable-x11" > + flags)))) These will probably be detected by the build system itself, there's no need for you to add them. If not, then it's up to the packager of other variants to specify them, not you. > + (flags (list > + (string-append "--installprefix=" out) > + (string-append "ZLIB=" zlib-static > "/lib/libz.a") > + (string-append "LZ4=" lz4-static > "/lib/liblz4.a") > + "--nogzip-man-pages" ;; guix will do it > + "--threads")) This should be done via #:configure-flags. Now that I think of it… > + (replace 'configure I don't think this is needed at all. At most, you should filter the incompatible flags from configure-flags in some way, but you might also want to patch configure, so that it swallows them. > Refactor 'install-doc' phase into 'build+install-stex' and > 'build+install-doc' 'build+install' implies, that it should actually be two phases, build and install. I already talked about install-stex, so you should only refactor install-doc insofar as it is needed to accommodate changes in the build system. Regards, Leo
Hi, On 3/16/21 3:37 AM, Leo Prikler wrote: > Am Montag, den 15.03.2021, 18:53 -0400 schrieb Philip McGrath: >> - (sha256 (base32 >> "1synadgaycca39jfx525975ss9y0lkl516sdrc62wrrllamm8n21")) >> + (sha256 >> "16vjsik9rrzbabbhbxbaha51ppi3f9n8rk59pc6zdyffs0vziy4i") > You're inadvertently stripping away base32. I thought I'd read that explicitly calling base32 was redundant and no longer recommended: is there a reason to keep it? >> - (commit (string-append "v" version)))) >> + ;; This commit includes a fix for which we would >> + ;; otherwise want to use a snippet. >> + ;; When there's a new tagged release, >> + ;; go back to using (string-append "v" version) >> + (commit "54051494434a197772bf6ca5b4e6cf6be55f39a5"))) > Could we then not cherry-pick this commit as a patch? Or is there more > needed? We could, but the upstream history is simply v1.2.2 -> my patch -> Kent Dybvig's merge commit accepting it. I thought doing it this way clarified that it's not a Guix-specific patch that should stay around indefinitely. Is there a reason to prefer cherry-picking it as a patch? > >> + ;; pre-built bootfiles for unsupported >> systems: >> + "boot/a6nt" >> + "boot/a6osx" >> + "boot/i3nt" >> + "boot/i3osx" >> + "boot/ta6nt" >> + "boot/ta6osx" >> + "boot/ti3nt" >> + "boot/ti3osx"))))))) > What about pre-built bootfiles for supported systems? Do we still need > those? If we so, I don't think it is right to delete anything in boot; > if not we should delete it altogether. Currently, you need a bootfile for your current system to bootstrap the Chez compiler; once you have a running Chez, you can build bootfiles for any system Chez supports (which is more systems than it includes in its git repository for bootstrapping). The bootfiles you build will be byte-for-byte identical to pre-built ones, if pre-built ones were provided: Chez in fact builds them twice and errors if they aren't. So, I'm not sure why we would want these files, which are over 20 MB and are only useful for bootstrapping Chez on Windows or Mac OS. But if there is a reason to want them, we can keep them! >> + ;; put these where configure expects them to be >> + (add-after 'unpack 'unpack-nanopass+stex >> + (lambda* (#:key native-inputs inputs #:allow-other-keys) >> + (let ((patch-source-shebangs >> (assoc-ref %standard-phases 'patch-source- >> shebangs))) > I don't think you need patch-source-shebangs directly after unpack. > Those shebangs would be patched in their phase anyway – the only reason > we needed it before was because we delayed unpacking until configure. Right, thanks for catching this! >> + (setenv "CC" "gcc") > This should be determined via cc-for-target. Will do. >> + (apply invoke >> + "./configure" >> + flags) > This can very likely be one line. Also, it should probably be done via > configure-flags. > >> [outputs]: Add "stex" > I don't think an stex output is justified in and of itself, or is it Oops, I'd actually removed the "stex" output, but I missed it in the commit message. >> + (flags (if (assoc-ref inputs "ncurses") >> + flags >> + (cons "--disable-curses" >> + flags))) >> + (flags (if (assoc-ref inputs "libx11") >> + flags >> + (cons "--disable-x11" >> + flags)))) > These will probably be detected by the build system itself, there's no > need for you to add them. If not, then it's up to the packager of > other variants to specify them, not you. IIRC, Chez's build system errors without these flags if the library isn't found. Also, I intend to be "the packager of the other variants"—my primary motivation for sending these patches is to reuse as much as possible for Racket's fork of Chez, rather than all of the messy copy-pasting we're doing now. But see below … >> + (flags (list >> + (string-append "--installprefix=" out) >> + (string-append "ZLIB=" zlib-static >> "/lib/libz.a") >> + (string-append "LZ4=" lz4-static >> "/lib/liblz4.a") >> + "--nogzip-man-pages" ;; guix will do it >> + "--threads")) > This should be done via #:configure-flags. > > Now that I think of it… > >> + (replace 'configure > I don't think this is needed at all. At most, you should filter the > incompatible flags from configure-flags in some way, but you might also > want to patch configure, so that it swallows them. I'm not enthusiastic about the idea of maintaining such a patch for two variants of this custom configure script which accept different sets of flags, but neither of which accepts *any* of the flags that would be added by the 'configure phase from %standard-phases. That procedure offers no way to interpose between its adding the flags and its invoking the configure script, so I don't see a good alternative to replacing it. In fact, I'd modeled my implementation on the one from %standard-phases, which similarly has a big `let*` expression adding implicit phases based on the inputs and outputs. If you think it's important to support #:configure-flags, I could change this implementation to accept them, in which case I'd be ok with leaving out handling of --disable-curses and --disable-x11. But I think anyone packaging a variant of Chez will need to be aware of its idiosyncratic configure script. >> Refactor 'install-doc' phase into 'build+install-stex' and >> 'build+install-doc' > 'build+install' implies, that it should actually be two phases, build > and install. I already talked about install-stex, so you should only > refactor install-doc insofar as it is needed to accommodate changes in > the build system. It could be two phases, but the current install-doc phase does, in fact, also build the docs, in addition to installing them. (It actually neglects to install the HTML release notes but installs two copies of the PDF user's guide.) I'm ok with calling it install-doc if you prefer. I guess I could even split it into two phases, but that seems like it would just make things more complicated for no obvious benefit. As I mentioned, build+install-stex actually just "installs" stex to a temporary directory right now. I think I could probably skip it and rely on Chez's on-the-fly compilation, but I didn't see a problem with doing it this way. -Philip
Hi, Am Dienstag, den 16.03.2021, 16:19 -0400 schrieb Philip McGrath: > Hi, > > On 3/16/21 3:37 AM, Leo Prikler wrote: > > Am Montag, den 15.03.2021, 18:53 -0400 schrieb Philip McGrath: > > > - (sha256 (base32 > > > "1synadgaycca39jfx525975ss9y0lkl516sdrc62wrrllamm8n21")) > > > + (sha256 > > > "16vjsik9rrzbabbhbxbaha51ppi3f9n8rk59pc6zdyffs0vziy4i") > > You're inadvertently stripping away base32. > > I thought I'd read that explicitly calling base32 was redundant and > no > longer recommended: is there a reason to keep it? Without a reference to where you've read that, it'll be hard to verify or falsify that claim. Both master and core-updates regularly see sha256/base32 is fancy syntax around hash-digest and hash-algo as far as I understand, so I doubt you recall this correctly. > > > - (commit (string-append "v" version)))) > > > + ;; This commit includes a fix for which we would > > > + ;; otherwise want to use a snippet. > > > + ;; When there's a new tagged release, > > > + ;; go back to using (string-append "v" version) > > > + (commit > > > "54051494434a197772bf6ca5b4e6cf6be55f39a5"))) > > Could we then not cherry-pick this commit as a patch? Or is there > > more > > needed? > > We could, but the upstream history is simply v1.2.2 -> my patch -> > Kent > Dybvig's merge commit accepting it. I thought doing it this way > clarified that it's not a Guix-specific patch that should stay > around > indefinitely. Is there a reason to prefer cherry-picking it as a > patch? You'll probably hear differing opinions about that, and that's fine, but my personal reason to prefer cherry-picking would be, that it makes it very obvious, what changed from the base – that being the patch modulo offset changes – and doesn't invite people to go out saying "aha, but I found this commit and I like that more, let's take it". In other words, this is very subjective, but I believe we should stick as close to releases as is reasonable. > > > + ;; pre-built bootfiles for unsupported > > > systems: > > > + "boot/a6nt" > > > + "boot/a6osx" > > > + "boot/i3nt" > > > + "boot/i3osx" > > > + "boot/ta6nt" > > > + "boot/ta6osx" > > > + "boot/ti3nt" > > > + "boot/ti3osx"))))))) > > What about pre-built bootfiles for supported systems? Do we still > > need > > those? If we so, I don't think it is right to delete anything in > > boot; > > if not we should delete it altogether. > > Currently, you need a bootfile for your current system to bootstrap > the > Chez compiler; once you have a running Chez, you can build bootfiles > for > any system Chez supports (which is more systems than it includes in > its > git repository for bootstrapping). The bootfiles you build will be > byte-for-byte identical to pre-built ones, if pre-built ones were > provided: Chez in fact builds them twice and errors if they aren't. > So, > I'm not sure why we would want these files, which are over 20 MB and > are > only useful for bootstrapping Chez on Windows or Mac OS. But if there > is > a reason to want them, we can keep them! I don't think we can necessarily trust the boot files in this configuration. "They are bit-for-bit identical" can also mean, that the login() hack was successfully applied for all you know. But trusting trust aside, I don't think this is the right way of "miraculously slashing" half of chez' bootstrap. If you do want to follow the direction of making the bootstrap explicit, how about a chez-boot minipackage, that only keeps the relevant boot files for the target system? (This would of course need to be done in a separate patch, which you can attach to this series, however.) > > > + (apply invoke > > > + "./configure" > > > + flags) > > This can very likely be one line. Also, it should probably be done > > via > > configure-flags. > > > > > [outputs]: Add "stex" > > I don't think an stex output is justified in and of itself, or is > > it > > Oops, I'd actually removed the "stex" output, but I missed it in the > commit message. I do think I saw it in the actual commit as well, but I might be mistaken about that. > > > + (flags (if (assoc-ref inputs "ncurses") > > > + flags > > > + (cons "--disable-curses" > > > + flags))) > > > + (flags (if (assoc-ref inputs "libx11") > > > + flags > > > + (cons "--disable-x11" > > > + flags)))) > > These will probably be detected by the build system itself, there's > > no > > need for you to add them. If not, then it's up to the packager of > > other variants to specify them, not you. > > IIRC, Chez's build system errors without these flags if the library > isn't found. Also, I intend to be "the packager of the other > variants"—my primary motivation for sending these patches is to reuse > as > much as possible for Racket's fork of Chez, rather than all of the > messy > copy-pasting we're doing now. But see below … Fair enough, but either way you should just cons them onto #:configure- flags where applicable. > > > + (flags (list > > > + (string-append "--installprefix=" > > > out) > > > + (string-append "ZLIB=" zlib-static > > > "/lib/libz.a") > > > + (string-append "LZ4=" lz4-static > > > "/lib/liblz4.a") > > > + "--nogzip-man-pages" ;; guix will do > > > it > > > + "--threads")) > > This should be done via #:configure-flags. > > > > Now that I think of it… > > > > > + (replace 'configure > > I don't think this is needed at all. At most, you should filter > > the > > incompatible flags from configure-flags in some way, but you might > > also > > want to patch configure, so that it swallows them. > > I'm not enthusiastic about the idea of maintaining such a patch for > two > variants of this custom configure script which accept different sets > of > flags, but neither of which accepts *any* of the flags that would be > added by the 'configure phase from %standard-phases. That procedure > offers no way to interpose between its adding the flags and its > invoking > the configure script, so I don't see a good alternative to replacing > it. > In fact, I'd modeled my implementation on the one from %standard- > phases, > which similarly has a big `let*` expression adding implicit phases > based > on the inputs and outputs. > > If you think it's important to support #:configure-flags, I could > change > this implementation to accept them, in which case I'd be ok with > leaving > out handling of --disable-curses and --disable-x11. But I think > anyone > packaging a variant of Chez will need to be aware of its > idiosyncratic > configure script. Perhaps I was a little too harsh. If you need to replace 'configure to not let gnu-build-system's own flags pass into this idiosyncratic script, you are of course allowed to do that. I just think storing configure flags inside the argument that's named like that is a better choice, than an ad-hoc construction. Whatever you see inside that standard-phase is done precisely because storing it as an argument would not be an option. > > > Refactor 'install-doc' phase into 'build+install-stex' and > > > 'build+install-doc' > > 'build+install' implies, that it should actually be two phases, > > build > > and install. I already talked about install-stex, so you should > > only > > refactor install-doc insofar as it is needed to accommodate changes > > in > > the build system. > > It could be two phases, but the current install-doc phase does, in > fact, > also build the docs, in addition to installing them. (It actually > neglects to install the HTML release notes but installs two copies > of > the PDF user's guide.) I'm ok with calling it install-doc if you > prefer. > I guess I could even split it into two phases, but that seems like > it > would just make things more complicated for no obvious benefit. That is irrelevant, `make install' is not prohibited from building stuff. > As I mentioned, build+install-stex actually just "installs" stex to > a > temporary directory right now. I think I could probably skip it and > rely > on Chez's on-the-fly compilation, but I didn't see a problem with > doing > it this way. Sounds like it might work. Regards, Leo
Hi, On 3/16/21 5:09 PM, Leo Prikler wrote: > Am Dienstag, den 16.03.2021, 16:19 -0400 schrieb Philip McGrath: >> On 3/16/21 3:37 AM, Leo Prikler wrote: >>> Am Montag, den 15.03.2021, 18:53 -0400 schrieb Philip McGrath: >>>> - (sha256 (base32 >>>> "1synadgaycca39jfx525975ss9y0lkl516sdrc62wrrllamm8n21")) >>>> + (sha256 >>>> "16vjsik9rrzbabbhbxbaha51ppi3f9n8rk59pc6zdyffs0vziy4i") >>> You're inadvertently stripping away base32. >> >> I thought I'd read that explicitly calling base32 was redundant and I think I'd conflated multiple things in my memory. I see that the expansion of `package` applies the same special handling of the `sha256` field for a literal string as one wrapped with `base32` (recognized with `free-identifier=?`), including checking and conversion to a bytevector at compile time. >>>> + ;; When there's a new tagged release, >>>> + ;; go back to using (string-append "v" version) >>>> + (commit >>>> "54051494434a197772bf6ca5b4e6cf6be55f39a5"))) >>> Could we then not cherry-pick this commit as a patch? Or is there >>> more >>> needed? >> >> We could, but the upstream history is simply v1.2.2 -> my patch -> >> Kent >> Dybvig's merge commit accepting it. I thought doing it this way >> clarified that it's not a Guix-specific patch that should stay >> around >> indefinitely. Is there a reason to prefer cherry-picking it as a >> patch? > You'll probably hear differing opinions about that, and that's fine, > but my personal reason to prefer cherry-picking would be, that it makes > it very obvious, what changed from the base – that being the patch > modulo offset changes – and doesn't invite people to go out saying > "aha, but I found this commit and I like that more, let's take it". In > other words, this is very subjective, but I believe we should stick as > close to releases as is reasonable. I can understand that perspective. From my point of view, I'm inclined to see the release plus one upstream commit (the first commit since 2017) as closer to the release than a free-floating patch file: with a patch, I can see what has changed, but I need to evaluate why it was changed, if the change is still necessary, and so forth, rather than relying on the upstream maintainers' judgement. > I don't think we can necessarily trust the boot files in this > configuration. "They are bit-for-bit identical" can also mean, that > the login() hack was successfully applied for all you know. Yes, the "trusting trust" issues are especially striking if you consider that Chez Scheme was non-free software from 1984 to 2016, and the first libre release likewise needed the bootfiles of its ancestors. My point here was just to save space, from the perspective that these are build artifacts which can be easily recreated by anyone on a GNU/Linux system. But I don't think it's especially important, so I'm keeping them. I hope it might turn out to be possible eventually to bootstrap via Racket, maybe even by just picking an older version of the bootstrap code before the Racket fork's #!base-rtd gained a vector of ancestors rather than a parent and a few such things, but I think there's more to be done around Racket packaging before investigating that. -Philip
diff --git a/gnu/packages/chez.scm b/gnu/packages/chez.scm index eac556c4d0..5dd1185c43 100644 --- a/gnu/packages/chez.scm +++ b/gnu/packages/chez.scm @@ -4,6 +4,7 @@ ;;; Copyright © 2017, 2019 Tobias Geerinckx-Rice <me@tobias.gr> ;;; Copyright © 2019 Brett Gilio <brettg@gnu.org> ;;; Copyright © 2020 Brendan Tildesley <mail@brendan.scot> +;;; Copyright © 2021 Philip McGrath <philip@philipmcgrath.com> ;;; ;;; This file is part of GNU Guix. ;;; @@ -43,13 +44,13 @@ #:use-module (srfi srfi-1)) (define nanopass - (let ((version "1.9.1")) + (let ((version "1.9.2")) (origin (method git-fetch) (uri (git-reference (url "https://github.com/nanopass/nanopass-framework-scheme") (commit (string-append "v" version)))) - (sha256 (base32 "1synadgaycca39jfx525975ss9y0lkl516sdrc62wrrllamm8n21")) + (sha256 "16vjsik9rrzbabbhbxbaha51ppi3f9n8rk59pc6zdyffs0vziy4i") (file-name (git-file-name "nanopass" version))))) (define stex