[bug#35155] [PATCH] build-system/cargo: refactor phases to successfully build

[Ivan Petkov]

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 <address@hidden> 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