Message ID | 20220413120052.25602-1-attila@lendvai.name |
---|---|
State | Accepted |
Headers | show |
Series | [bug#54906] build: go-build-system: Add support for #:skip-build? #t. | expand |
Context | Check | Description |
---|---|---|
cbaines/comparison | success | View comparision |
cbaines/git branch | success | View Git branch |
cbaines/applying patch | success | View Laminar job |
cbaines/issue | success | View issue |
Thanks for the patch! I’m not qualified to evaluate this, so I Cc’d Sarah and Leo who have previously worked on the go-build-system. @Sarah and @Leo: Could you please comment on the issue at https://issues.guix.gnu.org/54906?
kindly pinging the involved parties, because i have some extensive work on the golang importer(*) that depends on this, or at least on the decision whether this will be merged or not. the longer those commits are sitting on my side, the higher the chance of a commit to master that will lead to a painful merge session... https://github.com/attila-lendvai-patches/guix/commits/import -- • attila lendvai • PGP: 963F 5D5F 45C7 DFCD 0A39 -- Every task involves constraint, Solve the thing without complaint; There are magic links and chains Forged to loose our rigid brains. Structures, strictures, though they bind, Strangely liberate the mind.
(Please forgive me for using your patch to try out reviewing some Guix patches! This is also my first time reviewing code over email, so feedback welcome!) I don't have the context for why such a change is needed, but purely from a code perspective this LGTM. Unfortunately I have no power to commit this.
Attila Lendvai schreef op wo 13-04-2022 om 14:00 [+0200]: > This mimics the same feature of the cargo-build-system. > > * guix/build-system/go.scm (go-build): Add skip-build? keyword param and > propagate it. > * guix/build/go-build-system.scm (build): Add skip-build? keyword param. The new WIP antioxidant-build-system (intended to replace cargo-build- system) will not have #:skip-build?, because the new build system actually reuses the build results off the dependents. Likewise, maybe long-term someone will figure out how to do something similar for go -- e.g., https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32919#5 mentions a ‘go build cache’. So it seems more of a work-around than a feature to me. Maybe after building, the new cache entries could be copied to an output, and before building, the cache could be populated by old cache entries from dependents? That would allow for only having to compile the dependencies only once, reusing them for all dependents. Greetings, Maxime
> The new WIP antioxidant-build-system (intended to replace cargo-build- > system) will not have #:skip-build?, because the new build system > actually reuses the build results off the dependents. > > Likewise, maybe long-term someone will figure out how to do something > similar for go -- e.g., > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32919#5 mentions a ‘go > build cache’. > > So it seems more of a work-around than a feature to me. i lack here the necessary resolution from the bird's eye view perspective, so let me describe the actual ache that i'm trying to resolve with this: currently, the GO-BUILD-SYSTEM does not reuse build artifacts of the dependencies, only includes them as source. in the current setup, i.e. without SKIP-BUILD?, if i want to import an app with 100+ dependencies, then i need to make sure that all those 100+ dependencies build fine by themselves. this is substantially more work. if there is SKIP-BUILD?, then i can just set it to false in the importer for all the dependencies, and only flip it to true for the leaf packages that i'm actualy trying to build. it seems to me that i should just remove the SKIP-BUILD? assumption from the go importer for now, and file my commits against vanilla master. i'll proceed with that. thanks for the feedback! -- • attila lendvai • PGP: 963F 5D5F 45C7 DFCD 0A39 -- “People do not seem to realize that their opinion of the world is also a confession of character.” — Ralph Waldo Emerson (1803–1882)
Attila Lendvai schreef op do 28-04-2022 om 10:56 [+0000]: > in the current setup, i.e. without SKIP-BUILD?, if i want to import an > app with 100+ dependencies, then i need to make sure that all those > 100+ dependencies build fine by themselves. this is substantially more > work. > > if there is SKIP-BUILD?, then i can just set it to false in the > importer for all the dependencies, and only flip it to true for the > leaf packages that i'm actualy trying to build. Except for long build times due to not reusing build results, I don't follow: if dependency X doesn't build, doesn't that imply that dependent Y won't build either? Conversely, if dependent Y builds, doesn't that imply that the dependents can also be built by theirselves? > it seems to me that i should just remove the SKIP-BUILD? assumption > from the go importer for now, and file my commits against vanilla > master. > > i'll proceed with that. To be clear, my comment was more about the wording (feature / work- around / ...) than about the addition of #:skip-build?. Greetings, Maxime.
> > if there is SKIP-BUILD?, then i can just set it to false in the > > importer for all the dependencies, and only flip it to true for the > > leaf packages that i'm actualy trying to build. > > > Except for long build times due to not reusing build results, I don't > follow: if dependency X doesn't build, doesn't that imply that > dependent Y won't build either? Conversely, if dependent Y builds, > doesn't that imply that the dependents can also be built by > theirselves? i'm afraid i'm stepping beyond my level of knowledge here... but i think this may not be true for golang. and AFAIU, the current GO-BUILD-SYSTEM doesn't reuse any build artifacts. it only arranges the sources of the dependencies in a way that the invoked `go build ...` can find them. > To be clear, my comment was more about the wording (feature / work- > around / ...) than about the addition of #:skip-build?. thanks for clarifying that! -- • attila lendvai • PGP: 963F 5D5F 45C7 DFCD 0A39 -- “Most economic fallacies derive… from the tendency to assume that there is a fixed pie, that one party can gain only at the expense of another.” — Milton Friedman (1912–2006)
Attila Lendvai schreef op do 28-04-2022 om 12:14 [+0000]: > i'm afraid i'm stepping beyond my level of knowledge here... but i > think this may not be true for golang. Barring any evidence to the contrary, I'm going to assume this is not the case, because this tends to be false for other languages. And even if the dependent does build while the dependency doesn't, then either that would cause problems at runtime due to the dependency being broken or the dependency is unused, i.e., it wasn't an actual dependency after all. Attila Lendvai schreef op do 28-04-2022 om 12:14 [+0000]: > and AFAIU, the current GO-BUILD-SYSTEM doesn't reuse any build > artifacts. Currently, it doesn't reuse them, but in the past it did, and maybe in the future it can do again. More generally, the use of #:skip-build? and ‘let's only actually build all the things in the leaf package’ has lead to several problems in Rust: * things that weren't actually dependencies were packaged E.g.: all crates that require an unstable rust compiler. * things that are only required on platforms that Guix doesn't support anyways were packaged (e.g.: winapi, redox, cocoa and foundation crates (e.g.: crates using ‘unstable’ features which cannot be compiled, or crates * cycles (doesn't apply to Go though) * packages with incorrect dependency information, that only happen to work because of how #:skip-build? implies propagation and because of Cargo's dependency resolving algorithms smoothing over them (don't know if this applies to Go). * impossibility of grafting (not relevant to Go, I think Go is too static-library-specific for that?) While maybe not all are 100% caused by #:skip-build? or apply to Go 100%, I don't think these issues should be spread to Go as well, so TBC, ¬LGTM. (I guess this invalid my previous remark: > To be clear, my comment was more about the wording (feature / work- > around / ...) than about the addition of #:skip-build?. ). Greetings, Maxime.
diff --git a/guix/build-system/go.scm b/guix/build-system/go.scm index 5e0e5bbad3..6bcb3656be 100644 --- a/guix/build-system/go.scm +++ b/guix/build-system/go.scm @@ -175,6 +175,7 @@ (define* (go-build name inputs (import-path "") (unpack-path "") (build-flags ''()) + (skip-build? #f) (tests? #t) (allow-go-reference? #f) (system (%current-system)) @@ -205,7 +206,8 @@ (define builder #:import-path #$import-path #:unpack-path #$unpack-path #:build-flags #$build-flags - #:tests? #$tests? + #:skip-build? #$skip-build? + #:tests? #$(and tests? (not skip-build?)) #:allow-go-reference? #$allow-go-reference? #:inputs #$(input-tuples->gexp inputs))))) diff --git a/guix/build/go-build-system.scm b/guix/build/go-build-system.scm index 7f25e05d0d..637d66a6f1 100644 --- a/guix/build/go-build-system.scm +++ b/guix/build/go-build-system.scm @@ -254,22 +254,23 @@ (define (go-inputs inputs) (_ #f)) inputs)))) -(define* (build #:key import-path build-flags #:allow-other-keys) +(define* (build #:key skip-build? import-path build-flags #:allow-other-keys) "Build the package named by IMPORT-PATH." - (with-throw-handler - #t - (lambda _ - (apply invoke "go" "install" - "-v" ; print the name of packages as they are compiled - "-x" ; print each command as it is invoked - ;; Respectively, strip the symbol table and debug - ;; information, and the DWARF symbol table. - "-ldflags=-s -w" - `(,@build-flags ,import-path))) - (lambda (key . args) - (display (string-append "Building '" import-path "' failed.\n" - "Here are the results of `go env`:\n")) - (invoke "go" "env")))) + (or skip-build? + (with-throw-handler + #t + (lambda _ + (apply invoke "go" "install" + "-v" ; print the name of packages as they are compiled + "-x" ; print each command as it is invoked + ;; Respectively, strip the symbol table and debug + ;; information, and the DWARF symbol table. + "-ldflags=-s -w" + `(,@build-flags ,import-path))) + (lambda (key . args) + (display (string-append "Building '" import-path "' failed.\n" + "Here are the results of `go env`:\n")) + (invoke "go" "env"))))) ;; Can this also install commands??? (define* (check #:key tests? import-path #:allow-other-keys)