diff mbox series

[bug#72925,v12,2/2] gnu: Improve user-experience for jpm.

Message ID 4665e2a478a63cf0bc6b8a18a2fb44687298bbd7.1728359428.git.suhail@bayesians.ca
State New
Headers show
Series gnu: Add jpm package. | expand

Commit Message

Suhail Singh Oct. 8, 2024, 4:16 a.m. UTC 
* gnu/packages/lisp.scm (jpm): Ensure jpm respects JANET_HEADERPATH and
JANET_LIBPATH if set by user.  Ensure gcc/g++ is able to find header files and
compilation-related utilities.

Change-Id: Ic7218dbd10e6fabddded50894b82492de8cabc88
---
 gnu/packages/lisp.scm | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

Comments

Ludovic Courtès Oct. 12, 2024, 5:22 p.m. UTC | #1 
Suhail Singh <suhailsingh247@gmail.com> skribis:

> * gnu/packages/lisp.scm (jpm): Ensure jpm respects JANET_HEADERPATH and
> JANET_LIBPATH if set by user.  Ensure gcc/g++ is able to find header files and
> compilation-related utilities.
>
> Change-Id: Ic7218dbd10e6fabddded50894b82492de8cabc88

[...]

> +                         (wrap-program (string-append #$output "/bin/jpm")
> +                           `("JANET_HEADERPATH" ":" prefix
> +                             (,(string-append #$janet "/include/janet")))
> +                           `("JANET_LIBPATH" ":" prefix
> +                             (,(string-append #$janet "/lib")))
> +                           `("C_INCLUDE_PATH" ":" prefix
> +                             (,(string-append gcc-toolchain "/include")))
> +                           `("CPLUS_INCLUDE_PATH" ":" prefix
> +                             (,(string-append gcc-toolchain "/include/c++")
> +                              ,(string-append gcc-toolchain "/include")))
> +                           `("LIBRARY_PATH" ":" prefix
> +                             (,(string-append gcc-toolchain "/lib")
> +                              ,(string-append gcc-toolchain "/lib64")))
> +                           `("PATH" ":" prefix
> +                             (,(string-append gcc-toolchain "/bin")
> +                              ,(string-append #$coreutils "/bin"))))))))))
> +    (inputs (list bash-minimal
> +                  ;; Lazily resolve the gcc-toolchain to avoid a circular
> +                  ;; dependency.
> +                  (module-ref (resolve-interface '(gnu packages commencement))
> +                              'gcc-toolchain)))

I suppose JPM shells out to GCC to compiler Janet (or C?) code, right?

I’d recommend adding ‘gcc’, ‘glibc’, ‘binutils’, and ‘ld-wrapper’ to
‘inputs’; that’d less us avoid the ‘gcc-toolchain’ dance.

Then in, the phase above, make sure to ‘search-input-file’ rather than
direct references to these variables.

How does that sound?

Alternatively, depending on how important this is for JPM, you could
also leave it up to users to install ‘gcc-toolchain’ alongside JPM when
they need it.

Thanks,
Ludo’.
Suhail Singh Oct. 12, 2024, 8:22 p.m. UTC | #2 
Ludovic Courtès <ludo@gnu.org> writes:

> I suppose JPM shells out to GCC to compiler Janet (or C?) code, right?

Yes.

> I’d recommend adding ‘gcc’, ‘glibc’, ‘binutils’, and ‘ld-wrapper’ to
> ‘inputs’; that’d less us avoid the ‘gcc-toolchain’ dance.
>
> Then in, the phase above, make sure to ‘search-input-file’ rather than
> direct references to these variables.
>
> How does that sound?

Sounds good.

> Alternatively, depending on how important this is for JPM, you could
> also leave it up to users to install ‘gcc-toolchain’ alongside JPM when
> they need it.

It's a pretty essential dependency for JPM, so adding gcc, glibc etc to
inputs would be preferable.

I'll submit v13 shortly.  I look forward to your elaboration on why
direct references aren't preferable.  Thank you for taking the time to
review.
Suhail Singh Oct. 12, 2024, 9:14 p.m. UTC | #3 
Ludovic Courtès <ludo@gnu.org> writes:

>> +                         (wrap-program (string-append #$output "/bin/jpm")
>> +                           `("JANET_HEADERPATH" ":" prefix
>> +                             (,(string-append #$janet "/include/janet")))
>> +                           `("JANET_LIBPATH" ":" prefix
>> +                             (,(string-append #$janet "/lib")))
>> +                           `("C_INCLUDE_PATH" ":" prefix
>> +                             (,(string-append gcc-toolchain "/include")))
>> +                           `("CPLUS_INCLUDE_PATH" ":" prefix
>> +                             (,(string-append gcc-toolchain "/include/c++")
>> +                              ,(string-append gcc-toolchain "/include")))
>> +                           `("LIBRARY_PATH" ":" prefix
>> +                             (,(string-append gcc-toolchain "/lib")
>> +                              ,(string-append gcc-toolchain "/lib64")))
>> +                           `("PATH" ":" prefix
>> +                             (,(string-append gcc-toolchain "/bin")
>> +                              ,(string-append #$coreutils "/bin"))))))))))
>> +    (inputs (list bash-minimal
>> +                  ;; Lazily resolve the gcc-toolchain to avoid a circular
>> +                  ;; dependency.
>> +                  (module-ref (resolve-interface '(gnu packages commencement))
>> +                              'gcc-toolchain)))
>
> I suppose JPM shells out to GCC to compiler Janet (or C?) code, right?
>
> I’d recommend adding ‘gcc’, ‘glibc’, ‘binutils’, and ‘ld-wrapper’ to
> ‘inputs’; that’d less us avoid the ‘gcc-toolchain’ dance.

It seems "ld-wrapper" requires (gnu packages commencement) and this
results in doing the lazy dereference similar to gcc-toolchain above.
Am I doing something wrong?  If not, is the guidance still to add
"ld-wrapper" to inputs?

> Then in, the phase above, make sure to ‘search-input-file’ rather than
> direct references to these variables.

Hmm just to be clear, instead of:

#+begin_src scheme
  `("C_INCLUDE_PATH" ":" prefix
    (,(string-append gcc-toolchain "/include")))
#+end_src

What we want is something like:

#+begin_src scheme
  `("C_INCLUDE_PATH" ":" prefix
    (,(search-input-file inputs "/include")))
#+end_src

Is my understanding correct?

If so, adding "gcc", "glibc", "binutils", and "ld-wrapper" seems
insufficient to make that work.  The search-input-file for "/include"
results in a "&search-error" when building the package.
diff mbox series

Patch

diff --git a/gnu/packages/lisp.scm b/gnu/packages/lisp.scm
index 04b56ea99b..4223540913 100644
--- a/gnu/packages/lisp.scm
+++ b/gnu/packages/lisp.scm
@@ -30,6 +30,7 @@ 
 ;;; Copyright © 2024 bigbug <bigbookofbug@proton.me>
 ;;; Copyright © 2024 Ashish SHUKLA <ashish.is@lostca.se>
 ;;; Copyright © 2024 Omar Bassam <omar.bassam88@gmail.com>
+;;; Copyright © 2024 Suhail Singh <suhail@bayesians.ca>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -959,18 +960,34 @@  (define-public jpm
                        (setenv "PREFIX"
                                #$output)))
                    (replace 'install
-                     (lambda _
+                     (lambda* (#:key inputs #:allow-other-keys)
                        (for-each (lambda (dir)
                                    (mkdir-p (string-append #$output "/" dir)))
                                  '("lib/janet/jpm" "share/man/man1"))
                        (invoke "janet" "bootstrap.janet"
                                "configs/linux_config.janet")
-                       (wrap-program (string-append #$output "/bin/jpm")
-                         `("JANET_HEADERPATH" ":" =
-                           (,(string-append #$janet "/include/janet")))
-                         `("JANET_LIBPATH" ":" =
-                           (,(string-append #$janet "/lib")))))))))
-    (inputs (list bash-minimal))
+                       (let ((gcc-toolchain (assoc-ref inputs "gcc-toolchain")))
+                         (wrap-program (string-append #$output "/bin/jpm")
+                           `("JANET_HEADERPATH" ":" prefix
+                             (,(string-append #$janet "/include/janet")))
+                           `("JANET_LIBPATH" ":" prefix
+                             (,(string-append #$janet "/lib")))
+                           `("C_INCLUDE_PATH" ":" prefix
+                             (,(string-append gcc-toolchain "/include")))
+                           `("CPLUS_INCLUDE_PATH" ":" prefix
+                             (,(string-append gcc-toolchain "/include/c++")
+                              ,(string-append gcc-toolchain "/include")))
+                           `("LIBRARY_PATH" ":" prefix
+                             (,(string-append gcc-toolchain "/lib")
+                              ,(string-append gcc-toolchain "/lib64")))
+                           `("PATH" ":" prefix
+                             (,(string-append gcc-toolchain "/bin")
+                              ,(string-append #$coreutils "/bin"))))))))))
+    (inputs (list bash-minimal
+                  ;; Lazily resolve the gcc-toolchain to avoid a circular
+                  ;; dependency.
+                  (module-ref (resolve-interface '(gnu packages commencement))
+                              'gcc-toolchain)))
     (propagated-inputs (list janet))
     (native-search-paths
      (list $SSL_CERT_DIR $SSL_CERT_FILE))