diff mbox series

[bug#62551] Added new transformation option: --with-configure-flag

Message ID 87y1m4p40i.fsf_-_@gnu.org
State New
Headers show
Series [bug#62551] Added new transformation option: --with-configure-flag | expand

Commit Message

Ludovic Courtès May 4, 2023, 2:25 p.m. UTC
Hi Sarthak,

Sarthak Shah <shahsarthakw@gmail.com> skribis:

> * doc/guix.texi (--with-configure-flag): Added documentation for
> --with-configure-flag
> * guix/transformations.scm (transform-package-configure-flag): New
> function, changes to %transformations and
> show-transformation-options-help/detailed
> * tests/transformations.scm (test-equal "options-transformation,
> with-configure-flag"): Added a test for --with-configure-flag

Nice!

I made the superficial changes below, which can be summarized like this:

  • Allow for equal signs in the configure flag itself.

  • Remove build system check: we don’t do that for the other options;
    instead, document the limitation and leave it up to the user.

  • Tweak the style of the system test to avoid ‘cadr’ (info "(guix)
    Data Types and Pattern Matching").

  • Add a CMake example in the manual and tweak wording.

I’ll follow up with a news entry so people who run ‘guix pull’ can learn
about the new option.

Great work, thank you!

Ludo’.

Comments

pelzflorian (Florian Pelz) May 5, 2023, 11:59 a.m. UTC | #1
Hi all.  This option indeed is great work, however when testing the
CMake example from the news entry, it fails to build.  Is it just me?
It seems like it should work.  Ludo, are you maybe using a lapack from
another channel?

Regards,
Florian
diff mbox series

Patch

diff --git a/doc/guix.texi b/doc/guix.texi
index ff0e62053b..55221a10c3 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -12919,23 +12919,33 @@  Package Transformation Options
 In this example, glibc itself as well as everything that leads to
 Coreutils in the dependency graph is rebuilt.
 
-@item --with-configure-flag=@var{package}=@var{configure-flag}
-Add @var{configure-flag} to the list of configure-flags applied to
-the arguments of @var{package}, where @var{package} is a spec such as
-@code{guile@@3.1} or @code{glibc}. The build system of @var{package}
-must support the argument @code{#:configure-flags}.
+@cindex configure flags, changing them
+@item --with-configure-flag=@var{package}=@var{flag}
+Append @var{flag} to the configure flags of @var{package}, where
+@var{package} is a spec such as @code{guile@@3.0} or @code{glibc}.  The
+build system of @var{package} must support the @code{#:configure-flags}
+argument.
 
-For example, the command below builds GNU Hello with the
-configure-flag @code{--disable-nls}:
+For example, the command below builds GNU@tie{}Hello with the
+configure flag @code{--disable-nls}:
 
 @example
 guix build hello --with-configure-flag=hello=--disable-nls
 @end example
 
-@quotation Warning
-Currently, there is a primitive check for whether the build system
-supports the argument @code{#:configure-flags} or not, however
-users should not rely on it.
+The following command passes an extra flag to @command{cmake} as it
+builds @code{lapack}:
+
+@example
+guix build lapack \
+  --with-configure-flag=lapack=-DBUILD_COMPLEX=OFF
+@end example
+
+@quotation Note
+Under the hood, this option works by passing the
+@samp{#:configure-flags} argument to the build system of the package of
+interest (@pxref{Build Systems}).  Most build systems support that
+option but some do not.  In that case, an error is raised.
 @end quotation
 
 @cindex upstream, latest version
diff --git a/guix/transformations.scm b/guix/transformations.scm
index 9cbac5df9e..943b2768c6 100644
--- a/guix/transformations.scm
+++ b/guix/transformations.scm
@@ -677,49 +677,42 @@  (define (transform-package-tests specs)
         obj)))
 
 (define (transform-package-configure-flag specs)
-  "Return a procedure that, when passed a package and a flag, adds the flag to #:configure-flags in the package's
-'arguments' field."
+  "Return a procedure that, when passed a package and a flag, adds the flag to
+#:configure-flags in the package's 'arguments' field."
   (define (package-with-configure-flag p extra-flag)
     (package/inherit p
       (arguments
        (substitute-keyword-arguments (package-arguments p)
-         ((#:configure-flags list-of-flags (quote '()))
-          #~(cons* #$extra-flag #$list-of-flags))))))
+         ((#:configure-flags flags #~'())
+          ;; Add EXTRA-FLAG to the end so it can potentially override FLAGS.
+          #~(append #$flags '(#$extra-flag)))))))
 
-
-  (define %BUILD-SYSTEMS-WITHOUT-CONFIGURE-FLAGS
-    ;; These build systems do not have a #:configure-flags parameter
-'(android-ndk asdf/sbcl asdf/ecl asdf/source cargo channel chicken clojure copy dub dune elm emacs go guile julia linux-module maven minetest-mod minify node perl rakudo rebar ruby scons texlive tree-sitter trivial))
-
-  (define (build-system-supports-flags? spec)
-    ;; XXX: a more sophisticated approach could be added that checks the given build system for a configure-flags option
-    ;; if a new build system is added, it needs to be added to the %BUILD-SYSTEMS-WITHOUT-CONFIGURE-FLAGS list manually
-    (not (member (build-system-name (package-build-system spec))
-                 %BUILD-SYSTEMS-WITHOUT-CONFIGURE-FLAGS)))
-
-  (define cflags
+  (define configure-flags
     ;; Spec/flag alist.
-     (map (lambda (spec)
-            (match (string-tokenize spec %not-equal)
-              ((spec flag)
-               (cons spec flag))
-              (_
-               (raise (formatted-message
-                       (G_ "~a: invalid package configure-flags specification")
-                       spec)))))
-          specs))
+    (map (lambda (spec)
+           ;; Split SPEC on the first equal sign (the configure flag might
+           ;; contain equal signs, as in '-DINTSIZE=32').
+           (let ((equal (string-index spec #\=)))
+             (match (and equal
+                         (list (string-take spec equal)
+                               (string-drop spec (+ 1 equal))))
+               ((spec flag)
+                (cons spec flag))
+               (_
+                (raise (formatted-message
+                        (G_ "~a: invalid package configure flag specification")
+                        spec))))))
+         specs))
 
   (define rewrite
     (package-input-rewriting/spec
      (map (match-lambda
             ((spec . flags)
              (cons spec (cut package-with-configure-flag <> flags))))
-          cflags)))
+          configure-flags)))
 
   (lambda (obj)
-    (if (and
-         (package? obj)
-         (build-system-supports-flags? obj))
+    (if (package? obj)
         (rewrite obj)
         obj)))
 
@@ -1004,7 +997,7 @@  (define (show-transformation-options-help/detailed)
                          add FILE to the list of patches of PACKAGE"))
   (display (G_ "
       --with-configure-flag=PACKAGE=FLAG
-                         add FLAG to the list of #:configure-flags of PACKAGE"))
+                         append FLAG to the configure flags of PACKAGE"))
   (display (G_ "
       --with-latest=PACKAGE
                          use the latest upstream release of PACKAGE"))
diff --git a/tests/transformations.scm b/tests/transformations.scm
index 31fd042d31..704818b9ed 100644
--- a/tests/transformations.scm
+++ b/tests/transformations.scm
@@ -409,12 +409,15 @@  (define* (depends-on-toolchain? p #:optional (toolchain "gcc-toolchain"))
               (package-arguments (package-replacement dep0))))))))
 
 (test-equal "options->transformation, with-configure-flag"
-  '(cons* "--flag" '())
+  '(append '() '("--flag=42"))
   (let* ((p   (dummy-package "foo"
                 (build-system gnu-build-system)))
-         (t   (options->transformation '((with-configure-flag . "foo=--flag")))))
+         (t   (options->transformation
+               '((with-configure-flag . "foo=--flag=42")))))
     (let ((new (t p)))
-      (gexp->approximate-sexp (cadr (memq #:configure-flags (package-arguments new)))))))
+      (match (package-arguments new)
+        ((#:configure-flags flags)
+         (gexp->approximate-sexp flags))))))
 
 (test-assert "options->transformation, without-tests"
   (let* ((dep (dummy-package "dep"))