GNU bug report logs - #35318
[PATCH] Update cargo-build-system to expand package inputs

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 35318 in the body.
You can then email your comments to 35318 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ivan Petkov <ivanppetkov <at> gmail.com>
To: guix-patches <at> gnu.org
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>,
 Chris Marusich <cmmarusich <at> gmail.com>
Subject: [PATCH] Update cargo-build-system to expand package inputs
Date: Thu, 18 Apr 2019 22:34:05 -0700

[Message part 1 (text/plain, inline)]

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

[0001-packages-allow-dynamic-input-closure-computation.patch (application/octet-stream, attachment)]

[0002-build-system-cargo-expand-transitive-package-inputs.patch (application/octet-stream, attachment)]

[0003-gnu-crate-add-unicode-xid.patch (application/octet-stream, attachment)]

[0004-gnu-crate-Add-proc-macro2-and-quote.patch (application/octet-stream, attachment)]

[Message part 6 (text/plain, inline)]

Message #8 received at 35318 <at> debbugs.gnu.org (full text, mbox):

From: Ivan Petkov <ivanppetkov <at> gmail.com>
To: 35318 <at> debbugs.gnu.org
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>,
 Chris Marusich <cmmarusich <at> gmail.com>
Subject: Re: [PATCH] Update cargo-build-system to expand package inputs
Date: Sat, 4 May 2019 09:40:37 -0700

Friendly ping!

Thanks,
—Ivan

Message #11 received at 35318 <at> debbugs.gnu.org (full text, mbox):

From: Danny Milosavljevic <dannym <at> scratchpost.org>
To: Ivan Petkov <ivanppetkov <at> gmail.com>, <ludo <at> gnu.org>
Cc: 35318 <at> debbugs.gnu.org, Chris Marusich <cmmarusich <at> gmail.com>
Subject: Re: [PATCH] Update cargo-build-system to expand package inputs
Date: Sat, 4 May 2019 20:31:23 +0200

[Message part 1 (text/plain, inline)]

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.

[Message part 2 (application/pgp-signature, inline)]

Message #14 received at 35318 <at> debbugs.gnu.org (full text, mbox):

From: Ivan Petkov <ivanppetkov <at> gmail.com>
To: Danny Milosavljevic <dannym <at> scratchpost.org>
Cc: ludo <at> gnu.org, 35318 <at> debbugs.gnu.org, Chris Marusich <cmmarusich <at> gmail.com>
Subject: Re: [PATCH] Update cargo-build-system to expand package inputs
Date: Sat, 4 May 2019 14:09:44 -0700

Hi Danny,

Thanks for the feedback!

> On May 4, 2019, at 11:31 AM, Danny Milosavljevic <dannym <at> 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.

> 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

Message #17 received at 35318 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Ivan Petkov <ivanppetkov <at> gmail.com>
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>, 35318 <at> debbugs.gnu.org,
 Chris Marusich <cmmarusich <at> gmail.com>
Subject: Re: [bug#35318] [PATCH] Update cargo-build-system to expand package
 inputs
Date: Mon, 06 May 2019 10:00:50 +0200

Hello,

Ivan Petkov <ivanppetkov <at> gmail.com> skribis:

> From ca6dfd9451f22c4d6dc02aa7eceee0c35800dd57 Mon Sep 17 00:00:00 2001
> From: Ivan Petkov <ivanppetkov <at> 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:

> 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’.

Message #20 received at 35318 <at> debbugs.gnu.org (full text, mbox):

From: Ivan Petkov <ivanppetkov <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>, 35318 <at> debbugs.gnu.org,
 Chris Marusich <cmmarusich <at> gmail.com>
Subject: Re: [bug#35318] [PATCH] Update cargo-build-system to expand package
 inputs
Date: Mon, 6 May 2019 09:04:32 -0700

Hi Ludo,

> On May 6, 2019, at 1:00 AM, Ludovic Courtès <ludo <at> 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.

> 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.

> 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

Message #23 received at 35318 <at> debbugs.gnu.org (full text, mbox):

From: Ivan Petkov <ivanppetkov <at> gmail.com>
To: 35318 <at> debbugs.gnu.org
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Chris Marusich <cmmarusich <at> gmail.com>
Subject: Re: [bug#35318] [PATCH] Update cargo-build-system to expand package
 inputs
Date: Thu, 9 May 2019 16:17:46 -0700

[Message part 1 (text/plain, inline)]

Updated the patch series as per Ludo’s feedback.

Thanks,
—Ivan

[0001-build-system-cargo-expand-transitive-package-inputs.patch (application/octet-stream, attachment)]

[0002-gnu-crate-add-unicode-xid.patch (application/octet-stream, attachment)]

[0003-gnu-crate-Add-proc-macro2-and-quote.patch (application/octet-stream, attachment)]

Message #26 received at 35318 <at> debbugs.gnu.org (full text, mbox):

From: Ivan Petkov <ivanppetkov <at> gmail.com>
To: 35318 <at> debbugs.gnu.org
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Chris Marusich <cmmarusich <at> gmail.com>
Subject: Re: [bug#35318] [PATCH] Update cargo-build-system to expand package
 inputs
Date: Tue, 14 May 2019 23:08:06 -0700

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

Message #29 received at 35318 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Ivan Petkov <ivanppetkov <at> gmail.com>
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>, 35318 <at> debbugs.gnu.org,
 Chris Marusich <cmmarusich <at> gmail.com>
Subject: Re: [bug#35318] [PATCH] Update cargo-build-system to expand package
 inputs
Date: Wed, 15 May 2019 14:44:29 +0200

Hello!

Ivan Petkov <ivanppetkov <at> gmail.com> skribis:

> 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’.

Message #32 received at 35318 <at> debbugs.gnu.org (full text, mbox):

From: Ivan Petkov <ivanppetkov <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>, 35318 <at> debbugs.gnu.org,
 Chris Marusich <cmmarusich <at> gmail.com>
Subject: Re: [bug#35318] [PATCH] Update cargo-build-system to expand package
 inputs
Date: Sun, 19 May 2019 18:00:01 -0700

[Message part 1 (text/plain, inline)]

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

[0001-build-system-cargo-expand-transitive-crate-sources.patch (application/octet-stream, attachment)]

[0002-build-system-cargo-use-sources-from-package-sources.patch (application/octet-stream, attachment)]

[0003-build-system-cargo-don-t-copy-source-as-an-output.patch (application/octet-stream, attachment)]

[0004-doc-Update-cargo-build-system-parameter-docs.patch (application/octet-stream, attachment)]

[0005-import-crate-import-sources-with-.crate-extension.patch (application/octet-stream, attachment)]

[0006-import-crate-define-dependencies-as-arguments.patch (application/octet-stream, attachment)]

[0007-gnu-crate-add-unicode-xid.patch (application/octet-stream, attachment)]

[0008-gnu-crate-Add-proc-macro2-and-quote.patch (application/octet-stream, attachment)]

Message #35 received at 35318 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Ivan Petkov <ivanppetkov <at> gmail.com>
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>, 35318 <at> debbugs.gnu.org,
 Chris Marusich <cmmarusich <at> gmail.com>
Subject: Re: [bug#35318] [PATCH] Update cargo-build-system to expand package
 inputs
Date: Mon, 20 May 2019 21:38:36 +0200

Hi Ivan!

Ivan Petkov <ivanppetkov <at> gmail.com> skribis:

> 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.

> From 5457f60036ce1354b4b89b9c3c423cca14e3a777 Mon Sep 17 00:00:00 2001
> From: Ivan Petkov <ivanppetkov <at> 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.

[...]

> +(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?

> +(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.

> +(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’?

> +(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’.

Message #38 received at 35318 <at> debbugs.gnu.org (full text, mbox):

From: Ivan Petkov <ivanppetkov <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>, 35318 <at> debbugs.gnu.org,
 Chris Marusich <cmmarusich <at> gmail.com>
Subject: Re: [bug#35318] [PATCH] Update cargo-build-system to expand package
 inputs
Date: Tue, 21 May 2019 19:48:24 -0700

Hi Ludo!

> On May 20, 2019, at 12:38 PM, Ludovic Courtès <ludo <at> 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.

>> +(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)?

>> +(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!

>> +(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.

> 
> That’s it.  :-)
> 
> Thank you for improving Rust support!

Happy to help :)

—Ivan

Message #41 received at 35318 <at> debbugs.gnu.org (full text, mbox):

From: Chris Marusich <cmmarusich <at> gmail.com>
To: Ivan Petkov <ivanppetkov <at> gmail.com>
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>,
 Ludovic Courtès <ludo <at> gnu.org>, 35318 <at> debbugs.gnu.org
Subject: Re: [bug#35318] [PATCH] Update cargo-build-system to expand package
 inputs
Date: Sat, 08 Jun 2019 11:44:55 -0700

[Message part 1 (text/plain, inline)]

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

[signature.asc (application/pgp-signature, inline)]

Message #44 received at 35318 <at> debbugs.gnu.org (full text, mbox):

From: Ivan Petkov <ivanppetkov <at> gmail.com>
To: Chris Marusich <cmmarusich <at> gmail.com>
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>,
 Ludovic Courtès <ludo <at> gnu.org>, 35318 <at> debbugs.gnu.org
Subject: Re: [bug#35318] [PATCH] Update cargo-build-system to expand package
 inputs
Date: Sat, 8 Jun 2019 16:33:50 -0700

[Message part 1 (text/plain, inline)]

Hi Chris,

> On Jun 8, 2019, at 11:44 AM, Chris Marusich <cmmarusich <at> 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!

> 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.

>    - 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!

> 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.

>    - 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

[Message part 2 (text/html, inline)]

Message #47 received at 35318 <at> debbugs.gnu.org (full text, mbox):

From: Ivan Petkov <ivanppetkov <at> gmail.com>
To: Chris Marusich <cmmarusich <at> gmail.com>
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>, 35318 <at> debbugs.gnu.org
Subject: Re: [bug#35318] [PATCH] Update cargo-build-system to expand package
 inputs
Date: Sun, 9 Jun 2019 16:53:48 -0700

[Message part 1 (text/plain, inline)]

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

[0001-build-system-cargo-expand-transitive-crate-sources.patch (application/octet-stream, attachment)]

[0002-build-system-cargo-use-sources-from-package-sources.patch (application/octet-stream, attachment)]

[0003-build-system-cargo-don-t-copy-source-as-an-output.patch (application/octet-stream, attachment)]

[0004-doc-Update-cargo-build-system-parameter-docs.patch (application/octet-stream, attachment)]

[0005-import-crate-define-dependencies-as-arguments.patch (application/octet-stream, attachment)]

[0006-gnu-crate-add-unicode-xid.patch (application/octet-stream, attachment)]

[0007-gnu-crate-Add-proc-macro2-and-quote.patch (application/octet-stream, attachment)]

Message #52 received at 35318-done <at> debbugs.gnu.org (full text, mbox):

From: Chris Marusich <cmmarusich <at> gmail.com>
To: Ivan Petkov <ivanppetkov <at> gmail.com>
Cc: Danny Milosavljevic <dannym <at> scratchpost.org>,
 Chris Marusich <cmmarusich <at> gmail.com>, 35318-done <at> debbugs.gnu.org
Subject: Re: [bug#35318] [PATCH] Update cargo-build-system to expand package
 inputs
Date: Tue, 11 Jun 2019 18:14:03 -0700

[Message part 1 (text/plain, inline)]

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

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.