| Message ID | DM5PR1001MB2105208A37F6D9141D01B0C8C5EF0@DM5PR1001MB2105.namprd10.prod.outlook.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#44249,v3] gnu: emacs: Make strip-double-wrap more robust. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/issue | success | View issue |
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
Hello, > (Can you reopen this bug report please?) I think you need to open a new one. > So I see 3 possible solutions: > 1. Accept my first patch and give up on match > 2. Accept this patch and modify almost every emacs varient (I did test building them all) > 3. Figure out some proper module inheritence > > I think option 3 is the most correct, but I'm lazy so I'm leaning > towards option 1. I think I cannot help you, as I'm not sure about how module inheritance is handled. Maybe someone else can chime in. Regards,
Nicolas Goaziou <mail@nicolasgoaziou.fr> writes: > Hello, > >> (Can you reopen this bug report please?) > > I think you need to open a new one. > >> So I see 3 possible solutions: >> 1. Accept my first patch and give up on match >> 2. Accept this patch and modify almost every emacs varient (I did test building them all) >> 3. Figure out some proper module inheritence >> >> I think option 3 is the most correct, but I'm lazy so I'm leaning >> towards option 1. > > I think I cannot help you, as I'm not sure about how module inheritance > is handled. Maybe someone else can chime in. I haven't followed the discussion in detail, but from skimming through the thread, is it just about adding (ice-9 match) to the build system modules? In that case I think #:modules ((ice-9 match) ,@%glib-or-gtk-build-system-modules) ...should do the trick.
Hi Morgan, Morgan.J.Smith@outlook.com skribis: > From: Morgan Smith <Morgan.J.Smith@outlook.com> > > * gnu/packages/emacs.scm (emacs) [strip-double-wrap]: Use regex to find emacs > executable. This works even when the version is changed by package > transformations (e.g., version=git.master). [...] > (with-directory-excursion (assoc-ref outputs "out") > - (copy-file (string-append > - "bin/emacs-" > - ,(let ((this-version (package-version this-package))) > - (or (false-if-exception > - (version-major+minor+point this-version)) > - (version-major+minor this-version)))) > - "bin/emacs") > + (copy-file > + (match (find-files "bin" "^emacs-") > + ((executable . _) executable)) If we assume there should be just one “^emacs-” executable, you can change the match clause to reflect it: (match (find-files "bin" "^emacs-") ((executable) executable)) To be even more defensive, you could refine the regexp to “^emacs-[0-9]”. > + "bin/emacs") [...] > + ((#:modules modules) > + `((guix build gnu-build-system) > + (guix build utils) > + (ice-9 match))))) Unless I’m missing something, you don’t need to repeat #:modules in every variant: the ‘arguments’ field is inherited by those variants, and that includes #:modules. You can check easily that re-adding #:modules has no effect by checking the output of, say: ./pre-inst-env guix build emacs-xwidgets -d --no-grafts before and after removing the ((#:modules modules) …) bit. Could you send an updated patch? This is the last missing bit before one can run things like: guix install emacs-next --with-branch=emacs-next=master :-) Thanks, Ludo’.
I've actually been sorta half working on this for a while now. The problem is exactly that the modules field is inherited. See each build system includes its own module in the modules field. The various emacsen are built with different build systems. So emacs is going to need to import (guix build glib-or-gtk-build-system) and emacs-minimal is going to want (guix build gnu-build-system). By setting the modules to be the glib-or-gtk-build-system, we override the default modules in each inherited package. This means building emacs-minimal would result in this error: no code for module (guix build glib-or-gtk-build-system) I'm not entirely certain why it worked for you but it looks like maybe you included the gnu-build system instead of the glib-or-gtk-build-system. I think to solve this issue proper, we need to come up with a way to use %default-modules. Currently this variable isn't usable in this context, but as gnu/packages/code.scm:791 says: ";; FIXME use %default-modules"
Hi, Morgan Smith <Morgan.J.Smith@outlook.com> skribis: > I've actually been sorta half working on this for a while now. > > The problem is exactly that the modules field is inherited. See each > build system includes its own module in the modules field. The various > emacsen are built with different build systems. So emacs is going to > need to import (guix build glib-or-gtk-build-system) and emacs-minimal > is going to want (guix build gnu-build-system). By setting the modules > to be the glib-or-gtk-build-system, we override the default modules in > each inherited package. This means building emacs-minimal would result > in this error: > > no code for module (guix build glib-or-gtk-build-system) Ooh, my bad, I had completely overlooked this “detail”. Then I guess the patch is fine though… in this case you could exceptionally ;-) write (car (find-files …)) so you don’t even need to both importing (ice-9 match). That’d save quite a few lines of code. WDYT? Thanks! Ludo’.
Hi Ludo, I think that's an elegant and wonderful idea. Assuming the first patch in this thread still applies, I vote we just apply that one. There has been some discussion in this thread on if the regex should actually look for numbers or not (notably Nicolas and you). I could go either way. I'm pretty sure the regex that's already there that matches 2 or more period separated numbers will always hold true, but feel free to loosen up the regex a little if you feel otherwise. Thanks, Morgan On 1/16/21 4:54 PM, Ludovic Courtès wrote: > Hi, > > Morgan Smith <Morgan.J.Smith@outlook.com> skribis: > >> I've actually been sorta half working on this for a while now. >> >> The problem is exactly that the modules field is inherited. See each >> build system includes its own module in the modules field. The various >> emacsen are built with different build systems. So emacs is going to >> need to import (guix build glib-or-gtk-build-system) and emacs-minimal >> is going to want (guix build gnu-build-system). By setting the modules >> to be the glib-or-gtk-build-system, we override the default modules in >> each inherited package. This means building emacs-minimal would result >> in this error: >> >> no code for module (guix build glib-or-gtk-build-system) > > Ooh, my bad, I had completely overlooked this “detail”. > > Then I guess the patch is fine though… in this case you could > exceptionally ;-) write (car (find-files …)) so you don’t even need to > both importing (ice-9 match). That’d save quite a few lines of code. > > WDYT? > > Thanks! > > Ludo’. >
diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scm index 4963379d74..4d1080f9dd 100644 --- a/gnu/packages/emacs.scm +++ b/gnu/packages/emacs.scm @@ -123,6 +123,9 @@ (build-system glib-or-gtk-build-system) (arguments `(#:tests? #f ; no check target + #:modules ((guix build glib-or-gtk-build-system) + (guix build utils) + (ice-9 match)) #:configure-flags (list "--with-modules" "--with-cairo" "--disable-build-details") @@ -196,17 +199,12 @@ (lambda* (#:key outputs #:allow-other-keys) ;; Directly copy emacs-X.Y to emacs, so that it is not wrapped ;; twice. This also fixes a minor issue, where WMs would not be - ;; able to track emacs back to emacs.desktop. The version is - ;; accessed using using THIS-PACKAGE so it "just works" for - ;; inherited Emacs packages of different versions. + ;; able to track emacs back to emacs.desktop. (with-directory-excursion (assoc-ref outputs "out") - (copy-file (string-append - "bin/emacs-" - ,(let ((this-version (package-version this-package))) - (or (false-if-exception - (version-major+minor+point this-version)) - (version-major+minor this-version)))) - "bin/emacs") + (copy-file + (match (find-files "bin" "^emacs-") + ((executable . _) executable)) + "bin/emacs") #t))) (add-before 'reset-gzip-timestamps 'make-compressed-files-writable ;; The 'reset-gzip-timestamps phase will throw a permission error @@ -328,7 +326,11 @@ languages.") ((#:phases phases) `(modify-phases ,phases (delete 'restore-emacs-pdmp) - (delete 'strip-double-wrap))))) + (delete 'strip-double-wrap))) + ((#:modules modules) + `((guix build gnu-build-system) + (guix build utils) + (ice-9 match))))) (inputs `(("guix-emacs.el" ,(search-auxiliary-file "emacs/guix-emacs.el")) ("ncurses" ,ncurses))) @@ -348,7 +350,11 @@ editor (with xwidgets support)") ((#:phases phases) `(modify-phases ,phases (delete 'restore-emacs-pdmp) - (delete 'strip-double-wrap))))) + (delete 'strip-double-wrap))) + ((#:modules modules) + `((guix build gnu-build-system) + (guix build utils) + (ice-9 match))))) (inputs `(("webkitgtk" ,webkitgtk) ("libxcomposite" ,libxcomposite) @@ -375,7 +381,11 @@ editor (console only)") ((#:phases phases) `(modify-phases ,phases (delete 'restore-emacs-pdmp) - (delete 'strip-double-wrap))))))) + (delete 'strip-double-wrap))) + ((#:modules modules) + `((guix build gnu-build-system) + (guix build utils) + (ice-9 match))))))) (define-public emacs-no-x-toolkit (package/inherit emacs @@ -392,7 +402,11 @@ editor (without an X toolkit)" ) ((#:phases phases) `(modify-phases ,phases (delete 'restore-emacs-pdmp) - (delete 'strip-double-wrap))))))) + (delete 'strip-double-wrap))) + ((#:modules modules) + `((guix build gnu-build-system) + (guix build utils) + (ice-9 match))))))) (define-public emacs-wide-int (package/inherit emacs
From: Morgan Smith <Morgan.J.Smith@outlook.com> * gnu/packages/emacs.scm (emacs) [strip-double-wrap]: Use regex to find emacs executable. This works even when the version is changed by package transformations (e.g., version=git.master). --- (Can you reopen this bug report please?) So I see 3 possible solutions: 1. Accept my first patch and give up on match 2. Accept this patch and modify almost every emacs varient (I did test building them all) 3. Figure out some proper module inheritence I think option 3 is the most correct, but I'm lazy so I'm leaning towards option 1. --- gnu/packages/emacs.scm | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-)