[PATCH] Update cargo-build-system to expand package inputs

  • Done
  • quality assurance status badge
Details
4 participants
  • Chris Marusich
  • Danny Milosavljevic
  • Ivan Petkov
  • Ludovic Courtès
Owner
unassigned
Submitted by
Ivan Petkov
Severity
normal
I
I
Ivan Petkov wrote on 19 Apr 2019 07:34
(address . guix-patches@gnu.org)
E6292059-4F4A-4360-BF82-CB4674A997E5@gmail.com
When building, cargo requires the source of all transitive dependencies to be present in its index.
Rather than force package definitions to list out all transitive inputs, the build system will
automatically expand the inputs as necessary.

Because it is possible for crates to have apparent cycles in their native dependencies,
the build system must take some measures to work around potential cycles. This patch series
takes an initial naive stab at resolving the problem, by never building native-inputs.

I plan on revisiting this sometime soon and making the system a bit more intelligent
(namely building native-inputs where possible and breaking any cycles). But for now this should
unblock working on package definitions.

I’ve also included three rust crates as a proof of concept that the system works and it handles
potential native-input cycles. These crates do nothing on their own, but they’re heavily depended
upon by the rest of the crates ecosystem, so they will eventually be useful to have in guix.

—Ivan

I
I
Ivan Petkov wrote on 4 May 2019 18:40
(address . 35318@debbugs.gnu.org)
AE1F9948-6987-44C0-8F4F-EC16C7465B2F@gmail.com
Friendly ping!

Thanks,
—Ivan
D
D
Danny Milosavljevic wrote on 4 May 2019 20:31
20190504203123.2af2049f@scratchpost.org
Hi Ludo,
Hi Ivan,

@Ludo:

Could you take a look at patch 1? It's allowing the lookup of transitive
dependencies in Guix to be more flexible. Is it OK?

It's used in patch 2 in order to consider both inputs and propagated inputs
rather than just propagated inputs.

@Ivan:

Thanks! I've tested it and it works.

But I don't understand yet why you change the role of "inputs" compared
to how it is in the rest of Guix.

You have this:

+(define-public rust-proc-macro2
+ (package
[...]
+ (build-system cargo-build-system)
+ (native-inputs
+ `(("rust-quote" ,rust-quote "src")))
+ (inputs
+ `(("rust-unicode-xid" ,rust-unicode-xid "src")))
[...]

Here, inputs refer to SOURCE parts of packages which are definitely not
referred to at runtime. Does "guix gc --references ...rust-proc-macro2..."
really refer to the source of rust-unicode-xid ? I checked, it doesn't,
neither for the "src" derivation nor for the "out" derivation.

I think the general approach is good but I'm not certain that this won't
break other parts of Guix. If it doesn't, fine. @Ludo: WDYT?

Details:

A Rust crate has dependencies and dev-dependencies.

The crate needs the dev-dependencies only when building, not at runtime.

Let "transitives of X" mean "X and transitives of immediate dependencies of X and
transitives of immediate dev-dependencies of X", recursively.

The crate needs the source code of all its transitives to be available when
building, but needs none of the source code at runtime.

A crate at run time only requires the immediate, if even that (probably not!),
dependencies and none of the dev-dependencies, and not as source code.

So it's really not a propagated-input, although it kinda seems like a weird
version of a propagated-input while building (something like a
native-propagated-input).

If this can't be generalized (and I'm not sure of that--Go has a similar
static library-y view), we could also do those as (arguments ...) for the
rust build system only--although not sure how to do the resolving of
transitives then.
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAlzN2nsACgkQ5xo1VCww
uqXiQAgAnsh6D7ZoDv+8JU2L3Tvi484coVCFAHiHHV/2Shcy+I8O3jvgnibnToFA
Nt9n1jZ/LgI33ikjAU/MsdCebS5EUmfiguQj8UHv7eem9kw3CPAHmZE9dAoYFs/P
Kz5LZaQWmpCV6vonQco5DHHTiS/EKmDgS4AWNLqeZv42/pP3BHYG0Sy/w4qIDYm5
emni8sEnsJzSFG1qhvyV/i3Ywg07VqmNRh7FRgFFCUHb+pf5V375L6OaHT8+0fsX
gGxNBKfWa7e+9qRkz2fBCyuFwZtysPbixkjzKYAg3GXS47Bk9euZ2AKYaiRed7OU
tnFno5v+ZkKt3XYMXnfP7BIkLKXKrw==
=oYVS
-----END PGP SIGNATURE-----


I
I
Ivan Petkov wrote on 4 May 2019 23:09
(name . Danny Milosavljevic)(address . dannym@scratchpost.org)
0222B4A7-FFCD-42C3-B8EA-2CD933F1ADA4@gmail.com
Hi Danny,

Thanks for the feedback!

Toggle quote (29 lines)
> On May 4, 2019, at 11:31 AM, Danny Milosavljevic <dannym@scratchpost.org> wrote:
>
> @Ivan:
>
> Thanks! I've tested it and it works.
>
> But I don't understand yet why you change the role of "inputs" compared
> to how it is in the rest of Guix.
>
> You have this:
>
> +(define-public rust-proc-macro2
> + (package
> [...]
> + (build-system cargo-build-system)
> + (native-inputs
> + `(("rust-quote" ,rust-quote "src")))
> + (inputs
> + `(("rust-unicode-xid" ,rust-unicode-xid "src")))
> [...]
>
> Here, inputs refer to SOURCE parts of packages which are definitely not
> referred to at runtime. Does "guix gc --references ...rust-proc-macro2..."
> really refer to the source of rust-unicode-xid ? I checked, it doesn't,
> neither for the "src" derivation nor for the "out" derivation.
>
> I think the general approach is good but I'm not certain that this won't
> break other parts of Guix. If it doesn't, fine. @Ludo: WDYT?

To my understanding, Guix only needs the inputs and native-inputs to be present
in the store during build time, and only propagated-inputs need to be present
in the store during runtime. Since cargo crates don’t need the source present
at runtime, propagated-inputs seemed inappropriate to me.

Pardon my ignorance on Guix, but what do you mean by “changing the role
of inputs”? Unless by this you mean changing the semantics of “expanding”
the inputs, then yes this is a departure from the existing usage. In my mind,
I want the Guix package definition to mirror the cargo one as it would be a
nightmare to maintain a list of the expanded transitive packages in each
definition by hand.

Toggle quote (24 lines)
> Details:
>
> A Rust crate has dependencies and dev-dependencies.
>
> The crate needs the dev-dependencies only when building, not at runtime.
>
> Let "transitives of X" mean "X and transitives of immediate dependencies of X and
> transitives of immediate dev-dependencies of X", recursively.
>
> The crate needs the source code of all its transitives to be available when
> building, but needs none of the source code at runtime.
>
> A crate at run time only requires the immediate, if even that (probably not!),
> dependencies and none of the dev-dependencies, and not as source code.
>
> So it's really not a propagated-input, although it kinda seems like a weird
> version of a propagated-input while building (something like a
> native-propagated-input).
>
> If this can't be generalized (and I'm not sure of that--Go has a similar
> static library-y view), we could also do those as (arguments ...) for the
> rust build system only--although not sure how to do the resolving of
> transitives then.

If this needs to be it’s own Guix concept, perhaps it would be along the lines
of propagated-native-inputs?

I opted to make the cargo-build-system perform the work of the transitive
lookups since I wasn’t sure if this would truly be a generalized feature…

—Ivan
L
L
Ludovic Courtès wrote on 6 May 2019 10:00
Re: [bug#35318] [PATCH] Update cargo-build-system to expand package inputs
(name . Ivan Petkov)(address . ivanppetkov@gmail.com)
87ftpsnhal.fsf@gnu.org
Hello,

Ivan Petkov <ivanppetkov@gmail.com> skribis:

Toggle quote (10 lines)
> From ca6dfd9451f22c4d6dc02aa7eceee0c35800dd57 Mon Sep 17 00:00:00 2001
> From: Ivan Petkov <ivanppetkov@gmail.com>
> Date: Tue, 16 Apr 2019 03:32:44 -0700
> Subject: [PATCH 1/4] packages: allow dynamic input closure computation
>
> * guix/packages: (transitive-inputs): Rename to
> package-transitive-dependencies.
> (package-transitive-dependencies): Add proc parameter and use it.
> (transitive-inputs): Add it.

There’s nothing written in terms of “dependencies”; instead everything
is written in terms of “inputs”, so I’d like to remain consistent here.

If we need something more specific, I’d rather see it as a private
procedure in (guix build-system cargo-build-system).

Danny wrote:

Toggle quote (3 lines)
> It's used in patch 2 in order to consider both inputs and propagated inputs
> rather than just propagated inputs.

I’m not sure I want to know the details :-), but it seems to be what
‘package-transitive-inputs’ does, no?

(define (package-transitive-inputs package)
"Return the transitive inputs of PACKAGE---i.e., its direct inputs along
with their propagated inputs, recursively."
(transitive-inputs (package-direct-inputs package)))

Do you have an example of a package where this is not enough?

Thanks,
Ludo’.
I
I
Ivan Petkov wrote on 6 May 2019 18:04
(name . Ludovic Courtès)(address . ludo@gnu.org)
2C03880B-F90E-4949-9637-DC918B6D40A0@gmail.com
Hi Ludo,

Toggle quote (5 lines)
> On May 6, 2019, at 1:00 AM, Ludovic Courtès <ludo@gnu.org> wrote:
>
> There’s nothing written in terms of “dependencies”; instead everything
> is written in terms of “inputs”, so I’d like to remain consistent here.

I admit, I wasn’t feeling very inspired with coming up with a name, but I wanted
to avoid confusing it with the existing package-transitive-inputs procedure.

Toggle quote (3 lines)
> If we need something more specific, I’d rather see it as a private
> procedure in (guix build-system cargo-build-system).

I wanted to reuse the core traversal/memoization that was defined in the
original package-transitive-inputs procedure, but I can fork the implementation
as a private procedure in the cargo-build-system for now.

Toggle quote (3 lines)
> I’m not sure I want to know the details :-), but it seems to be what
> ‘package-transitive-inputs’ does, no?

package-transitive-inputs captures all transitive propagated-inputs, but the
cargo-build-system needs all transitive propagated-inputs and regular inputs
as well (modeling cargo dependencies as propagated-inputs does not seem
appropriate).

Thanks,
—Ivan
I
I
I
Ivan Petkov wrote on 15 May 2019 08:08
(address . 35318@debbugs.gnu.org)
D24AE813-AA31-4C08-9909-264F2DA41444@gmail.com
Hi everyone,

Chris and I had a very productive discussion around this patch series this
evening. We discussed an alternative approach to allowing the cargo-build-system
to capture all transitive Rust crate sources without changing the established
semantics around Guix inputs and native-inputs.

The short summary is introducing crates as new arguments in the package
definition. These arguments will be expanded to include the sources of any
transitive sources when lowered to a derivation, while preserving any
other Guix inputs/native-inputs the package may wish to include.

I'll be sending an updated patch series here once I get a chance to work on
this, and I’ll elaborate on the solution with more specifics then!

Thanks,
--Ivan
L
L
Ludovic Courtès wrote on 15 May 2019 14:44
(name . Ivan Petkov)(address . ivanppetkov@gmail.com)
87pnojvqdu.fsf@gnu.org
Hello!

Ivan Petkov <ivanppetkov@gmail.com> skribis:

Toggle quote (13 lines)
> Chris and I had a very productive discussion around this patch series this
> evening. We discussed an alternative approach to allowing the cargo-build-system
> to capture all transitive Rust crate sources without changing the established
> semantics around Guix inputs and native-inputs.
>
> The short summary is introducing crates as new arguments in the package
> definition. These arguments will be expanded to include the sources of any
> transitive sources when lowered to a derivation, while preserving any
> other Guix inputs/native-inputs the package may wish to include.
>
> I'll be sending an updated patch series here once I get a chance to work on
> this, and I’ll elaborate on the solution with more specifics then!

OK, sounds good, thanks for the update!

And sorry for not being more responsive.

Ludo’.
I
I
Ivan Petkov wrote on 20 May 2019 03:00
(name . Ludovic Courtès)(address . ludo@gnu.org)
6A0A0108-A722-4D73-85B7-E61AB8230026@gmail.com
Hi everyone!

I’ve updated this patch series. The cargo-build-system will now expect any
crate dependencies to be listed as arguments. Specifically, regular cargo
dependencies should be specified via the #:cargo-deps parameter, and any cargo
dev-dependnecies should be specified via the #:cargo-dev-deps parameter.

The cargo-build-system will traverse any inputs specified in those parameters,
and any inputs they may have in their #:cargo-deps parameter as well,
extracting their package sources and adding them as native-inputs to the
current bag being built. This avoids having to define new semantics for package
inputs/native-inputs for expanding all transitive sources.

There are several implications of this decision:
* Building a package definition does not require actually building/checking
any dependent crates. This can be a benefits:
- For example, sometimes a crate may have an optional dependency on some OS
specific package which cannot be built or run on the current system. This
approach means that the build will not fail if cargo ends up internally ignoring
the dependency.
- It avoids waiting for quadratic builds from source: cargo always builds
dependencies within the current workspace. This is largely due to Rust not
having a stable ABI and other resolutions that cargo applies. This means that
if we have a depencency chain of X -> Y -> Z and we build each definition
independently the following will happen:
* Cargo will build and test crate Z
* Cargo will build crate Z in Y's workspace, then build and test Y
* Cargo will build crates Y and Z in X's workspace, then build and test X
* But there are also some downsides with this approach:
- If a dependent crate is subtly broken on the system (i.e. it builds but its
tests fail) the consuming crates may build and test successfully but
actually fail during normal usage (however, the CI will still build all
packages which will give visibility in case packages suddenly break).
- Because crates aren't declared as regular inputs, other Guix facilities
such as tracking package graphs may not work by default (however, this is
something that can always be extended or reworked in the future).

Please let me know if anything is unclear, I’m happy to elaborate if needed!

Thanks,
—Ivan
L
L
Ludovic Courtès wrote on 20 May 2019 21:38
(name . Ivan Petkov)(address . ivanppetkov@gmail.com)
87pnocq5kz.fsf@gnu.org
Hi Ivan!

Ivan Petkov <ivanppetkov@gmail.com> skribis:

Toggle quote (2 lines)
> Please let me know if anything is unclear, I’m happy to elaborate if needed!

I’ll only comment superficially because I haven’t followed the rest of
the discussion and I know too little about Rust; hopefully Danny and
Chris can provide feedback.

Toggle quote (13 lines)
> From 5457f60036ce1354b4b89b9c3c423cca14e3a777 Mon Sep 17 00:00:00 2001
> From: Ivan Petkov <ivanppetkov@gmail.com>
> Date: Tue, 16 Apr 2019 03:37:44 -0700
> Subject: [PATCH 1/8] build-system/cargo: expand transitive crate sources
>
> * guix/build/cargo: (package-cargo-deps): Add it.
> (package-cargo-dev-deps): Add it.
> (cargo-transitive-deps): Add it.
> (expand-crate-sources): Add it.
> (lower): New cargo-deps nd cargo-dev-deps keywords.
> Use expand-crate-sources.
> (private-keywords): Add new keywords.

[...]

Toggle quote (6 lines)
> +(define (package-cargo-deps p)
> + (apply
> + (lambda* (#:key (cargo-deps '()) #:allow-other-keys)
> + cargo-deps)
> + (package-arguments p)))

It’s surprising style. It seems redundant with the ‘inputs’ field, but
IIUC, the main difference here is that you can simply name dependencies,
even if there’s no Guix package for it, right?

Toggle quote (6 lines)
> +(define (package-cargo-dev-deps p)
> + (apply
> + (lambda* (#:key (cargo-dev-deps '()) #:allow-other-keys)
> + cargo-dev-deps)
> + (package-arguments p)))

As a rule of thumb, please avoid abbreviations in identifiers (info
"(guix) Coding Style"). So that would be
‘package-development-dependencies’ or something like that.

Toggle quote (12 lines)
> +(define (crate-transitive-deps inputs)
> + "Return the closure of INPUTS when considering the 'cargo-deps' and
> +'cargod-dev-deps' edges. Omit duplicate inputs, except for those
> +already present in INPUTS itself.
> +
> +This is implemented as a breadth-first traversal such that INPUTS is
> +preserved, and only duplicate extracted inputs are removed.
> +
> +Forked from ((guix packages) transitive-inputs) since this extraction
> +uses slightly different rules compared to the rest of Guix (i.e. we
> +do not extract the conventional inputs)."

Perhaps call it ‘crate-closure’?

Toggle quote (4 lines)
> +(define (expand-crate-sources cargo-deps cargo-dev-deps)
> + "Extract all transitive sources for CARGO-DEPS and CARGO-DEV-DEPS along their
> +'cargo-deps' edges.

Maybe s/cargo-deps/inputs/ and s/cargo-dev-deps/development-inputs/?

I’d prefer to stick to the same terminology as in the rest of the code
if we’re talking about the same sort of input lists.

That’s it. :-)

Thank you for improving Rust support!

Ludo’.
I
I
Ivan Petkov wrote on 22 May 2019 04:48
(name . Ludovic Courtès)(address . ludo@gnu.org)
43DF2996-1DEA-40CD-92FC-B27433F63AE7@gmail.com
Hi Ludo!

Toggle quote (12 lines)
> On May 20, 2019, at 12:38 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>
>> +(define (package-cargo-deps p)
>> + (apply
>> + (lambda* (#:key (cargo-deps '()) #:allow-other-keys)
>> + cargo-deps)
>> + (package-arguments p)))
>
> It’s surprising style. It seems redundant with the ‘inputs’ field, but
> IIUC, the main difference here is that you can simply name dependencies,
> even if there’s no Guix package for it, right?

That’s one benefit, the other is that we’re defining our own new semantics
on the cargo-specific inputs here to be treated like propagated-inputs, but
without actually making the store install them when a Rust binary is substituted.

Toggle quote (10 lines)
>> +(define (package-cargo-dev-deps p)
>> + (apply
>> + (lambda* (#:key (cargo-dev-deps '()) #:allow-other-keys)
>> + cargo-dev-deps)
>> + (package-arguments p)))
>
> As a rule of thumb, please avoid abbreviations in identifiers (info
> "(guix) Coding Style"). So that would be
> ‘package-development-dependencies’ or something like that.

Thanks for the tip, I’ll update these names.

Since the actual cargo documentation actually refers to “dev-dependencies”
do you think it’s better to use “cargo-dev-dependencies” (for consistency that
Rust programmers might be used to), or stick with “cargo-development-dependencies”
(for Guix consistencies)?

Toggle quote (14 lines)
>> +(define (crate-transitive-deps inputs)
>> + "Return the closure of INPUTS when considering the 'cargo-deps' and
>> +'cargod-dev-deps' edges. Omit duplicate inputs, except for those
>> +already present in INPUTS itself.
>> +
>> +This is implemented as a breadth-first traversal such that INPUTS is
>> +preserved, and only duplicate extracted inputs are removed.
>> +
>> +Forked from ((guix packages) transitive-inputs) since this extraction
>> +uses slightly different rules compared to the rest of Guix (i.e. we
>> +do not extract the conventional inputs)."
>
> Perhaps call it ‘crate-closure’?

Sure that works, I’ll rename this!

Toggle quote (9 lines)
>> +(define (expand-crate-sources cargo-deps cargo-dev-deps)
>> + "Extract all transitive sources for CARGO-DEPS and CARGO-DEV-DEPS along their
>> +'cargo-deps' edges.
>
> Maybe s/cargo-deps/inputs/ and s/cargo-dev-deps/development-inputs/?
>
> I’d prefer to stick to the same terminology as in the rest of the code
> if we’re talking about the same sort of input lists.

I can rename this as well.

Toggle quote (5 lines)
>
> That’s it. :-)
>
> Thank you for improving Rust support!

Happy to help :)

—Ivan
C
C
Chris Marusich wrote on 8 Jun 2019 20:44
(name . Ivan Petkov)(address . ivanppetkov@gmail.com)
87ftoj9as8.fsf@gmail.com
Hi Ivan,

Sorry for taking so long to review this. In short, I think these
changes are good, and if nobody has more feedback in the next few days,
I will merge it and we can see how it works in practice!

At a high level, I think these changes have the following pros and cons:

Pros:

- We can now build Rust crates. Fantastic!
- The "dependencies" and "dev-dependencies" terminology is the same
as Cargo uses, so it will be familiar to Rust programmers.
- We avoid O(N^2) build time, where N is the number of dependencies.
- We can use the importer to quickly import new crates and iterate.

Cons:

- Because the "dependencies" and "dev-dependencies" are specified as
package arguments instead of any kind of "input", they won't show
up in some of the graphs produced by "guix package". However, in
theory "guix graph" could be taught to display nodes for crate
"dependencies" and "dev-dependencies", too.
- Everyone who defines a Guix package for a crate must make sure the
origin's file name ends in ".cargo", since the cargo-build-system
now assumes that any input ending in ".cargo" is a crate that
should be extracted into the build's crate-dir. This is a little
brittle, and I wish we had a better way to check this, but I can't
think of a better way at the moment. Since you've updated the
importer to always add this, it probably won't be much of an issue
in practice, since that's the default way new crates will be added
to Guix. Going forward, maybe we can avoid this by just checking
the inputs to see which ones are gzipped tarballs containing
Cargo.toml files.

Limitations imposed by Rust/Cargo itself:

- I'm not a Rust or Cargo expert, but my current understanding is
that it isn't feasible to save the artifacts produced when
building a crate for re-use when building another crate. In the
world of C, it is common to produce a library, and then link
against that library when building other software later. In Guix,
when building a C library, we install the built artifacts (e.g.,
.so files) into the output, so that those artifacts can be used as
the input for another package's build. It seems that, by Cargo's
design, it isn't currently feasible to do the same sort of thing
with Cargo: that is, it isn't feasible to build artifacts, install
them somewhere for later use, and then later re-use them in
another Cargo build. I'd be glad to learn that I'm mistaken, but
currently that is my understanding.

- Related to this, I doubt that a Rust programmer will be able to
invoke a command like "guix environment my-crate" (even if we
teach it to understand crate "dependencies" and
"dev-dependencies") to make all the dependencies required to build
my-crate available. If a Rust programmer wants to hack on
my-crate, they'll probably still just use "cargo" to do it without
using Guix at all. Is there any way to avoid this and make it
possible to get the dependencies used by Guix in the build, so
that a Rust programmer can hack around using precisely those
dependencies? If this were C or Python, you could do that using
"guix environment," but I'm not sure how this could work with Rust
crates.

Thanks again for working on this!

--
Chris
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEy/WXVcvn5+/vGD+x3UCaFdgiRp0FAlz8AicACgkQ3UCaFdgi
Rp0zWg/+Kp3+TU4z4vQUW6gczOYeguf2hNuDl1n6F77mhOg0SbjowkylP8gOldZo
9oBE2ErU7nYuZuWgeJSDI1ux/MLLpgOnz7nle4oE9lZNxJnrjhsoqOh+lXoFJ3b8
n9OEVaBt0RfTDoixo1oCByJSdharclt+PwReazvqQRByhW8F9S4Hz1tFCnpoq4O/
znkQ5wFNgYvMTOFvHG9OVKUhbXbZStO8ugHdUYw2Wo9BTEf5y3WFKx8yZSSLCQVs
pUJlLt8hx+pYzgsrRF/KN8N3Ui+ZOwPcV5/nXYp083IJ47EA5iyal7P4WcPKDJtq
wHQRpBkpIcLKM1L52dH8n/0fo3x+zbxLZJXwpvMwUhJrEgSACP7Mk03FZOMVt6Z9
dEBsqukK6DfAWLdxOEX99ahiM9tk17SMQ5TmaDdG/MUdq5bnXi7zka7TbaF9M86+
Ztj9RlzdpPJ4G5CnutHhOfAsO/EHqNtd1Jl8vTMzxYo8LgudGVbvMcPq0eiAMAY4
ye8gvnBTFNO+/LTkeXFdNXZYenbmiwFpPHlA0DmnCsS/Tizv51bDLdPfcodke0AV
4kiRqk8qbGhmCKb/XtMqulqhZ8b2GkAUt3UOPlAhuS/ClY9lywX1QNB/eCb2zMRI
u3nCkL43STMRCJok/yocXrhLrzaEULZ4AW07F3sJ4vg0JG3PxTE=
=xlDy
-----END PGP SIGNATURE-----

I
I
Ivan Petkov wrote on 9 Jun 2019 01:33
(name . Chris Marusich)(address . cmmarusich@gmail.com)
A1FB5766-B98C-486B-AB7E-FA4DFEFB2CA9@gmail.com
Hi Chris,

Toggle quote (6 lines)
> On Jun 8, 2019, at 11:44 AM, Chris Marusich <cmmarusich@gmail.com> wrote:
>
> Sorry for taking so long to review this. In short, I think these
> changes are good, and if nobody has more feedback in the next few days,
> I will merge it and we can see how it works in practice!

Thanks for the feedback, and thanks for the wonderful pros and cons summary!

Toggle quote (8 lines)
> Cons:
>
> - Because the "dependencies" and "dev-dependencies" are specified as
> package arguments instead of any kind of "input", they won't show
> up in some of the graphs produced by "guix package". However, in
> theory "guix graph" could be taught to display nodes for crate
> "dependencies" and "dev-dependencies", too.

Totally correct, we can iterate on this in the future.

Toggle quote (12 lines)
> - Everyone who defines a Guix package for a crate must make sure the
> origin's file name ends in ".cargo", since the cargo-build-system
> now assumes that any input ending in ".cargo" is a crate that
> should be extracted into the build's crate-dir. This is a little
> brittle, and I wish we had a better way to check this, but I can't
> think of a better way at the moment. Since you've updated the
> importer to always add this, it probably won't be much of an issue
> in practice, since that's the default way new crates will be added
> to Guix. Going forward, maybe we can avoid this by just checking
> the inputs to see which ones are gzipped tarballs containing
> Cargo.toml files.

We discussed an alternative solution to this offline, namely we can ask
tar to check if a Cargo.toml file exists at the top level without fully unpacking
the archive. If it exists, we can assume the tarball is a crate source and only
then unpack it in the vendor directory.

This will make things less brittle as the `.crate` convention won’t be necessary
(a potential downside would be that if there happens to be some arbitrary input
which happens to be a tarball which happens to have a Cargo.toml, it will get
included in the vendor directory without us knowing about it. I think the chances
of this happening in practice are virtually zero, so we can iterate on this if it ever
becomes a problem).

I’m going to try to update the patch series to include this change over the next
few days (along with Ludo’s naming-related feedback). If I don’t get a chance to
finish this, we can always merge it in later!

Toggle quote (16 lines)
> Limitations imposed by Rust/Cargo itself:
>
> - I'm not a Rust or Cargo expert, but my current understanding is
> that it isn't feasible to save the artifacts produced when
> building a crate for re-use when building another crate. In the
> world of C, it is common to produce a library, and then link
> against that library when building other software later. In Guix,
> when building a C library, we install the built artifacts (e.g.,
> .so files) into the output, so that those artifacts can be used as
> the input for another package's build. It seems that, by Cargo's
> design, it isn't currently feasible to do the same sort of thing
> with Cargo: that is, it isn't feasible to build artifacts, install
> them somewhere for later use, and then later re-use them in
> another Cargo build. I'd be glad to learn that I'm mistaken, but
> currently that is my understanding.

Your understanding here is correct.

Toggle quote (13 lines)
> - Related to this, I doubt that a Rust programmer will be able to
> invoke a command like "guix environment my-crate" (even if we
> teach it to understand crate "dependencies" and
> "dev-dependencies") to make all the dependencies required to build
> my-crate available. If a Rust programmer wants to hack on
> my-crate, they'll probably still just use "cargo" to do it without
> using Guix at all. Is there any way to avoid this and make it
> possible to get the dependencies used by Guix in the build, so
> that a Rust programmer can hack around using precisely those
> dependencies? If this were C or Python, you could do that using
> "guix environment," but I'm not sure how this could work with Rust
> crates.

Correct again, a programmer will not be able to run `guix environment my-crate`,
however, I don’t think anyone will do this in practice, since you can’t “point” cargo
at that source closure/outputs anyway.

For hacking on Rust crates, I’d imagine a Rust programmer would simply stick with
cargo and the crates.io http://crates.io/ ecosystem proper. I don’t ever see guix as being a replacement
for crates.io http://crates.io/ (it would be impossible to keep up with the package changes, even if we
automate things, crates aren’t guaranteed to even build, etc.).

I’d imagine that crates should only be imported into guix if they’re necessary for supporting
an actual application or service written in Rust, but not used for day-to-day development.

—Ivan
Attachment: file
I
I
Ivan Petkov wrote on 10 Jun 2019 01:53
(name . Chris Marusich)(address . cmmarusich@gmail.com)
A0537D62-10F8-44C2-A2CC-5A0DB8E0DF7B@gmail.com
I’ve updated the patch series with the following improvements:

* I've applied any naming feedback which came from Ludo
* I've changed the cargo-build-system to only unpack inputs into the cargo
vendor directory if the following applies:
- The input is a path to a gzip tarball
- The archive contains a file called Cargo.toml at its root
- This means that we no longer require crate sources to include a
".crate" extension

Whew, given that this has gotten pretty long, I'd be happy to land this as it
is for now (barring any blocking issues!), and iterating further going forward!

Thanks again to everyone for their feedback!
—Ivan
C
C
Chris Marusich wrote on 12 Jun 2019 03:14
(name . Ivan Petkov)(address . ivanppetkov@gmail.com)
87lfy7zjtw.fsf@garuda.local.i-did-not-set--mail-host-address--so-tickle-me
Hi Ivan,

I've merged your changes as 2444abd9c124cc55f8f19a0462e06a2094f25a9d.
Thank you for taking the time to write this patch series! Now let's
start importing some crates! :-)

Some minor feedback for next time (I've made these minor changes on your
behalf already):

- In the ChangeLog, we generally capitalize the heading and add a
period.
- In the manual, we put two spaces after periods, not one.
- The tests/crate.scm began failing when you changed the importer logic;
I have taken the liberty of fixing it.
- When listing many parts that changed, you can write it like this,
instead of listing it on 3 separate lines:

(maybe-cargo-inputs, maybe-cargo-development-inputs)
(maybe-arguments): Add them.

That's all. Thanks again! I'm closing this now.

--
Chris
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEy/WXVcvn5+/vGD+x3UCaFdgiRp0FAl0AUdsACgkQ3UCaFdgi
Rp0NRg/+JQcJWhJ52KOPVNksrQrzOutFo77x3NUAWPr5U5Tmf2QZw4PKreXS3OOB
pk/G0jJm82LdycSzzPDAaTY5zhd7Oa2g3Mj+lJQjERxNd1YNl+52Gobs/4rddsrp
HzLggtjMqpC5Szh3T6rzhnv5jz+Vpq38Sy/HLCYeFo9PSZY460z4MXXxCj9zNt07
4Qk4ULJNDgHyh/+8BYsObhND0/ROB9jJifjee8EGKuYn9xLGsF7Sk3uhyybEFdAX
7mh981bJKZ7JGU9kHoRuXVtlxpXmScBAdaoHlbXX6i07kWyLiQH4x3ZiBsDe2hl8
aw83gZYlgJX6oDxMIuzZr4NaYCP7uD/LpwRNT8jHjelkfvJnw7AaG7I6Ie7XI2DH
GQsas018cwWOBzcNdtKdjnM2lWKJOwh3sz+0CzZR+R+XrVYP6kOK0/lMh38YBdD3
OKfnYGJqDpr54CmRJq5EiSI8+hiDGQgTj2vN6Gj7IYYho7vVd5E5Nte4uQ1yPTKA
xjKMaSGYb1hXLyuYTqKpc4eqz2PpKJ/sV6jrb/PFV4rIT7LPkafyXySL7h8bepLZ
r9eCfd7RA138K/9nilrDOiaTs5Kh230OjmctWnwCzFT9M+6ocVXf94+KPaC46D7l
uTl8fhdj5Pg0BQs18Qmt3vNgJV2v+pWK+siEgLmkKVAwzgTTSg4=
=PkkD
-----END PGP SIGNATURE-----

Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 35318@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 35318
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch