* [bug#35155] [PATCH] build-system/cargo: refactor phases to successfully build @ 2019-04-05 7:07 Ivan Petkov 2019-04-06 10:32 ` Danny Milosavljevic ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Ivan Petkov @ 2019-04-05 7:07 UTC (permalink / raw) To: 35155; +Cc: Chris Marusich [-- Attachment #1: Type: text/plain, Size: 1552 bytes --] This patch refactors the cargo-build-system builder code to correctly build imported Rust crates. Specifically the changes: * Do not rely on a Cargo.lock presence to determine installation. Cargo will automatically create the file at its first invocation, so instead we check the manifest for any executable targets before attempting an installation * Do not attempt to modify the Cargo.toml file. There are many system specific package in crates.io (e.g. for Windows, Redox, Fuschia, WASM, etc.) and attempting to keep up with what crates must be patched out is futile. * The build phases will honor a skip-build? flag which allows for short-circuiting for optional packages which require nightly features or cannot be built for the current platform. Changes which still need to be done: * Update the host-side code to expand transitive inputs: cargo requires that all transitive crate dependencies are present in its (vendored) index, but doing so by hand in the package definitions will become unwieldy. * Update the host-side code to detect any "circular" dependencies which can result from a naive import Unfortunately there isn't a good way to test this patch at the moment. Importing a non-trivial crate requires a lot of manual resolution, especially with the points above remaining unimplemented. If someone would really like to see the input package definitions I was using to test, I'd be happy to share, though I'd advise that it's pretty hacked up for my own convoluted testing at the moment. —Ivan [-- Attachment #2: 0001-build-system-cargo-refactor-phases-to-successfully-b.patch --] [-- Type: application/octet-stream, Size: 11122 bytes --] From 09c2c9e76e8c9d12bd230d953f9d197efbfb1079 Mon Sep 17 00:00:00 2001 From: Ivan Petkov <ivanppetkov@gmail.com> Date: Tue, 2 Apr 2019 03:02:51 -0700 Subject: [PATCH] build-system/cargo: refactor phases to successfully build * guix/build-system/cargo.scm (%cargo-build-system-modules): Add (json parser) (cargo-build): [vendor-dir]: Define flag and pass it to builder code. [cargo-test-flags]: Likewise. [skip-build?]: Likewise. * guix/build/cargo-build/system.scm (#:use-module): use (json parser) (package-name->crate-name): Delete it (manifest-targets): Add it (has-executable-target?): Add it (configure): Add #:vendor-dir name and use it. Don't touch Cargo.toml. Don't symlink to duplicate inputs. Remove useless registry line from cargo config. Define RUSTFLAGS to lift lint restrictions. (build): Add #:skip-build? flag and use it. (check): Likewise. Add #:cargo-tset-flags and pass it to cargo (install): Factor source logic to install-source. Define #:skip-build? flag and use it. Only install if executable targets are present. (install-source): Copy entire crate directory not just src. [generate-checksums] pass dummy file for unused second argument (%standard-phases): Add install-source phase --- guix/build-system/cargo.scm | 7 ++ guix/build/cargo-build-system.scm | 151 +++++++++++++++++------------- 2 files changed, 93 insertions(+), 65 deletions(-) diff --git a/guix/build-system/cargo.scm b/guix/build-system/cargo.scm index 7ff4e90f71..38290597ee 100644 --- a/guix/build-system/cargo.scm +++ b/guix/build-system/cargo.scm @@ -59,13 +59,17 @@ to NAME and VERSION." (define %cargo-build-system-modules ;; Build-side modules imported by default. `((guix build cargo-build-system) + (json parser) ,@%cargo-utils-modules)) (define* (cargo-build store name inputs #:key (tests? #t) (test-target #f) + (vendor-dir "guix-vendor") (cargo-build-flags ''("--release")) + (cargo-test-flags ''("--release")) + (skip-build? #f) (phases '(@ (guix build cargo-build-system) %standard-phases)) (outputs '("out")) @@ -90,7 +94,10 @@ to NAME and VERSION." source)) #:system ,system #:test-target ,test-target + #:vendor-dir ,vendor-dir #:cargo-build-flags ,cargo-build-flags + #:cargo-test-flags ,cargo-test-flags + #:skip-build? ,skip-build? #:tests? ,tests? #:phases ,phases #:outputs %outputs diff --git a/guix/build/cargo-build-system.scm b/guix/build/cargo-build-system.scm index 20087fa6c4..8616873ce8 100644 --- a/guix/build/cargo-build-system.scm +++ b/guix/build/cargo-build-system.scm @@ -1,6 +1,7 @@ ;;; GNU Guix --- Functional package management for GNU ;;; Copyright © 2016 David Craven <david@craven.ch> ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com> +;;; Copyright © 2019 Ivan Petkov <ivanppetkov@gmail.com> ;;; ;;; This file is part of GNU Guix. ;;; @@ -26,6 +27,7 @@ #:use-module (ice-9 ftw) #:use-module (ice-9 format) #:use-module (ice-9 match) + #:use-module (json parser) #:use-module (srfi srfi-1) #:use-module (srfi srfi-26) #:export (%standard-phases @@ -37,81 +39,88 @@ ;; ;; Code: -;; FIXME: Needs to be parsed from url not package name. -(define (package-name->crate-name name) - "Return the crate name of NAME." - (match (string-split name #\-) - (("rust" rest ...) - (string-join rest "-")) - (_ #f))) +(define (manifest-targets) + "Extract all targets from the Cargo.toml manifest" + (let* ((port (open-input-pipe "cargo read-manifest")) + (data (json->scm port)) + (targets (hash-ref data "targets" '()))) + (close-port port) + targets)) -(define* (configure #:key inputs #:allow-other-keys) +(define (has-executable-target?) + "Check if the current cargo project declares any binary targets." + (let* ((bin? (lambda (kind) (string=? kind "bin"))) + (get-kinds (lambda (dep) (hash-ref dep "kind"))) + (bin-dep? (lambda (dep) (find bin? (get-kinds dep))))) + (find bin-dep? (manifest-targets)))) + +(define* (configure #:key inputs + (vendor-dir "guix-vendor") + #:allow-other-keys) "Replace Cargo.toml [dependencies] section with guix inputs." - ;; Make sure Cargo.toml is writeable when the crate uses git-fetch. - (chmod "Cargo.toml" #o644) (chmod "." #o755) - (if (not (file-exists? "vendor")) - (if (not (file-exists? "Cargo.lock")) - (begin - (substitute* "Cargo.toml" - ((".*32-sys.*") " -") - ((".*winapi.*") " -") - ((".*core-foundation.*") " -")) - ;; Prepare one new directory with all the required dependencies. - ;; It's necessary to do this (instead of just using /gnu/store as the - ;; directory) because we want to hide the libraries in subdirectories - ;; share/rust-source/... instead of polluting the user's profile root. - (mkdir "vendor") - (for-each - (match-lambda - ((name . path) - (let ((crate (package-name->crate-name name))) - (when (and crate path) - (match (string-split (basename path) #\-) - ((_ ... version) - (symlink (string-append path "/share/rust-source") - (string-append "vendor/" (basename path))))))))) - inputs) - ;; Configure cargo to actually use this new directory. - (mkdir-p ".cargo") - (let ((port (open-file ".cargo/config" "w" #:encoding "utf-8"))) - (display " + ;; Prepare one new directory with all the required dependencies. + ;; It's necessary to do this (instead of just using /gnu/store as the + ;; directory) because we want to hide the libraries in subdirectories + ;; share/rust-source/... instead of polluting the user's profile root. + (mkdir-p vendor-dir) + (for-each + (match-lambda + ((name . path) + (let* ((rust-share (string-append path "/share/rust-source")) + (basepath (basename path)) + (link-dir (string-append vendor-dir "/" basepath))) + (and (file-exists? rust-share) + ;; Gracefully handle duplicate inputs + (not (file-exists? link-dir)) + (symlink rust-share link-dir))))) + inputs) + ;; Configure cargo to actually use this new directory. + (mkdir-p ".cargo") + (let ((port (open-file ".cargo/config" "w" #:encoding "utf-8"))) + (display " [source.crates-io] -registry = 'https://github.com/rust-lang/crates.io-index' replace-with = 'vendored-sources' [source.vendored-sources] directory = '" port) - (display (getcwd) port) - (display "/vendor" port) - (display "' + (display (string-append (getcwd) "/" vendor-dir) port) + (display "' " port) - (close-port port))))) - (setenv "CC" (string-append (assoc-ref inputs "gcc") "/bin/gcc")) + (close-port port)) - ;(setenv "CARGO_HOME" "/gnu/store") - ; (setenv "CMAKE_C_COMPILER" cc) + ;; Lift restriction on any lints: a crate author may have decided to opt + ;; into stricter lints (e.g. #![deny(warnings)]) during their own builds + ;; but we don't want any build failures that could be caused later by + ;; upgrading the compiler for example. + (setenv "RUSTFLAGS" "--cap-lints allow") + (setenv "CC" (string-append (assoc-ref inputs "gcc") "/bin/gcc")) #t) -(define* (build #:key (cargo-build-flags '("--release")) +(define* (build #:key + skip-build? + (cargo-build-flags '("--release")) #:allow-other-keys) "Build a given Cargo package." - (zero? (apply system* `("cargo" "build" ,@cargo-build-flags)))) + (or skip-build? + (zero? (apply system* `("cargo" "build" ,@cargo-build-flags))))) -(define* (check #:key tests? #:allow-other-keys) +(define* (check #:key + skip-build? + tests? + (cargo-test-flags '("--release")) + #:allow-other-keys) "Run tests for a given Cargo package." - (if (and tests? (file-exists? "Cargo.lock")) - (zero? (system* "cargo" "test")) - #t)) + (or skip-build? + (and + tests? + (zero? (apply system* `("cargo" "test" ,@cargo-test-flags)))))) (define (touch file-name) (call-with-output-file file-name (const #t))) -(define* (install #:key inputs outputs #:allow-other-keys) - "Install a given Cargo package." +(define* (install-source #:key inputs outputs #:allow-other-keys) + "Install the source for a given Cargo package." (let* ((out (assoc-ref outputs "out")) (src (assoc-ref inputs "source")) (rsrc (string-append (assoc-ref outputs "src") @@ -122,22 +131,34 @@ directory = '" port) ;; Until this changes we are working around this by ;; distributing crates as source and replacing ;; references in Cargo.toml with store paths. - (copy-recursively "src" (string-append rsrc "/src")) + (copy-recursively "." rsrc) (touch (string-append rsrc "/.cargo-ok")) - (generate-checksums rsrc src) + (generate-checksums rsrc "/dev/null") (install-file "Cargo.toml" rsrc) - ;; When the package includes executables we install - ;; it using cargo install. This fails when the crate - ;; doesn't contain an executable. - (if (file-exists? "Cargo.lock") - (zero? (system* "cargo" "install" "--root" out)) - (begin - (mkdir out) - #t)))) + #t)) + +(define* (install #:key inputs outputs skip-build? #:allow-other-keys) + "Install a given Cargo package." + (let* ((out (assoc-ref outputs "out"))) + (mkdir-p out) + + ;; Make cargo reuse all the artifacts we just built instead + ;; of defaulting to making a new temp directory + (setenv "CARGO_TARGET_DIR" "./target") + ;; Force cargo to honor our .cargo/config definitions + ;; https://github.com/rust-lang/cargo/issues/6397 + (setenv "CARGO_HOME" ".") + + ;; Only install crates which include binary targets, + ;; otherwise cargo will raise an error. + (or skip-build? + (not (has-executable-target?)) + (zero? (system* "cargo" "install" "--path" "." "--root" out))))) (define %standard-phases (modify-phases gnu:%standard-phases (delete 'bootstrap) + (add-before 'configure 'install-source install-source) (replace 'configure configure) (replace 'build build) (replace 'check check) -- 2.21.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [bug#35155] [PATCH] build-system/cargo: refactor phases to successfully build 2019-04-05 7:07 [bug#35155] [PATCH] build-system/cargo: refactor phases to successfully build Ivan Petkov @ 2019-04-06 10:32 ` Danny Milosavljevic 2019-04-06 10:41 ` Danny Milosavljevic 2019-04-06 23:27 ` Chris Marusich 2 siblings, 0 replies; 10+ messages in thread From: Danny Milosavljevic @ 2019-04-06 10:32 UTC (permalink / raw) To: Ivan Petkov; +Cc: Chris Marusich, 35155 [-- Attachment #1: Type: text/plain, Size: 1500 bytes --] Hi Ivan, good idea. (Finally the hack with the Cargo.lock is gone :) ) >There are many system specific > package in crates.io (e.g. for Windows, Redox, Fuschia, WASM, etc.) and > attempting to keep up with what crates must be patched out is futile. I agree. > * The build phases will honor a skip-build? flag which allows for > short-circuiting for optional packages which require nightly features or cannot > be built for the current platform. Ok, I guess. > Changes which still need to be done: > * Update the host-side code to expand transitive inputs: cargo requires that > all transitive crate dependencies are present in its (vendored) index, but > doing so by hand in the package definitions will become unwieldy. Yeah. Let's do that in an extra patch. > * Update the host-side code to detect any "circular" dependencies which can > result from a naive import Yeah. > Unfortunately there isn't a good way to test this patch at the moment. > Importing a non-trivial crate requires a lot of manual resolution, especially > with the points above remaining unimplemented. > If someone would really like to see the input package definitions I was using > to test, I'd be happy to share, though I'd advise that it's pretty hacked up > for my own convoluted testing at the moment. No, I can use my own set of hacked-together package definition to test it. (The state of my hacked-together package definitions is not good enough to merge :) ) [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* [bug#35155] [PATCH] build-system/cargo: refactor phases to successfully build 2019-04-05 7:07 [bug#35155] [PATCH] build-system/cargo: refactor phases to successfully build Ivan Petkov 2019-04-06 10:32 ` Danny Milosavljevic @ 2019-04-06 10:41 ` Danny Milosavljevic 2019-04-06 16:12 ` Ivan Petkov 2019-04-06 23:27 ` Chris Marusich 2 siblings, 1 reply; 10+ messages in thread From: Danny Milosavljevic @ 2019-04-06 10:41 UTC (permalink / raw) To: Ivan Petkov; +Cc: Chris Marusich, 35155 [-- Attachment #1: Type: text/plain, Size: 159 bytes --] >+ (generate-checksums rsrc "/dev/null") Hmm, is the "package" entry in the final file ".cargo-checksums.json" useless? Can it be left off completely? [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* [bug#35155] [PATCH] build-system/cargo: refactor phases to successfully build 2019-04-06 10:41 ` Danny Milosavljevic @ 2019-04-06 16:12 ` Ivan Petkov 0 siblings, 0 replies; 10+ messages in thread From: Ivan Petkov @ 2019-04-06 16:12 UTC (permalink / raw) To: Danny Milosavljevic; +Cc: Chris Marusich, 35155 Hi Danny, > On Apr 6, 2019, at 3:41 AM, Danny Milosavljevic <dannym@scratchpost.org> wrote: > >> + (generate-checksums rsrc "/dev/null") > > Hmm, is the "package" entry in the final file ".cargo-checksums.json" useless? > Can it be left off completely? Yes, it appears that cargo expects there to be a “package” hash definition in .cargo-checksum.json, but it doesn’t verify the contents (it only checks if they’ve changed since the last time it updated its Cargo.lock file). Since this procedure is directly used when building rust-1.20, I didn’t want to change its definition and get blocked on bootstrapping all of rustc again. Though I agree, we should update the procedure at some point. —Ivan ^ permalink raw reply [flat|nested] 10+ messages in thread
* [bug#35155] [PATCH] build-system/cargo: refactor phases to successfully build 2019-04-05 7:07 [bug#35155] [PATCH] build-system/cargo: refactor phases to successfully build Ivan Petkov 2019-04-06 10:32 ` Danny Milosavljevic 2019-04-06 10:41 ` Danny Milosavljevic @ 2019-04-06 23:27 ` Chris Marusich 2019-04-07 2:02 ` Ivan Petkov 2 siblings, 1 reply; 10+ messages in thread From: Chris Marusich @ 2019-04-06 23:27 UTC (permalink / raw) To: Ivan Petkov; +Cc: 35155 [-- Attachment #1: Type: text/plain, Size: 4871 bytes --] Hi Ivan! Ivan Petkov <ivanppetkov@gmail.com> writes: > This patch refactors the cargo-build-system builder code to correctly build > imported Rust crates. This is great! Thank you for taking the time to work on it. I have only some high level questions and minor comments. It seems fine to me in general. Since no packages currently use the cargo-build-system, I think we can basically just commit this on master and keep moving forward. Danny, do you agree? > * Do not rely on a Cargo.lock presence to determine installation. Cargo will > automatically create the file at its first invocation, so instead we check > the manifest for any executable targets before attempting an installation Sounds good. > * Do not attempt to modify the Cargo.toml file. There are many system > specific package in crates.io (e.g. for Windows, Redox, Fuschia, WASM, > etc.) and attempting to keep up with what crates must be patched out > is futile. That seems reasonable, too. > * The build phases will honor a skip-build? flag which allows for > short-circuiting for optional packages which require nightly features > or cannot be built for the current platform. Can you elaborate on the motivation for this? Are there situations in which we need to build an optional package, but we aren't actually going to use its build output anywhere? If I'm understanding this correctly, it seems like this new flag would allow us to write a package definition that doesn't actually build a package. If that's the case, I'm not sure why we would bother writing the package definition in the first place. > Changes which still need to be done: > * Update the host-side code to expand transitive inputs: cargo requires that > all transitive crate dependencies are present in its (vendored) index, but > doing so by hand in the package definitions will become unwieldy. > * Update the host-side code to detect any "circular" dependencies which can > result from a naive import I agree with that plan. If I've been following along correctly, once we figure out how to properly make all the transitive crate dependencies (in source form) available in the build environment (and resolve the circular dependencies), it should open the door to importing many Rust packages. > * guix/build-system/cargo.scm (%cargo-build-system-modules): > Add (json parser) Nitpick: You're missing a period here, and in a few more sentences in the ChangeLog entry. > Add #:cargo-tset-flags and pass it to cargo Nitpick: The word "tset" should be "test". > (install): Factor source logic to install-source. > Define #:skip-build? flag and use it. > Only install if executable targets are present. > (install-source): Copy entire crate directory not just src. I'm not as familiar with Rust packaging as you are, so the correctness of this part is not as clear to me. My understanding is that a Cargo package that is a library needs to install its source (so that other Cargo libraries/applications can use it) but not any executables. On the other hand, a Cargo package that is an application needs to install executables (so that a user can run it), but not its source. Is that right? What about Cargo packages that are both libraries and applications? Do those even exist? If you could help me understand (or point me to docs that will help me understand) the model that Rust/Cargo follows here, it would be helpful. > +(define* (configure #:key inputs > + (vendor-dir "guix-vendor") > + #:allow-other-keys) > "Replace Cargo.toml [dependencies] section with guix inputs." Is this docstring still accurate after these changes? > + ;; Lift restriction on any lints: a crate author may have decided to opt > + ;; into stricter lints (e.g. #![deny(warnings)]) during their own builds > + ;; but we don't want any build failures that could be caused later by > + ;; upgrading the compiler for example. > + (setenv "RUSTFLAGS" "--cap-lints allow") Is this necessary? The docs seem to suggest that Cargo always sets it to "allow" anyway: https://doc.rust-lang.org/rustc/lints/levels.html "This feature is used heavily by Cargo; it will pass --cap-lints allow when compiling your dependencies, so that if they have any warnings, they do not pollute the output of your build." > @@ -122,22 +131,34 @@ directory = '" port) > ;; Until this changes we are working around this by > ;; distributing crates as source and replacing > ;; references in Cargo.toml with store paths. Is this comment still accurate? > - (generate-checksums rsrc src) > + (generate-checksums rsrc "/dev/null") This probably deserves a short comment to clarify the intent. Really nice stuff - thank you for sharing it! -- Chris [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* [bug#35155] [PATCH] build-system/cargo: refactor phases to successfully build 2019-04-06 23:27 ` Chris Marusich @ 2019-04-07 2:02 ` Ivan Petkov 2019-04-07 3:05 ` Ivan Petkov 2019-04-07 8:40 ` Chris Marusich 0 siblings, 2 replies; 10+ messages in thread From: Ivan Petkov @ 2019-04-07 2:02 UTC (permalink / raw) To: Chris Marusich; +Cc: 35155 Hi Chris! Thank you for the detailed review! Happy to elaborate on any of your questions, please let me know if you think any of my responses warrant additional code comments. > On Apr 6, 2019, at 4:27 PM, Chris Marusich <cmmarusich@gmail.com> wrote: > >> * The build phases will honor a skip-build? flag which allows for >> short-circuiting for optional packages which require nightly features >> or cannot be built for the current platform. > > Can you elaborate on the motivation for this? Are there situations in > which we need to build an optional package, but we aren't actually going > to use its build output anywhere? This is meant to be an escape hatch for to skip builds when necessary. Nothing is setting this flag right now, but eventually the host-side code may need to set this for certain situations. > If I'm understanding this correctly, it seems like this new flag would > allow us to write a package definition that doesn't actually build a > package. If that's the case, I'm not sure why we would bother writing > the package definition in the first place. That’s an accurate observation. Some context: Cargo requires that all possible transitive dependencies are present in its index/vendor directory. This is so it can deterministically build Cargo.lock files independently of the current platform or what conditional features are enabled. There are several ways to package crates within guix with respect to dependent crates: a) Don’t pull in optional dependencies, or those for unsupported systems, patch out the Cargo.toml file so they’re outright ignored by cargo b) Create stubs for any unsupported/unpacked crates, basically a Cargo.toml definition with the expected crate name/version and no code c) Package all crates depended upon by a crate we’re interested in, possibly annotating it in ways that it isn’t actually built by the CI (e.g. system specific packages which there is no CI support for). As mentioned in my earlier email, I believe option a to be a non-starter since it will be a never-ending uphill battle. I personally believe option c is the best long term approach (we may reserve option b for dire situations). This way if guix is ever ported to other systems (e.g. Linux subsystem for Windows) packages can still be used by consumers without them having to backfill half the crates ecosystem. Given this information, here’s how I anticipate we’ll want to skip doing actual package builds: * If the package is annotated as for a specific platform (e.g. Windows/Redox/Fuscia) the host-side build code can populate (#:skip-build? #t) so it doesn’t fail during CI builds but its still accessible for consuming crates. * If we have a “circular” dependency as part of dev-dependencies (e.g. one crate pulls in an upstream crate during testing to ensure it doesn’t break anything) we’ll need to detangle the dependency graph by rewriting duplicate packages to include the #:skip-build? flag. I can elaborate more on this in a separate email. >> Changes which still need to be done: >> * Update the host-side code to expand transitive inputs: cargo requires that >> all transitive crate dependencies are present in its (vendored) index, but >> doing so by hand in the package definitions will become unwieldy. >> * Update the host-side code to detect any "circular" dependencies which can >> result from a naive import > > I agree with that plan. If I've been following along correctly, once we > figure out how to properly make all the transitive crate dependencies > (in source form) available in the build environment (and resolve the > circular dependencies), it should open the door to importing many Rust > packages. Yes, precisely, it will be much easier to spot an resolve any other bugs or feature gaps once that’s in place. > >> * guix/build-system/cargo.scm (%cargo-build-system-modules): >> Add (json parser) > > Nitpick: You're missing a period here, and in a few more sentences in > the ChangeLog entry. Happy to update. After skimming the contributing guidelines I was left with the impression that commit messages are to only include the concrete changes that were made, and any additional elaboration should be made in code comments. What’s the right process for fixing this? Just send an updated patch to this thread? > >> Add #:cargo-tset-flags and pass it to cargo > > Nitpick: The word "tset" should be "test”. Whoops, that’s what I get for writing commit messages late at night :) > >> (install): Factor source logic to install-source. >> Define #:skip-build? flag and use it. >> Only install if executable targets are present. >> (install-source): Copy entire crate directory not just src. > > I'm not as familiar with Rust packaging as you are, so the correctness > of this part is not as clear to me. > > My understanding is that a Cargo package that is a library needs to > install its source (so that other Cargo libraries/applications can use > it) but not any executables. On the other hand, a Cargo package that is > an application needs to install executables (so that a user can run it), > but not its source. Is that right? You are correct. When building a crate, cargo needs access to the source of all transitive dependencies, but it’s no longer needed after the build. The reason we now copy the entire crate directory (rather than just the src directory) is that some crates have build scripts which usually live outside of `src` and are needed to successfully build. Although the Cargo.toml has the path to the root build script, there are crates (like serde) which have auxiliary build-script modules which aren’t shown in the manifest contents. Rather than muck around and try to guess where the build script code is, we can copy it all and let cargo sort it out. > What about Cargo packages that are > both libraries and applications? Do those even exist? Yes these are a bit rare but they do exist. I don’t have any examples on hand, but you can have something akin to curl which can be used as a binary, as well as imported as a library to other projects. > >> +(define* (configure #:key inputs >> + (vendor-dir "guix-vendor") >> + #:allow-other-keys) >> "Replace Cargo.toml [dependencies] section with guix inputs." > > Is this docstring still accurate after these changes? I think the intent is still accurate, though I’ll tweak this to note vendoring dependencies instead of updating the Cargo.toml. > >> + ;; Lift restriction on any lints: a crate author may have decided to opt >> + ;; into stricter lints (e.g. #![deny(warnings)]) during their own builds >> + ;; but we don't want any build failures that could be caused later by >> + ;; upgrading the compiler for example. >> + (setenv "RUSTFLAGS" "--cap-lints allow") > > Is this necessary? The docs seem to suggest that Cargo always sets it > to "allow" anyway: > > https://doc.rust-lang.org/rustc/lints/levels.html > > "This feature is used heavily by Cargo; it will pass --cap-lints allow > when compiling your dependencies, so that if they have any warnings, > they do not pollute the output of your build.” It’s true that cargo applies this to dependencies, but it doesn’t do this for the top level package that’s currently being built (e.g. if the CI is building some library crate in isolation). As Guix maintainers, we wouldn’t want jobs to start failing because of a new lint cropping up somewhere in between versions. > >> @@ -122,22 +131,34 @@ directory = '" port) >> ;; Until this changes we are working around this by >> ;; distributing crates as source and replacing >> ;; references in Cargo.toml with store paths. > > Is this comment still accurate? I think this is still accurate (modulo Cargo.toml/vendoring word choice which I can tweak). > >> - (generate-checksums rsrc src) >> + (generate-checksums rsrc "/dev/null") > > This probably deserves a short comment to clarify the intent. Do you mean commenting on the intent of `generate-checksums` or the intent of the /dev/null parameter? —Ivan ^ permalink raw reply [flat|nested] 10+ messages in thread
* [bug#35155] [PATCH] build-system/cargo: refactor phases to successfully build 2019-04-07 2:02 ` Ivan Petkov @ 2019-04-07 3:05 ` Ivan Petkov 2019-04-07 8:40 ` Chris Marusich 1 sibling, 0 replies; 10+ messages in thread From: Ivan Petkov @ 2019-04-07 3:05 UTC (permalink / raw) To: Chris Marusich; +Cc: 35155 [-- Attachment #1: Type: text/plain, Size: 680 bytes --] Hi Chris! > On Apr 6, 2019, at 7:02 PM, Ivan Petkov <ivanppetkov@gmail.com> wrote: > >> >>> * guix/build-system/cargo.scm (%cargo-build-system-modules): >>> Add (json parser) >> >> Nitpick: You're missing a period here, and in a few more sentences in >> the ChangeLog entry. > > Happy to update. After skimming the contributing guidelines I was left > with the impression that commit messages are to only include the concrete > changes that were made, and any additional elaboration should be made > in code comments. Please disregard this comment, I had misread your original email. I’ve attached an updated version of my original patch. —Ivan [-- Attachment #2: 0001-build-system-cargo-refactor-phases-to-successfully-b.patch --] [-- Type: application/octet-stream, Size: 11459 bytes --] From 74c59e18e339a71372367291446c93e33a526156 Mon Sep 17 00:00:00 2001 From: Ivan Petkov <ivanppetkov@gmail.com> Date: Tue, 2 Apr 2019 03:02:51 -0700 Subject: [PATCH] build-system/cargo: refactor phases to successfully build * guix/build-system/cargo.scm (%cargo-build-system-modules): Add (json parser). (cargo-build): [vendor-dir]: Define flag and pass it to builder code. [cargo-test-flags]: Likewise. [skip-build?]: Likewise. * guix/build/cargo-build/system.scm (#:use-module): use (json parser). (package-name->crate-name): Delete it. (manifest-targets): Add it. (has-executable-target?): Add it. (configure): Add #:vendor-dir name and use it. Don't touch Cargo.toml. Don't symlink to duplicate inputs. Remove useless registry line from cargo config. Define RUSTFLAGS to lift lint restrictions. (build): Add #:skip-build? flag and use it. (check): Likewise. Add #:cargo-test-flags and pass it to cargo. (install): Factor source logic to install-source. Define #:skip-build? flag and use it. Only install if executable targets are present. (install-source): Copy entire crate directory not just src. [generate-checksums] pass dummy file for unused second argument. (%standard-phases): Add install-source phase. --- guix/build-system/cargo.scm | 9 +- guix/build/cargo-build-system.scm | 155 +++++++++++++++++------------- 2 files changed, 95 insertions(+), 69 deletions(-) diff --git a/guix/build-system/cargo.scm b/guix/build-system/cargo.scm index 7ff4e90f71..dc137421e9 100644 --- a/guix/build-system/cargo.scm +++ b/guix/build-system/cargo.scm @@ -59,13 +59,17 @@ to NAME and VERSION." (define %cargo-build-system-modules ;; Build-side modules imported by default. `((guix build cargo-build-system) + (json parser) ,@%cargo-utils-modules)) (define* (cargo-build store name inputs #:key (tests? #t) (test-target #f) + (vendor-dir "guix-vendor") (cargo-build-flags ''("--release")) + (cargo-test-flags ''("--release")) + (skip-build? #f) (phases '(@ (guix build cargo-build-system) %standard-phases)) (outputs '("out")) @@ -90,8 +94,11 @@ to NAME and VERSION." source)) #:system ,system #:test-target ,test-target + #:vendor-dir ,vendor-dir #:cargo-build-flags ,cargo-build-flags - #:tests? ,tests? + #:cargo-test-flags ,cargo-test-flags + #:skip-build? ,skip-build? + #:tests? ,(and tests? (not skip-build?)) #:phases ,phases #:outputs %outputs #:search-paths ',(map search-path-specification->sexp diff --git a/guix/build/cargo-build-system.scm b/guix/build/cargo-build-system.scm index 20087fa6c4..b68a1f90d2 100644 --- a/guix/build/cargo-build-system.scm +++ b/guix/build/cargo-build-system.scm @@ -1,6 +1,7 @@ ;;; GNU Guix --- Functional package management for GNU ;;; Copyright © 2016 David Craven <david@craven.ch> ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com> +;;; Copyright © 2019 Ivan Petkov <ivanppetkov@gmail.com> ;;; ;;; This file is part of GNU Guix. ;;; @@ -26,6 +27,7 @@ #:use-module (ice-9 ftw) #:use-module (ice-9 format) #:use-module (ice-9 match) + #:use-module (json parser) #:use-module (srfi srfi-1) #:use-module (srfi srfi-26) #:export (%standard-phases @@ -37,81 +39,86 @@ ;; ;; Code: -;; FIXME: Needs to be parsed from url not package name. -(define (package-name->crate-name name) - "Return the crate name of NAME." - (match (string-split name #\-) - (("rust" rest ...) - (string-join rest "-")) - (_ #f))) - -(define* (configure #:key inputs #:allow-other-keys) - "Replace Cargo.toml [dependencies] section with guix inputs." - ;; Make sure Cargo.toml is writeable when the crate uses git-fetch. - (chmod "Cargo.toml" #o644) +(define (manifest-targets) + "Extract all targets from the Cargo.toml manifest" + (let* ((port (open-input-pipe "cargo read-manifest")) + (data (json->scm port)) + (targets (hash-ref data "targets" '()))) + (close-port port) + targets)) + +(define (has-executable-target?) + "Check if the current cargo project declares any binary targets." + (let* ((bin? (lambda (kind) (string=? kind "bin"))) + (get-kinds (lambda (dep) (hash-ref dep "kind"))) + (bin-dep? (lambda (dep) (find bin? (get-kinds dep))))) + (find bin-dep? (manifest-targets)))) + +(define* (configure #:key inputs + (vendor-dir "guix-vendor") + #:allow-other-keys) + "Vendor Cargo.toml dependencies as guix inputs." (chmod "." #o755) - (if (not (file-exists? "vendor")) - (if (not (file-exists? "Cargo.lock")) - (begin - (substitute* "Cargo.toml" - ((".*32-sys.*") " -") - ((".*winapi.*") " -") - ((".*core-foundation.*") " -")) - ;; Prepare one new directory with all the required dependencies. - ;; It's necessary to do this (instead of just using /gnu/store as the - ;; directory) because we want to hide the libraries in subdirectories - ;; share/rust-source/... instead of polluting the user's profile root. - (mkdir "vendor") - (for-each - (match-lambda - ((name . path) - (let ((crate (package-name->crate-name name))) - (when (and crate path) - (match (string-split (basename path) #\-) - ((_ ... version) - (symlink (string-append path "/share/rust-source") - (string-append "vendor/" (basename path))))))))) - inputs) - ;; Configure cargo to actually use this new directory. - (mkdir-p ".cargo") - (let ((port (open-file ".cargo/config" "w" #:encoding "utf-8"))) - (display " + ;; Prepare one new directory with all the required dependencies. + ;; It's necessary to do this (instead of just using /gnu/store as the + ;; directory) because we want to hide the libraries in subdirectories + ;; share/rust-source/... instead of polluting the user's profile root. + (mkdir-p vendor-dir) + (for-each + (match-lambda + ((name . path) + (let* ((rust-share (string-append path "/share/rust-source")) + (basepath (basename path)) + (link-dir (string-append vendor-dir "/" basepath))) + (and (file-exists? rust-share) + ;; Gracefully handle duplicate inputs + (not (file-exists? link-dir)) + (symlink rust-share link-dir))))) + inputs) + ;; Configure cargo to actually use this new directory. + (mkdir-p ".cargo") + (let ((port (open-file ".cargo/config" "w" #:encoding "utf-8"))) + (display " [source.crates-io] -registry = 'https://github.com/rust-lang/crates.io-index' replace-with = 'vendored-sources' [source.vendored-sources] directory = '" port) - (display (getcwd) port) - (display "/vendor" port) - (display "' + (display (string-append (getcwd) "/" vendor-dir) port) + (display "' " port) - (close-port port))))) - (setenv "CC" (string-append (assoc-ref inputs "gcc") "/bin/gcc")) + (close-port port)) - ;(setenv "CARGO_HOME" "/gnu/store") - ; (setenv "CMAKE_C_COMPILER" cc) + ;; Lift restriction on any lints: a crate author may have decided to opt + ;; into stricter lints (e.g. #![deny(warnings)]) during their own builds + ;; but we don't want any build failures that could be caused later by + ;; upgrading the compiler for example. + (setenv "RUSTFLAGS" "--cap-lints allow") + (setenv "CC" (string-append (assoc-ref inputs "gcc") "/bin/gcc")) #t) -(define* (build #:key (cargo-build-flags '("--release")) +(define* (build #:key + skip-build? + (cargo-build-flags '("--release")) #:allow-other-keys) "Build a given Cargo package." - (zero? (apply system* `("cargo" "build" ,@cargo-build-flags)))) + (or skip-build? + (zero? (apply system* `("cargo" "build" ,@cargo-build-flags))))) -(define* (check #:key tests? #:allow-other-keys) +(define* (check #:key + tests? + (cargo-test-flags '("--release")) + #:allow-other-keys) "Run tests for a given Cargo package." - (if (and tests? (file-exists? "Cargo.lock")) - (zero? (system* "cargo" "test")) + (if tests? + (zero? (apply system* `("cargo" "test" ,@cargo-test-flags))) #t)) (define (touch file-name) (call-with-output-file file-name (const #t))) -(define* (install #:key inputs outputs #:allow-other-keys) - "Install a given Cargo package." +(define* (install-source #:key inputs outputs #:allow-other-keys) + "Install the source for a given Cargo package." (let* ((out (assoc-ref outputs "out")) (src (assoc-ref inputs "source")) (rsrc (string-append (assoc-ref outputs "src") @@ -120,24 +127,36 @@ directory = '" port) ;; Rust doesn't have a stable ABI yet. Because of this ;; Cargo doesn't have a search path for binaries yet. ;; Until this changes we are working around this by - ;; distributing crates as source and replacing - ;; references in Cargo.toml with store paths. - (copy-recursively "src" (string-append rsrc "/src")) + ;; vendoring the crates' sources by symlinking them + ;; to store paths. + (copy-recursively "." rsrc) (touch (string-append rsrc "/.cargo-ok")) - (generate-checksums rsrc src) + (generate-checksums rsrc "/dev/null") (install-file "Cargo.toml" rsrc) - ;; When the package includes executables we install - ;; it using cargo install. This fails when the crate - ;; doesn't contain an executable. - (if (file-exists? "Cargo.lock") - (zero? (system* "cargo" "install" "--root" out)) - (begin - (mkdir out) - #t)))) + #t)) + +(define* (install #:key inputs outputs skip-build? #:allow-other-keys) + "Install a given Cargo package." + (let* ((out (assoc-ref outputs "out"))) + (mkdir-p out) + + ;; Make cargo reuse all the artifacts we just built instead + ;; of defaulting to making a new temp directory + (setenv "CARGO_TARGET_DIR" "./target") + ;; Force cargo to honor our .cargo/config definitions + ;; https://github.com/rust-lang/cargo/issues/6397 + (setenv "CARGO_HOME" ".") + + ;; Only install crates which include binary targets, + ;; otherwise cargo will raise an error. + (or skip-build? + (not (has-executable-target?)) + (zero? (system* "cargo" "install" "--path" "." "--root" out))))) (define %standard-phases (modify-phases gnu:%standard-phases (delete 'bootstrap) + (add-before 'configure 'install-source install-source) (replace 'configure configure) (replace 'build build) (replace 'check check) -- 2.21.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [bug#35155] [PATCH] build-system/cargo: refactor phases to successfully build 2019-04-07 2:02 ` Ivan Petkov 2019-04-07 3:05 ` Ivan Petkov @ 2019-04-07 8:40 ` Chris Marusich 2019-04-07 15:49 ` Ivan Petkov 1 sibling, 1 reply; 10+ messages in thread From: Chris Marusich @ 2019-04-07 8:40 UTC (permalink / raw) To: Ivan Petkov; +Cc: 35155 [-- Attachment #1: Type: text/plain, Size: 6858 bytes --] Hi Ivan, Ivan Petkov <ivanppetkov@gmail.com> writes: >> On Apr 6, 2019, at 4:27 PM, Chris Marusich <cmmarusich@gmail.com> wrote: >> >>> * The build phases will honor a skip-build? flag which allows for >>> short-circuiting for optional packages which require nightly features >>> or cannot be built for the current platform. >> >> Can you elaborate on the motivation for this? Are there situations in >> which we need to build an optional package, but we aren't actually going >> to use its build output anywhere? > > This is meant to be an escape hatch for to skip builds when necessary. > Nothing is setting this flag right now, but eventually the host-side code > may need to set this for certain situations. Understood. >> If I'm understanding this correctly, it seems like this new flag would >> allow us to write a package definition that doesn't actually build a >> package. If that's the case, I'm not sure why we would bother writing >> the package definition in the first place. > > That’s an accurate observation. Some context: > > Cargo requires that all possible transitive dependencies are present in > its index/vendor directory. This is so it can deterministically build Cargo.lock > files independently of the current platform or what conditional features are > enabled. Do you mean that if a crate X has an optional feature that requires crate Y, then Cargo requires Y to be present when building X even if X is being built with that feature disabled? > * If we have a “circular” dependency as part of dev-dependencies > (e.g. one crate pulls in an upstream crate during testing to ensure it > doesn’t break anything) we’ll need to detangle the dependency graph by > rewriting duplicate packages to include the #:skip-build? flag. I can > elaborate more on this in a separate email. I think here, you're talking about the situation in which crate A depends on crate B, which in turn depends on crate A's source, and to break the cycle we will replace B's dependency on A with a dependency on A', where A' is effectively just a source build of A (via #:skip-build? #t). Is that basically right? I agree it would be good to discuss the resolution of circular dependencies in another email thread, but I just wanted to double check with you that my basic understanding of your intent is correct. >>> Add #:cargo-tset-flags and pass it to cargo >> >> Nitpick: The word "tset" should be "test”. > > Whoops, that’s what I get for writing commit messages late at night :) It happens to the best of us! :-) > The reason we now copy the entire crate directory (rather than just the src > directory) is that some crates have build scripts which usually live outside > of `src` and are needed to successfully build. Although the Cargo.toml > has the path to the root build script, there are crates (like serde) which > have auxiliary build-script modules which aren’t shown in the manifest > contents. Rather than muck around and try to guess where the build script > code is, we can copy it all and let cargo sort it out. OK, that makes sense. I wasn't exactly sure why we changed this part, but now it makes sense. Thank you for explaining it! >> What about Cargo packages that are >> both libraries and applications? Do those even exist? > > Yes these are a bit rare but they do exist. I don’t have any examples on > hand, but you can have something akin to curl which can be used > as a binary, as well as imported as a library to other projects. In those rare cases, your changes are still good to go, right? We don't actually interact with the Cargo.lock file, and if there are any executables, your code will install them. Something else occurred to me. In packages of C libraries, such as the glibc package, we install libraries (e.g., ".so" files go into the "out" output, and ".a" files go into "static" output). However, here we are not installing any libraries like that. Do all Rust developers just use Cargo.toml files to declare their dependencies, and then let Cargo manage all the actual compiling and linking? Do they ever manually install a Rust library (without using Cargo) when hacking on a project? I may be way off base here, since I'm not (yet!) an expert Rustacean. If I'm confused, please help me to understand. >>> + ;; Lift restriction on any lints: a crate author may have decided to opt >>> + ;; into stricter lints (e.g. #![deny(warnings)]) during their own builds >>> + ;; but we don't want any build failures that could be caused later by >>> + ;; upgrading the compiler for example. >>> + (setenv "RUSTFLAGS" "--cap-lints allow") >> >> Is this necessary? The docs seem to suggest that Cargo always sets it >> to "allow" anyway: >> >> https://doc.rust-lang.org/rustc/lints/levels.html >> >> "This feature is used heavily by Cargo; it will pass --cap-lints allow >> when compiling your dependencies, so that if they have any warnings, >> they do not pollute the output of your build.” > > It’s true that cargo applies this to dependencies, but it doesn’t do this > for the top level package that’s currently being built (e.g. if the CI is > building some library crate in isolation). As Guix maintainers, we > wouldn’t want jobs to start failing because of a new lint cropping up > somewhere in between versions. Ah, I see. Then yes, I agree: we should set it. >>> - (generate-checksums rsrc src) >>> + (generate-checksums rsrc "/dev/null") >> >> This probably deserves a short comment to clarify the intent. > > Do you mean commenting on the intent of `generate-checksums` > or the intent of the /dev/null parameter? I mean the "/dev/null" argument, mainly. As far as I can tell, it looks like the generate-checksums procedure builds a file with checksums to satisfy some requirement of the cargo tool. That seems reasonable, but I'm not sure why we use "/dev/null" here. Finally, two more minor comments about code style which I don't think you need to change but are good to know going forward: - Instead of "system*", we prefer to use "invoke" (defined in (guix build utils)) whenever possible. It throws an exception when an error occurs, so it's harder to accidentally ignore errors. - Using "and" and "or" statements for control flow is OK, especially since you're using "system*". In fact, we do this in other parts of Guix, too. However, I personally feel that forms like "if", "when", and "unless" are clearer in some cases and should probably be preferred, especially when using "invoke" instead of "system*". That said, I think it's fine as is. Unless Danny (or someone else) has some more comments, I'll merge your changes in the next few days. -- Chris [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* [bug#35155] [PATCH] build-system/cargo: refactor phases to successfully build 2019-04-07 8:40 ` Chris Marusich @ 2019-04-07 15:49 ` Ivan Petkov [not found] ` <87mukz8p3g.fsf@garuda.local.i-did-not-set--mail-host-address--so-tickle-me> 0 siblings, 1 reply; 10+ messages in thread From: Ivan Petkov @ 2019-04-07 15:49 UTC (permalink / raw) To: Chris Marusich; +Cc: 35155 [-- Attachment #1: Type: text/plain, Size: 5837 bytes --] Hi Chris, > On Apr 7, 2019, at 1:40 AM, Chris Marusich <cmmarusich@gmail.com> wrote: > > Do you mean that if a crate X has an optional feature that requires > crate Y, then Cargo requires Y to be present when building X even if X > is being built with that feature disabled? Correct, the source to crate Y must be present when crate X is being built, transitively, or otherwise. >> * If we have a “circular” dependency as part of dev-dependencies >> (e.g. one crate pulls in an upstream crate during testing to ensure it >> doesn’t break anything) we’ll need to detangle the dependency graph by >> rewriting duplicate packages to include the #:skip-build? flag. I can >> elaborate more on this in a separate email. > > I think here, you're talking about the situation in which crate A > depends on crate B, which in turn depends on crate A's source, and to > break the cycle we will replace B's dependency on A with a dependency on > A', where A' is effectively just a source build of A (via #:skip-build? > #t). Is that basically right? > > I agree it would be good to discuss the resolution of circular > dependencies in another email thread, but I just wanted to double check > with you that my basic understanding of your intent is correct. Yep, your understanding of the situation and potential solution is correct. I’ll send out a different email thread around this since we might need some small tweaks to the overall build system to support this. >>> What about Cargo packages that are >>> both libraries and applications? Do those even exist? >> >> Yes these are a bit rare but they do exist. I don’t have any examples on >> hand, but you can have something akin to curl which can be used >> as a binary, as well as imported as a library to other projects. > > In those rare cases, your changes are still good to go, right? We don't > actually interact with the Cargo.lock file, and if there are any > executables, your code will install them. Yep, there’s no harm in building the “src” output of an application crate (outside of using up some store space) in case something else wants to depend on it. > Something else occurred to me. In packages of C libraries, such as the > glibc package, we install libraries (e.g., ".so" files go into the "out" > output, and ".a" files go into "static" output). However, here we are > not installing any libraries like that. Do all Rust developers just use > Cargo.toml files to declare their dependencies, and then let Cargo > manage all the actual compiling and linking? Do they ever manually > install a Rust library (without using Cargo) when hacking on a project? In general, cargo is used to perform all building an linking of the final rust outputs (binaries, shared libraries, and other rust artifacts). Cargo also supports defining arbitrary build scripts such as building some other non-rust dependency which is to be linked in the final outputs. https://doc.rust-lang.org/cargo/reference/build-scripts.html <https://doc.rust-lang.org/cargo/reference/build-scripts.html> However, cargo does not perform the actual distribution of these “external” dependencies. Crates may vendor their own external sources, or they may expect them to be present in the usual places, or they may support an environment variable which points them in the right direction. One example is the `jemalloc-sys` crate. If built with an environment variable pointing to a compiled version of jemalloc, it will build rust bindings which link to that binary. Otherwise it will build its own vendored version of the jemalloc source. Handling these packages will need to be done on a case-by-case basis with some additional setup glue in the package definitions, as necessary. >>>> - (generate-checksums rsrc src) >>>> + (generate-checksums rsrc "/dev/null") >>> >>> This probably deserves a short comment to clarify the intent. >> >> Do you mean commenting on the intent of `generate-checksums` >> or the intent of the /dev/null parameter? > > I mean the "/dev/null" argument, mainly. As far as I can tell, it looks > like the generate-checksums procedure builds a file with checksums to > satisfy some requirement of the cargo tool. That seems reasonable, but > I'm not sure why we use "/dev/null" here. Cargo expects the checksum package to include a checksum of each individual file as well as a checksum for the entire directory. The `generate-checksums` procedure doesn’t correctly handle the second parameter being a directory and raises an error. Luckily, cargo doesn’t seem to care about the contents of this checksum, as long as the declaration is there. I didn’t want to change the `generate-checksums` procedure just yet since it’s used during building of rust-1.20, and doing so will require a full bootstrap of the compiler chain, which would have blocked me for a while. > Finally, two more minor comments about code style which I don't think > you need to change but are good to know going forward: > > - Instead of "system*", we prefer to use "invoke" (defined in (guix > build utils)) whenever possible. It throws an exception when an error > occurs, so it's harder to accidentally ignore errors. > > - Using "and" and "or" statements for control flow is OK, especially > since you're using "system*". In fact, we do this in other parts of > Guix, too. However, I personally feel that forms like "if", "when", > and "unless" are clearer in some cases and should probably be > preferred, especially when using "invoke" instead of "system*”. Thank you for the tips, I’ll keep these in mind going forward. Still pretty new to guile, its APIs, and guix’s utilities on top of that! :) —Ivan [-- Attachment #2: Type: text/html, Size: 8215 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <87mukz8p3g.fsf@garuda.local.i-did-not-set--mail-host-address--so-tickle-me>]
* bug#35155: [PATCH] build-system/cargo: refactor phases to successfully build [not found] ` <87mukz8p3g.fsf@garuda.local.i-did-not-set--mail-host-address--so-tickle-me> @ 2019-04-09 16:01 ` Ivan Petkov 0 siblings, 0 replies; 10+ messages in thread From: Ivan Petkov @ 2019-04-09 16:01 UTC (permalink / raw) To: Chris Marusich; +Cc: 35155-done Thank you Chris! —Ivan > On Apr 9, 2019, at 3:12 AM, Chris Marusich <cmmarusich@gmail.com> wrote: > > Hi Ivan, > > I've committed this as 1d3acde5087d50af6a4901fd7614f0940eb7b41d on > master. Thank you! > > -- > Chris ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-04-09 16:02 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-04-05 7:07 [bug#35155] [PATCH] build-system/cargo: refactor phases to successfully build Ivan Petkov 2019-04-06 10:32 ` Danny Milosavljevic 2019-04-06 10:41 ` Danny Milosavljevic 2019-04-06 16:12 ` Ivan Petkov 2019-04-06 23:27 ` Chris Marusich 2019-04-07 2:02 ` Ivan Petkov 2019-04-07 3:05 ` Ivan Petkov 2019-04-07 8:40 ` Chris Marusich 2019-04-07 15:49 ` Ivan Petkov [not found] ` <87mukz8p3g.fsf@garuda.local.i-did-not-set--mail-host-address--so-tickle-me> 2019-04-09 16:01 ` bug#35155: " Ivan Petkov
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/guix.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).