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

[Chris Marusich <cmmarusich@gmail.com>]

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