Message ID | cff09d1a4d4201f5568bccfaad96fb624a1f4165.1617711307.git.efraim@flashner.co.il |
---|---|
State | Accepted |
Headers | show |
Series | Add 32-bit powerpc support | expand |
Context | Check | Description |
---|---|---|
cbaines/comparison | success | View comparision |
cbaines/git branch | success | View Git branch |
cbaines/applying patch | fail | View Laminar job |
cbaines/issue | success | View issue |
Efraim Flashner <efraim@flashner.co.il> writes: > * gnu/packages/base.scm (binutils)[arguments]: Add phase on > powerpc-linux to adjust the test suite. > * gnu/packages/commencement.scm (binutils-boot0)[arguments]: Move custom > phases after inherited arguments. Add phase on powerpc-linux to adjust > the test suite. Nits: adjust the test suite to do what? The message would be clearer if it explained the purpose of the adjustment. You could also name the phases you added/moved, for extra clarity, if you want to. > diff --git a/gnu/packages/base.scm b/gnu/packages/base.scm > index dbb7c619fe..b9fc0a6e29 100644 > --- a/gnu/packages/base.scm > +++ b/gnu/packages/base.scm > @@ -531,7 +531,16 @@ change. GNU make offers many powerful extensions over the standard utility.") > > ;; Make sure 'ar' and 'ranlib' produce archives in a > ;; deterministic fashion. > - "--enable-deterministic-archives"))) > + "--enable-deterministic-archives") > + ,@(if (string=? (%current-system) "powerpc-linux") > + `(#:phases > + (modify-phases %standard-phases > + (add-after 'unpack 'disable-rust-libiberty-test > + (lambda _ > + (substitute* "libiberty/testsuite/Makefile.in" > + ((" check-rust-demangle ") "")) > + #t)))) > + '()))) What's the problem? Presumably the test fails; a comment here could clarify that. If it's a test failure, has the issue been reported upstream? > (synopsis "Binary utilities: bfd gas gprof ld") > (description > diff --git a/gnu/packages/commencement.scm b/gnu/packages/commencement.scm > index 7c39a84008..f707a01d30 100644 > --- a/gnu/packages/commencement.scm > +++ b/gnu/packages/commencement.scm > @@ -2653,7 +2653,22 @@ exec " gcc "/bin/" program > #:modules ((guix build gnu-build-system) > (guix build utils) > (ice-9 ftw)) ; for 'scandir' > + > + ;; #:phases gets modified for powerpc-linux in binutils, > + ;; so #:phases here needs to be after the inherited one. > + ,@(substitute-keyword-arguments (package-arguments binutils) > + ((#:configure-flags cf) > + `(cons ,(string-append "--target=" (boot-triplet)) > + ,cf))) > + > #:phases (modify-phases %standard-phases > + ,@(if (string=? (%current-system) "powerpc-linux") > + '((add-after 'unpack 'disable-rust-libiberty-test > + (lambda _ > + (substitute* "libiberty/testsuite/Makefile.in" > + ((" check-rust-demangle ") "")) > + #t))) > + '()) > (add-after 'install 'add-symlinks > (lambda* (#:key outputs #:allow-other-keys) > ;; The cross-gcc invokes 'as', 'ld', etc, without the > @@ -2667,12 +2682,8 @@ exec " gcc "/bin/" program > (with-directory-excursion (string-append out "/bin") > (for-each (lambda (name) > (symlink name (remove-triplet-prefix name))) > - (scandir "." has-triplet-prefix?))))))) > + (scandir "." has-triplet-prefix?))))))))) > > - ,@(substitute-keyword-arguments (package-arguments binutils) > - ((#:configure-flags cf) > - `(cons ,(string-append "--target=" (boot-triplet)) > - ,cf))))) > (inputs (%boot0-inputs)))) > > (define libstdc++-boot0 I think you can put all of this in the substitute-keyword-arguments form, which would (1) eliminate the need for a comment, (2) eliminate the need to worry about the order of the keyword arguments, and (3) eliminate the need to duplicate the new phase in two package definitions: (define binutils-boot0 (package (inherit binutils) (source (bootstrap-origin (package-source binutils))) (name "binutils-cross-boot0") (arguments `(#:guile ,%bootstrap-guile #:implicit-inputs? #f #:modules ((guix build gnu-build-system) (guix build utils) (ice-9 ftw)) ; for 'scandir' ,@(substitute-keyword-arguments (package-arguments binutils) ((#:configure-flags cf) `(cons ,(string-append "--target=" (boot-triplet)) ,cf)) ;; The presence of '%standard-phases as the default value here is ;; important. It ensures that even when (package-argument ;; binutils) does not already contain the #:phases keyword ;; argument, the substitution will occur. If you omit a default ;; value and (package-arguments binutils) does not contain the ;; #:phases keyword argument (e.g., on an x86_64-linux system), ;; then the substitution will not occur, and no phases at all will ;; be added. ((#:phases phases '%standard-phases) `(modify-phases ,phases ,@(if (string=? (%current-system) "powerpc-linux") '((add-after 'unpack 'disable-rust-libiberty-test (lambda _ (substitute* "libiberty/testsuite/Makefile.in" ((" check-rust-demangle ") "")) #t))) '()) (add-after 'install 'add-symlinks (lambda* (#:key outputs #:allow-other-keys) ;; The cross-gcc invokes 'as', 'ld', etc, without the ;; triplet prefix, so add symlinks. (let ((out (assoc-ref outputs "out")) (triplet-prefix (string-append ,(boot-triplet) "-"))) (define (has-triplet-prefix? name) (string-prefix? triplet-prefix name)) (define (remove-triplet-prefix name) (substring name (string-length triplet-prefix))) (with-directory-excursion (string-append out "/bin") (for-each (lambda (name) (symlink name (remove-triplet-prefix name))) (scandir "." has-triplet-prefix?))))))))))) (inputs (%boot0-inputs)))) I played with this in the REPL, and it seems to produce the desired result (e.g., inspect by running ,pp (package-arguments binutils-boot0) or similar at the REPL). However, I didn't actually try building anything with it. What do you think?
Chris Marusich <cmmarusich@gmail.com> writes: > (define binutils-boot0 > (package > (inherit binutils) > (source (bootstrap-origin (package-source binutils))) > (name "binutils-cross-boot0") > (arguments > `(#:guile ,%bootstrap-guile > #:implicit-inputs? #f > > #:modules ((guix build gnu-build-system) > (guix build utils) > (ice-9 ftw)) ; for 'scandir' > > ,@(substitute-keyword-arguments (package-arguments binutils) > ((#:configure-flags cf) > `(cons ,(string-append "--target=" (boot-triplet)) > ,cf)) > ;; The presence of '%standard-phases as the default value here is > ;; important. It ensures that even when (package-argument > ;; binutils) does not already contain the #:phases keyword > ;; argument, the substitution will occur. If you omit a default > ;; value and (package-arguments binutils) does not contain the > ;; #:phases keyword argument (e.g., on an x86_64-linux system), > ;; then the substitution will not occur, and no phases at all will > ;; be added. > ((#:phases phases '%standard-phases) > `(modify-phases ,phases > ,@(if (string=? (%current-system) "powerpc-linux") > '((add-after 'unpack 'disable-rust-libiberty-test > (lambda _ > (substitute* "libiberty/testsuite/Makefile.in" > ((" check-rust-demangle ") "")) > #t))) > '()) > (add-after 'install 'add-symlinks > (lambda* (#:key outputs #:allow-other-keys) > ;; The cross-gcc invokes 'as', 'ld', etc, without the > ;; triplet prefix, so add symlinks. > (let ((out (assoc-ref outputs "out")) > (triplet-prefix (string-append ,(boot-triplet) "-"))) > (define (has-triplet-prefix? name) > (string-prefix? triplet-prefix name)) > (define (remove-triplet-prefix name) > (substring name (string-length triplet-prefix))) > (with-directory-excursion (string-append out "/bin") > (for-each > (lambda (name) > (symlink name (remove-triplet-prefix name))) > (scandir "." has-triplet-prefix?))))))))))) > > (inputs (%boot0-inputs)))) Sorry, I meant to write this instead: (define binutils-boot0 (package (inherit binutils) (source (bootstrap-origin (package-source binutils))) (name "binutils-cross-boot0") (arguments `(#:guile ,%bootstrap-guile #:implicit-inputs? #f #:modules ((guix build gnu-build-system) (guix build utils) (ice-9 ftw)) ; for 'scandir' ,@(substitute-keyword-arguments (package-arguments binutils) ((#:configure-flags cf) `(cons ,(string-append "--target=" (boot-triplet)) ,cf)) ;; The presence of '%standard-phases as the default value here is ;; important. It ensures that even when (package-argument ;; binutils) does not already contain the #:phases keyword ;; argument, the substitution will occur. If you omit a default ;; value and (package-arguments binutils) does not contain the ;; #:phases keyword argument (e.g., on an x86_64-linux system), ;; then the substitution will not occur, and no phases at all will ;; be added. ((#:phases phases '%standard-phases) `(modify-phases ,phases (add-after 'install 'add-symlinks (lambda* (#:key outputs #:allow-other-keys) ;; The cross-gcc invokes 'as', 'ld', etc, without the ;; triplet prefix, so add symlinks. (let ((out (assoc-ref outputs "out")) (triplet-prefix (string-append ,(boot-triplet) "-"))) (define (has-triplet-prefix? name) (string-prefix? triplet-prefix name)) (define (remove-triplet-prefix name) (substring name (string-length triplet-prefix))) (with-directory-excursion (string-append out "/bin") (for-each (lambda (name) (symlink name (remove-triplet-prefix name))) (scandir "." has-triplet-prefix?))))))))))) (inputs (%boot0-inputs)))) The point is that we can probably "inherit" the phases, so we wouldn't have to repeat ourselves.
On Sat, Apr 17, 2021 at 12:51:01PM -0700, Chris Marusich wrote: > Efraim Flashner <efraim@flashner.co.il> writes: > > > * gnu/packages/base.scm (binutils)[arguments]: Add phase on > > powerpc-linux to adjust the test suite. > > * gnu/packages/commencement.scm (binutils-boot0)[arguments]: Move custom > > phases after inherited arguments. Add phase on powerpc-linux to adjust > > the test suite. > > Nits: adjust the test suite to do what? The message would be clearer if > it explained the purpose of the adjustment. You could also name the > phases you added/moved, for extra clarity, if you want to. > > > diff --git a/gnu/packages/base.scm b/gnu/packages/base.scm > > index dbb7c619fe..b9fc0a6e29 100644 > > --- a/gnu/packages/base.scm > > +++ b/gnu/packages/base.scm > > @@ -531,7 +531,16 @@ change. GNU make offers many powerful extensions over the standard utility.") > > > > ;; Make sure 'ar' and 'ranlib' produce archives in a > > ;; deterministic fashion. > > - "--enable-deterministic-archives"))) > > + "--enable-deterministic-archives") > > + ,@(if (string=? (%current-system) "powerpc-linux") > > + `(#:phases > > + (modify-phases %standard-phases > > + (add-after 'unpack 'disable-rust-libiberty-test > > + (lambda _ > > + (substitute* "libiberty/testsuite/Makefile.in" > > + ((" check-rust-demangle ") "")) > > + #t)))) > > + '()))) > > What's the problem? Presumably the test fails; a comment here could > clarify that. > > If it's a test failure, has the issue been reported upstream? I'll check to see if I can find something upstream. If not I'll report it. FWIW Debian disables the test suite for powerpc. https://sources.debian.org/src/binutils/2.36.1-6/debian/rules/#L537 > > (synopsis "Binary utilities: bfd gas gprof ld") > > (description > > diff --git a/gnu/packages/commencement.scm b/gnu/packages/commencement.scm > > index 7c39a84008..f707a01d30 100644 > > --- a/gnu/packages/commencement.scm > > +++ b/gnu/packages/commencement.scm > > @@ -2653,7 +2653,22 @@ exec " gcc "/bin/" program > > #:modules ((guix build gnu-build-system) > > (guix build utils) > > (ice-9 ftw)) ; for 'scandir' > > + > > + ;; #:phases gets modified for powerpc-linux in binutils, > > + ;; so #:phases here needs to be after the inherited one. > > + ,@(substitute-keyword-arguments (package-arguments binutils) > > + ((#:configure-flags cf) > > + `(cons ,(string-append "--target=" (boot-triplet)) > > + ,cf))) > > + > > #:phases (modify-phases %standard-phases > > + ,@(if (string=? (%current-system) "powerpc-linux") > > + '((add-after 'unpack 'disable-rust-libiberty-test > > + (lambda _ > > + (substitute* "libiberty/testsuite/Makefile.in" > > + ((" check-rust-demangle ") "")) > > + #t))) > > + '()) > > (add-after 'install 'add-symlinks > > (lambda* (#:key outputs #:allow-other-keys) > > ;; The cross-gcc invokes 'as', 'ld', etc, without the > > @@ -2667,12 +2682,8 @@ exec " gcc "/bin/" program > > (with-directory-excursion (string-append out "/bin") > > (for-each (lambda (name) > > (symlink name (remove-triplet-prefix name))) > > - (scandir "." has-triplet-prefix?))))))) > > + (scandir "." has-triplet-prefix?))))))))) > > > > - ,@(substitute-keyword-arguments (package-arguments binutils) > > - ((#:configure-flags cf) > > - `(cons ,(string-append "--target=" (boot-triplet)) > > - ,cf))))) > > (inputs (%boot0-inputs)))) > > > > (define libstdc++-boot0 > > I think you can put all of this in the substitute-keyword-arguments > form, which would (1) eliminate the need for a comment, (2) eliminate > the need to worry about the order of the keyword arguments, and (3) > eliminate the need to duplicate the new phase in two package > definitions: > > (define binutils-boot0 > (package > (inherit binutils) > (source (bootstrap-origin (package-source binutils))) > (name "binutils-cross-boot0") > (arguments > `(#:guile ,%bootstrap-guile > #:implicit-inputs? #f > > #:modules ((guix build gnu-build-system) > (guix build utils) > (ice-9 ftw)) ; for 'scandir' > > ,@(substitute-keyword-arguments (package-arguments binutils) > ((#:configure-flags cf) > `(cons ,(string-append "--target=" (boot-triplet)) > ,cf)) > ;; The presence of '%standard-phases as the default value here is > ;; important. It ensures that even when (package-argument > ;; binutils) does not already contain the #:phases keyword > ;; argument, the substitution will occur. If you omit a default > ;; value and (package-arguments binutils) does not contain the > ;; #:phases keyword argument (e.g., on an x86_64-linux system), > ;; then the substitution will not occur, and no phases at all will > ;; be added. > ((#:phases phases '%standard-phases) > `(modify-phases ,phases > ,@(if (string=? (%current-system) "powerpc-linux") > '((add-after 'unpack 'disable-rust-libiberty-test > (lambda _ > (substitute* "libiberty/testsuite/Makefile.in" > ((" check-rust-demangle ") "")) > #t))) > '()) > (add-after 'install 'add-symlinks > (lambda* (#:key outputs #:allow-other-keys) > ;; The cross-gcc invokes 'as', 'ld', etc, without the > ;; triplet prefix, so add symlinks. > (let ((out (assoc-ref outputs "out")) > (triplet-prefix (string-append ,(boot-triplet) "-"))) > (define (has-triplet-prefix? name) > (string-prefix? triplet-prefix name)) > (define (remove-triplet-prefix name) > (substring name (string-length triplet-prefix))) > (with-directory-excursion (string-append out "/bin") > (for-each > (lambda (name) > (symlink name (remove-triplet-prefix name))) > (scandir "." has-triplet-prefix?))))))))))) > > (inputs (%boot0-inputs)))) > > I played with this in the REPL, and it seems to produce the desired > result (e.g., inspect by running ,pp (package-arguments binutils-boot0) > or similar at the REPL). However, I didn't actually try building > anything with it. > > What do you think? > I like the way you've done it (and the comment). The fewer places with copy/pasted code the better. I'm building it again now to see about the tests. With your changes I only need to add the phase in (gnu packages base).
Efraim Flashner <efraim@flashner.co.il> writes: >> If it's a test failure, has the issue been reported upstream? > > I'll check to see if I can find something upstream. If not I'll report > it. FWIW Debian disables the test suite for powerpc. > https://sources.debian.org/src/binutils/2.36.1-6/debian/rules/#L537 How significant is the failure, anyway? Are we likely to encounter trouble down the line on powerpc if we don't resolve this? I have no idea, honestly, so I'm asking because I just don't know. I looked at the rules file you linked. Are you sure it disables the test for powerpc? It reads: with_check := yes ifneq (,$(findstring nocheck,$(DEB_BUILD_OPTIONS))) # override buildd admins to run the testsuite anyway ... ifeq (,$(filter $(DEB_HOST_ARCH), m68k powerpc sh4 sparc64)) with_check := disabled through DEB_BUILD_OPTIONS endif endif #with_check := disabled for this upload I'm not sure what the values for DEB_HOST_ARCH or DEB_BUILD_OPTIONS would be here. However, it seems to be setting with_check to disabled if and only if (1) nocheck shows up in DEB_BUILD_OPTIONS and (2) DEB_HOST_ARCH is not one of m68k, powerpc, sh4, or sparc64. In other words, it seems to enable the tests unless nocheck is in DEB_BUILD_OPTIONS, in which case it STILL enables the tests provided that DEB_HOST_ARCH is one of m68k, powerpc, sh4, or sparc64. I'm not very familiar with Debian rules files, so perhaps I'm mistaken. However, I think this means that when DEB_BUILD_OPTIONS doesn't contain "nocheck" (presumably it doesn't usually?), the tests will be run for every platform. In any case, reporting this upstream seems like the right thing to do. > I like the way you've done it (and the comment). The fewer places with > copy/pasted code the better. I'm building it again now to see about the > tests. With your changes I only need to add the phase in (gnu packages > base). I think the comment is honestly more than necessary, but if you find it helpful, perhaps someone in the future might, too. It describes a possibly surprising behavior of substitute-keyword-arguments that can happen any time you invoke it. I just wanted to call it out specifically for the purpose of this patch review.
On Wed, Apr 21, 2021 at 10:11:48PM -0700, Chris Marusich wrote: > Efraim Flashner <efraim@flashner.co.il> writes: > > >> If it's a test failure, has the issue been reported upstream? > > > > I'll check to see if I can find something upstream. If not I'll report > > it. FWIW Debian disables the test suite for powerpc. > > https://sources.debian.org/src/binutils/2.36.1-6/debian/rules/#L537 > > How significant is the failure, anyway? Are we likely to encounter > trouble down the line on powerpc if we don't resolve this? I have no > idea, honestly, so I'm asking because I just don't know. I submitted the bug upstream. It turns out it was a bug in the test suite which only showed up on big-endian systems. They've fixed it in libiberty (in GCC) and I added the patch to binutils on core-updates. Tests pass on powerpc-linux and x86_64-linux. It also made this commit much much smaller :)
diff --git a/gnu/packages/base.scm b/gnu/packages/base.scm index dbb7c619fe..b9fc0a6e29 100644 --- a/gnu/packages/base.scm +++ b/gnu/packages/base.scm @@ -531,7 +531,16 @@ change. GNU make offers many powerful extensions over the standard utility.") ;; Make sure 'ar' and 'ranlib' produce archives in a ;; deterministic fashion. - "--enable-deterministic-archives"))) + "--enable-deterministic-archives") + ,@(if (string=? (%current-system) "powerpc-linux") + `(#:phases + (modify-phases %standard-phases + (add-after 'unpack 'disable-rust-libiberty-test + (lambda _ + (substitute* "libiberty/testsuite/Makefile.in" + ((" check-rust-demangle ") "")) + #t)))) + '()))) (synopsis "Binary utilities: bfd gas gprof ld") (description diff --git a/gnu/packages/commencement.scm b/gnu/packages/commencement.scm index 7c39a84008..f707a01d30 100644 --- a/gnu/packages/commencement.scm +++ b/gnu/packages/commencement.scm @@ -2653,7 +2653,22 @@ exec " gcc "/bin/" program #:modules ((guix build gnu-build-system) (guix build utils) (ice-9 ftw)) ; for 'scandir' + + ;; #:phases gets modified for powerpc-linux in binutils, + ;; so #:phases here needs to be after the inherited one. + ,@(substitute-keyword-arguments (package-arguments binutils) + ((#:configure-flags cf) + `(cons ,(string-append "--target=" (boot-triplet)) + ,cf))) + #:phases (modify-phases %standard-phases + ,@(if (string=? (%current-system) "powerpc-linux") + '((add-after 'unpack 'disable-rust-libiberty-test + (lambda _ + (substitute* "libiberty/testsuite/Makefile.in" + ((" check-rust-demangle ") "")) + #t))) + '()) (add-after 'install 'add-symlinks (lambda* (#:key outputs #:allow-other-keys) ;; The cross-gcc invokes 'as', 'ld', etc, without the @@ -2667,12 +2682,8 @@ exec " gcc "/bin/" program (with-directory-excursion (string-append out "/bin") (for-each (lambda (name) (symlink name (remove-triplet-prefix name))) - (scandir "." has-triplet-prefix?))))))) + (scandir "." has-triplet-prefix?))))))))) - ,@(substitute-keyword-arguments (package-arguments binutils) - ((#:configure-flags cf) - `(cons ,(string-append "--target=" (boot-triplet)) - ,cf))))) (inputs (%boot0-inputs)))) (define libstdc++-boot0