| Message ID | 950a7d3637ddf17be84f52dc2ae0aac2ce4c4be6.1725547778.git.omar.bassam88@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#72925] adding jpm package | expand |
Hi Omar, Thank you for submitting this patch. A few comments: Omar Bassam <omar.bassam88@gmail.com> writes: > Change-Id: I730ef2f5c874c5142a580a42af76180e95d93ccd > --- > gnu/packages/lisp.scm | 52 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 52 insertions(+) > > diff --git a/gnu/packages/lisp.scm b/gnu/packages/lisp.scm > index 5d4399f145..96698d375d 100644 Could you please update the reroll-count when you send amendments to the patch? IIUC, this version should have been v2. > +(define-public jpm > + (package > + (name "jpm") > + (version "1.1.0") > + (source (origin > + (method git-fetch) > + (uri (git-reference > + (url "https://github.com/janet-lang/jpm.git") > + (commit (string-append "v" version)))) > + (file-name (git-file-name name version)) > + (sha256 (base32 "05rdxigmiy7vf93s16a8n2029lq33073jccz1rjl4iisxj6piw4l")))) > + (build-system trivial-build-system) > ... > + (substitute* (string-append %output "/bin/jpm") > + (("/usr/bin/env janet") > + (string-append #$janet "/bin/janet"))) Is my understanding correct that you're updating the shebang line here? If so, would it be better to use the copy-build-system instead? If not, could you please elaborate? If copy-build-system does indeed turn out to be better suited, could you please send v3 of the patch?
Thank you for taking the time to look into my patch. Sorry, I'm new to Guix and to this workflow. So, forgive me if my questions look a bit naive: 1. What do you mean by reroll count for the patch? 2. I looked at the copy-build-system documentation. I'm not sure how it can be used here. I'm not just updating the shebang. As you can already see in the patch, I'm doing a lot of string substitutions in the source code itself because some values are hard coded. That's why I preferred to use the trivial-build-system to have more control of what I need to substitute. Thanks, Omar On Wed, 18 Sept 2024 at 16:19, Suhail Singh <suhailsingh247@gmail.com> wrote: > Hi Omar, > > Thank you for submitting this patch. A few comments: > > Omar Bassam <omar.bassam88@gmail.com> writes: > > > Change-Id: I730ef2f5c874c5142a580a42af76180e95d93ccd > > --- > > gnu/packages/lisp.scm | 52 +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 52 insertions(+) > > > > diff --git a/gnu/packages/lisp.scm b/gnu/packages/lisp.scm > > index 5d4399f145..96698d375d 100644 > > Could you please update the reroll-count when you send amendments to the > patch? IIUC, this version should have been v2. > > > +(define-public jpm > > + (package > > + (name "jpm") > > + (version "1.1.0") > > + (source (origin > > + (method git-fetch) > > + (uri (git-reference > > + (url "https://github.com/janet-lang/jpm.git") > > + (commit (string-append "v" version)))) > > + (file-name (git-file-name name version)) > > + (sha256 (base32 > "05rdxigmiy7vf93s16a8n2029lq33073jccz1rjl4iisxj6piw4l")))) > > + (build-system trivial-build-system) > > ... > > + (substitute* (string-append %output "/bin/jpm") > > + (("/usr/bin/env janet") > > + (string-append #$janet "/bin/janet"))) > > Is my understanding correct that you're updating the shebang line here? > If so, would it be better to use the copy-build-system instead? If not, > could you please elaborate? If copy-build-system does indeed turn out > to be better suited, could you please send v3 of the patch? > > -- > Suhail >
Omar Bassam <omar.bassam88@gmail.com> writes: > Thank you for taking the time to look into my patch. Sorry, I'm new to Guix > and to this workflow. So, forgive me if my questions look a bit naive: > 1. What do you mean by reroll count for the patch? Please refer to the man page of git-format-patch and look for --reroll-count : #+begin_quote -v <n>, --reroll-count=<n> Mark the series as the <n>-th iteration of the topic. The output filenames have v<n> prepended to them, and the subject prefix ("PATCH" by default, but configurable via the --subject-prefix option) has ` v<n>` appended to it. E.g. --reroll-count=4 may produce v4-0001-add-makefile.patch file that has "Subject: [PATCH v4 1/20] Add makefile" in it. <n> does not have to be an integer (e.g. "--reroll-count=4.4", or "--reroll-count=4rev2" are allowed), but the downside of using such a reroll-count is that the range-diff/interdiff with the previous version does not state exactly which version the new iteration is compared against. #+end_quote > 2. I looked at the copy-build-system documentation. I'm not sure how it can > be used here. I'm not just updating the shebang. As you can already see in > the patch, I'm doing a lot of string substitutions in the source code > itself because some values are hard coded. That's why I preferred to use > the trivial-build-system to have more control of what I need to substitute. Based on my understanding of the patch you are copying files, updating some references in files, and setting environment variables. I believe all of these are possible via the copy-build-system as well which is described as: #+begin_quote ;; Standard build procedure for simple packages that don't require much ;; compilation, mostly just copying files around. This is implemented as an ;; extension of `gnu-build-system'. #+end_quote If you'd like to learn more, you can grep under ./gnu/packages and look at some instances where it's used. I don't have experience with the trivial-build-system, which is why I wondered. > + (setenv "PREFIX" %output) > + (setenv "JANET_PREFIX" %output) > + (setenv "JANET_LIBPATH" (string-append %output "/lib/janet")) > + (setenv "JANET_MODPATH" (string-append %output "/lib/janet")) What would be a way to test that the above is doing the "correct" thing? Is there a sequence of steps that I can evaluate which will yield a different outcome depending on whether or not the above accomplishes what it intends to? Put another way, what breaks when the above aren't set (and how do I observe that failure)?
diff --git a/gnu/packages/lisp.scm b/gnu/packages/lisp.scm index 5d4399f145..96698d375d 100644 --- a/gnu/packages/lisp.scm +++ b/gnu/packages/lisp.scm @@ -29,6 +29,7 @@ ;;; Copyright © 2024 Andreas Enge <andreas@enge.fr> ;;; Copyright © 2024 bigbug <bigbookofbug@proton.me> ;;; Copyright © 2024 Ashish SHUKLA <ashish.is@lostca.se> +;;; Copyright © 2024 Omar Bassam <omar.bassam88@gmail.com> ;;; ;;; This file is part of GNU Guix. ;;; @@ -917,6 +918,57 @@ (define-public janet assembler, PEG) is less than 1MB.") (license license:expat))) +(define-public jpm + (package + (name "jpm") + (version "1.1.0") + (source (origin + (method git-fetch) + (uri (git-reference + (url "https://github.com/janet-lang/jpm.git") + (commit (string-append "v" version)))) + (file-name (git-file-name name version)) + (sha256 (base32 "05rdxigmiy7vf93s16a8n2029lq33073jccz1rjl4iisxj6piw4l")))) + (build-system trivial-build-system) + (arguments + (list + #:modules `((guix build utils)) + #:builder #~(begin + (use-modules (guix build utils)) + (mkdir %output) + (for-each (lambda (dir) (mkdir (string-append %output "/" dir))) + '("bin" "lib" "share" "share/man" "lib/janet" + "lib/janet/jpm" "share/man/man1")) + (copy-recursively (assoc-ref %build-inputs "source") "source") + (chdir "source") + (substitute* "configs/linux_config.janet" + (("auto-shebang true") "auto-shebang false")) + (substitute* "configs/linux_config.janet" + (("/usr/local") %output)) + (substitute* "jpm/shutil.janet" + (("cp") (string-append #$coreutils "/bin/cp"))) + (substitute* "jpm/declare.janet" + (("chmod") (string-append #$coreutils "/bin/chmod"))) + (setenv "PREFIX" %output) + (setenv "JANET_PREFIX" %output) + (setenv "JANET_LIBPATH" (string-append %output "/lib/janet")) + (setenv "JANET_MODPATH" (string-append %output "/lib/janet")) + (system* (string-append #$janet "/bin/janet") + "bootstrap.janet" "configs/linux_config.janet") + (substitute* (string-append %output "/bin/jpm") + (("/usr/bin/env janet") + (string-append #$janet "/bin/janet"))) + (copy-recursively (string-append #$janet "/include/janet") + (string-append %output "/include/janet")) + (copy-recursively (string-append #$janet "/lib") + (string-append %output "/lib"))))) + (inputs (list janet coreutils)) + (home-page "https://janet-lang.org/") + (synopsis "Janet Project Manager for the Janet programming language") + (description "JPM is the Janet Project Manager tool. It is for automating +builds and downloading dependencies of Janet projects.") + (license license:expat))) + (define-public lisp-repl-core-dumper (package (name "lisp-repl-core-dumper")