bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
Excellent, thanks a lot! :) signature.asc Description: PGP signature
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
Pierre Neidhardt skribis: > I'll be the road for a while, unable to work on this patch, so if anyone > wants to work on it and merge, please go ahead :) > > Left to do: > > - Suggestion: add a keyword to choose between asdf:compile-system and > asdf:load-system (default should be asdf:load-system). > - Make sure sbcl-stumpwm-kbd-layouts usees asdf:compile-system. > - Rebuild the Lisp world to test. I added a 'asd-operation' keyword parameter with a default value of "load-system", and I used it in the package definition of sbcl-stumpwm-kbd-layouts to use "compile-system" instead. Patches pushed as 6b5ef03a2582ab23228478018fd356e17db1daea and following. signature.asc Description: PGP signature
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
I've pushed the SBCL closure size reduction. I'll be the road for a while, unable to work on this patch, so if anyone wants to work on it and merge, please go ahead :) Left to do: - Suggestion: add a keyword to choose between asdf:compile-system and asdf:load-system (default should be asdf:load-system). - Make sure sbcl-stumpwm-kbd-layouts usees asdf:compile-system. - Rebuild the Lisp world to test. Cheers! Pierre signature.asc Description: PGP signature
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
While we are rebuilding the Lisp world, I suggest we remove Coreutils
from the SBCL closure since it's only needed on LispWorks and on
non-Linux:
--8<---cut here---start->8---
(add-after 'install 'remove-coreutils-references
;; They are only useful on non-Linux, non-SBCL.
(lambda* (#:key outputs #:allow-other-keys)
(let* ((out (assoc-ref outputs "out"))
(share-dir (string-append out "/share/sbcl/")))
(substitute* (string-append share-dir
"src/code/run-program.lisp")
(("\\(run-program \".*uname\"")
"(run-program \"uname\""))
(substitute* (string-append share-dir "contrib/asdf/asdf.lisp")
(("\\(\".*/usr/bin/env\"")
"(\"/usr/bin/env\""))
(substitute* (string-append share-dir "contrib/asdf/uiop.lisp")
(("\\(\".*/usr/bin/env\"")
"(\"/usr/bin/env\""))
#t)))
--8<---cut here---end--->8---
signature.asc
Description: PGP signature
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
Find the first draft attached. Do not merge, we need to figure out what to do with sbcl-stumpwm-kbd-layouts. signature.asc Description: PGP signature This fixes 2 flaws in the asdf-build-system: 1. Use asdf:load-system instead of asdf:compile-system. The latter is not recommended by the manual and fails with at least 2 systems. 2. Add the build directory to the ASDF registry so that .asd files are automatically found. This has a double benefit: - It dramatically simplifies package definition writing. - It fixes a bug which used to cause the check phase to fail. All commits after the first two adapt the Common Lisp package definitions to the new build system and in particular fix many check phases. >From fd4eb6c4d5fce8d3c1ef205b541ddf76ed0c478a Mon Sep 17 00:00:00 2001 From: Pierre Neidhardt Date: Fri, 1 Jul 2022 16:37:44 +0200 Subject: [PATCH 01/18] guix: build: Switch from asdf:compile-system to asdf:load-system. * guix/build/lisp-utils.scm (compile-systems): Switch from asdf:compile-system to asdf:load-system. According to the ASDF manual: This will make sure all the files in the system are compiled, but not necessarily load any of them in the current image; on most systems, it will _not_ load all compiled files in the current image. This function exists for symmetry with 'load-system' but is not recommended unless you are writing build scripts and know what you're doing. --- guix/build/lisp-utils.scm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/guix/build/lisp-utils.scm b/guix/build/lisp-utils.scm index 17d2637f87..bd6b21d5a6 100644 --- a/guix/build/lisp-utils.scm +++ b/guix/build/lisp-utils.scm @@ -116,7 +116,7 @@ (define (compile-systems systems asd-files) `(asdf:load-asd (truename ,asd-file))) asd-files) ,@(map (lambda (system) - `(asdf:compile-system ,system)) + `(asdf:load-system ,system)) systems (define (test-system system asd-files test-asd-file) -- 2.32.0 >From 2395f5bef855cb734d13745b9c90a3bbae550d56 Mon Sep 17 00:00:00 2001 From: Pierre Neidhardt Date: Fri, 1 Jul 2022 17:17:32 +0200 Subject: [PATCH 02/18] build-system: asdf: Let ASDF locate the .asd files. * guix/build-system/asdf.scm (package-with-build-system): Remove 'asd-files' and replace 'test-asd-file' by 'asd-test-systems'. (lower): Same. * guix/build/asdf-build-system.scm (source-asd-file): Remove since ASDF does it better than us. (find-asd-files): Same. (build): Remove unused asd-files argument. (check): Remove asd-files argument and replace asd-systems by asd-test-systems. * guix/build/lisp-utils.scm (compile-systems): Call to ASDF to find the systems. (test-system): Same. This approach has many benefits: - It's simplifies the build system. - The package definitions are easier to write. - It fixes a bug with systems that call asdf:clear-system which would cause the load to fail. See for instance test systems using Prove. --- guix/build-system/asdf.scm | 13 +++- guix/build/asdf-build-system.scm | 28 ++--- guix/build/lisp-utils.scm| 35 ++-- 3 files changed, 29 insertions(+), 47 deletions(-) diff --git a/guix/build-system/asdf.scm b/guix/build-system/asdf.scm index a0f4634db0..98231714d9 100644 --- a/guix/build-system/asdf.scm +++ b/guix/build-system/asdf.scm @@ -202,7 +202,7 @@ (define (new-inputs inputs-getter) (define base-arguments (if target-is-source? (strip-keyword-arguments - '(#:tests? #:asd-files #:lisp #:asd-systems #:test-asd-file) + '(#:tests? #:lisp #:asd-systems #:asd-test-systems) (package-arguments pkg)) (package-arguments pkg))) @@ -270,9 +270,8 @@ (define (asdf-build lisp-type) (lambda* (name inputs #:key source outputs (tests? #t) - (asd-files ''()) (asd-systems ''()) - (test-asd-file #f) + (asd-test-systems ''()) (phases '%standard-phases) (search-paths '()) (system (%current-system)) @@ -292,6 +291,11 @@ (define systems `(quote ,(list package-name))) asd-systems)) +(define test-systems + (if (null? (cadr asd-test-systems)) + systems + asd-test-systems)) + (define builder (with-imported-modules imported-modules #~(begin @@ -302,9 +306,8 @@ (define builder (%lisp-type #$lisp-type)) (asdf-build #:name #$name #:source #+source - #:asd-files #$asd-files #:asd-systems #$systems - #:test-asd-file #$test-asd-file + #:asd-test-systems #$test-systems #:system #$system #:tests? #$tests?
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
I found a blocker: Some StumpWM contribs like sbcl-stumpwm kbd-layout make calls at the top level which expect a running session of StumpWM, and thus asd:load-system will fail on them, while asdf:compile-system used to work. Suggestion: add an option to our build system to choose between asdf:load-system and asdf:compile-system. We default to asdf:load-system and use asdf:compile-system in stumpwm-contrib. Thoughts? signature.asc Description: PGP signature
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
Pierre Neidhardt skribis: > Robert Goldman from ASDF found out why the "COMPONENT not found" issue > happens: > > https://gitlab.common-lisp.net/asdf/asdf/-/issues/119#note_9808 > > So either we fix most of the Prove-depending libraries, or we just do > what's expected from every one, that is, add the directory to the ASDF > registries. > > The latter is much easier of course... As adding the build directory to ASDF's registry is easier and also simplifies asdf-build-system, would should do that. And we could still open issues upstream for libraries that are starting their test suites in a wrong way. signature.asc Description: PGP signature
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
Robert Goldman from ASDF found out why the "COMPONENT not found" issue happens: https://gitlab.common-lisp.net/asdf/asdf/-/issues/119#note_9808 So either we fix most of the Prove-depending libraries, or we just do what's expected from every one, that is, add the directory to the ASDF registries. The latter is much easier of course... signature.asc Description: PGP signature
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
Exactly, I already wrote the patch that did! :) Will send soon, need to do some more testing. signature.asc Description: PGP signature
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
Pierre Neidhardt skribis: > Hi Guillaume, > > I gave it a go and your suggestion indeed cuts it for cleavir and > cl-gamepad. > > It did not fix it for the tests though. > > I did some more testing, and this is what I found out on one of the > failing systems (cl-reexport): from a --pure sbcl repl, if I load the > .asd files manually then run test-system, I can reproduce the issue. > > However, if I run > > (asdf:initialize-source-registry > `(:source-registry (:tree ,(uiop:getcwd)) >:inherit-configuration)) > (asdf:test-system :cl-reexport) > > then it works! > > In other words, I believe that `asdf:load-asd' is yet another under-used > ASDF function, and we should probably go with the officially recommended > way, namely adding the source folder to the ASDF registry. > > Thoughts? > > I'll try it out then send a patch. > > Cheers! I think using the 'initialize-source-registry' technique instead of 'load-asd' would also make the '#:asd-files' and '#:test-asd-file' arguments of the build system unnecessary, so they could be removed.
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
Hi Guillaume, I gave it a go and your suggestion indeed cuts it for cleavir and cl-gamepad. It did not fix it for the tests though. I did some more testing, and this is what I found out on one of the failing systems (cl-reexport): from a --pure sbcl repl, if I load the .asd files manually then run test-system, I can reproduce the issue. However, if I run --8<---cut here---start->8--- (asdf:initialize-source-registry `(:source-registry (:tree ,(uiop:getcwd)) :inherit-configuration)) (asdf:test-system :cl-reexport) --8<---cut here---end--->8--- then it works! In other words, I believe that `asdf:load-asd' is yet another under-used ASDF function, and we should probably go with the officially recommended way, namely adding the source folder to the ASDF registry. Thoughts? I'll try it out then send a patch. Cheers! signature.asc Description: PGP signature
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
Am Freitag, dem 01.07.2022 um 12:16 +0200 schrieb Pierre Neidhardt: > [...] > > From the ASDF doc: > > --8<---cut here---start->8--- > This will make sure all the files in the system are compiled, but not > necessarily load any of them in the current image; on most systems, > it will _not_ load all compiled files in the current image. This > function exists for symmetry with 'load-system' but is not > recommended *unless you are writing build scripts* and know what > you're doing. > --8<---cut here---end--->8--- > > So should we really use it? In this case, I'd argue that we *are* the build script and that packagers know what they're doing when they override build in case that asdf:compile-system fails. Unless I'm wrong, we're not actually interested in loading all compiled files into the current image, are we? Cheers
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
Hi Liliana, It's tempting to think that Guix packages are good candidates for "build scripts", but in the face of it, it may very well be that ASDF authors had something completely different in mind. In any case, asdf:compile-system seems to be underused to the point that barely anyone beside Guix packagers ever experience it. Cheers! signature.asc Description: PGP signature
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
Pierre Neidhardt skribis: > Do you have time to try it out? Not right now, as I'm about to take a vacation. The main change is a one-liner in the 'compile-systems' function in "guix/build/lisp-utils.scm"; that would be quick. However recompiling all the Lisp packages and finding which of them could be simplified thanks to the change would take more time... So, if you have the time, go for it. signature.asc Description: PGP signature
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
Do you have time to try it out? signature.asc Description: PGP signature
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
Pierre Neidhardt skribis: > While trying to package > > https://github.com/s-expressionists/Cleavir > > I hit a strange issue in which it would fail to compile, while calling > `asdf:load-system' locally worked. > > Then I realized that our asdf-build-system/sbcl uses > `asdf:compile-system' instead of `asdf:load-system'. > > From the ASDF doc: > > This will make sure all the files in the system are compiled, but not > necessarily load any of them in the current image; on most systems, it > will _not_ load all compiled files in the current image. This function > exists for symmetry with 'load-system' but is not recommended unless you > are writing build scripts and know what you're doing. > > > So should we really use it? > > By the way this _may_ be related to the issue we've got with loading the > tests of some packages, like sbcl-jonathan: > > ;; Tests fail with: Component JONATHAN-ASD::JONATHAN-TEST not found, > ;; required by #. Why? > > [...] Hi, The cl-gamepad package has a similar issue (and a custom build phase using load-system instead of compile-system as a workaround). If the doc of ASDF indicates that load-system is the preferred way to compile systems, we should probably do that, and remove current workarounds and check if everything is still working. signature.asc Description: PGP signature
bug#56334: Should asdf-build-system/sbcl use load-system instead of compile-system?
While trying to package https://github.com/s-expressionists/Cleavir I hit a strange issue in which it would fail to compile, while calling `asdf:load-system' locally worked. Then I realized that our asdf-build-system/sbcl uses `asdf:compile-system' instead of `asdf:load-system'. From the ASDF doc: --8<---cut here---start->8--- This will make sure all the files in the system are compiled, but not necessarily load any of them in the current image; on most systems, it will _not_ load all compiled files in the current image. This function exists for symmetry with 'load-system' but is not recommended unless you are writing build scripts and know what you're doing. --8<---cut here---end--->8--- So should we really use it? By the way this _may_ be related to the issue we've got with loading the tests of some packages, like sbcl-jonathan: --8<---cut here---start->8--- ;; Tests fail with: Component JONATHAN-ASD::JONATHAN-TEST not found, ;; required by #. Why? --8<---cut here---end--->8--- Recipe to reproduce: - git clone https://github.com/s-expressionists/Cleavir - cd Cleavir - guix shell sbcl sbcl-acclimation sbcl-concrete-syntax-tree sbcl-closer-mop -- sbcl - (asdf:initialize-source-registry `(:source-registry (:tree ,(uiop:getcwd)) :inherit-configuration)) - (asdf:compile-system :cleavir-abstract-interpreter) --8<---cut here---start->8--- debugger invoked on a SB-PCL:CLASS-NOT-FOUND-ERROR in thread #: There is no class named CLEAVIR-ABSTRACT-INTERPRETER:STRATEGY. Type HELP for debugger help, or (SB-EXT:EXIT) to exit from SBCL. restarts (invokable by number or by possibly-abbreviated name): 0: [TRY-RECOMPILING ] Recompile control and try loading it again 1: [RETRY] Retry loading FASL for #. 2: [ACCEPT ] Continue, treating loading FASL for # as having been successful. 3: Retry ASDF operation. 4: [CLEAR-CONFIGURATION-AND-RETRY] Retry ASDF operation after resetting the configuration. 5: Retry ASDF operation. 6: Retry ASDF operation after resetting the configuration. 7: [ABORT] Exit debugger, returning to top level. --8<---cut here---end--->8--- And then - (asdf:load-system :cleavir-abstract-interpreter) works like a charm! Thoughts? Pierre signature.asc Description: PGP signature
