On Sat, Apr 17, 2021 at 12:51:01PM -0700, Chris Marusich wrote: > Efraim Flashner 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 אפרים פלשנר GPG key = A28B F40C 3E55 1372 662D 14F7 41AA E7DC CA3D 8351 Confidentiality cannot be guaranteed on emails sent or received unencrypted