From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:53530) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hCujE-00081k-Gu for guix-patches@gnu.org; Sat, 06 Apr 2019 19:28:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hCujD-0003mU-11 for guix-patches@gnu.org; Sat, 06 Apr 2019 19:28:04 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:34031) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1hCujC-0003mA-OA for guix-patches@gnu.org; Sat, 06 Apr 2019 19:28:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1hCujC-0001ek-ER for guix-patches@gnu.org; Sat, 06 Apr 2019 19:28:02 -0400 Subject: [bug#35155] [PATCH] build-system/cargo: refactor phases to successfully build In-Reply-To: Resent-Message-ID: From: Chris Marusich References: Date: Sat, 06 Apr 2019 16:27:14 -0700 Message-ID: <87k1g6zpd9.fsf@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Ivan Petkov Cc: 35155@debbugs.gnu.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi Ivan! Ivan Petkov writes: > This patch refactors the cargo-build-system builder code to correctly bui= ld > 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 w= ill > 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 t= hat > 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 c= an > 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 o= pt > + ;; into stricter lints (e.g. #![deny(warnings)]) during their own buil= ds > + ;; 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 =3D '" 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! =2D-=20 Chris --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEy/WXVcvn5+/vGD+x3UCaFdgiRp0FAlypNdIACgkQ3UCaFdgi Rp0rEw//ajG11XXdK0RIUqouPzge4MDgVLhWQS2batuokR5ScCkSyugNrfj4qv64 bt5OC4RH75etQEmJgZAedu2ZApUXVdgxrO5pvnENkuo/qY6f/k7D+JMgHfdaY3T7 X1ydiTUOp4hLixM9CxM7hcYwcbj6UDLmeX5fCynLy7rU4keXR0sqsaCfGmxMpXzg RUneIsfLIfKD2U6aPOeGN3CTtMKpDKx3jUxEE24VedIYLNE7KxbKBjbAg0UN/20q dehiPDkV1Je/5GWKrKGceji3IAT60MksL3ey42E3EalzXaaHCxYhBD1GDb0Zwwud F8HaMD8a+v55KUFD7+hpSZJygpYq3z3RDsxBZYLoHTdu6Y5XTCHjNhLNc0FvXA9l fUP9P0brkvgL0+XKmsQKZEaRYF3+S63Ax5TrAr0ePqbi1bqCH9zxT89nwBmd9uCC 3hz6z4GOrODstRxpUHHDi50/1sI1mZh6R0poN6EY3Y2SyzC84ZYhaldusIeUwmsC VystaFXDtl9wDuwTiS/spBXCcOGzBvis77sT0RWiWxV93jSWfI7MA/qqW0CFr5bf +CmkWh90+rdfEQDIbWSbGHcij3jY8lD3qZHPmj7HUnhUkek7lJYqierFK+sCZC8J NZO2IAk+TAl+kZZSOp6+WewQ6YgtOJ/8tjDjNTthc80Uudgqb9Q= =oCMR -----END PGP SIGNATURE----- --=-=-=--