Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cargo rustc should accept multiple packages #12474

Open
mkeeter opened this issue Aug 10, 2023 · 9 comments
Open

cargo rustc should accept multiple packages #12474

mkeeter opened this issue Aug 10, 2023 · 9 comments
Labels
A-rustflags Area: rustflags C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-rustc E-medium Experience: Medium S-needs-rfc Status: Needs an RFC to make progress.

Comments

@mkeeter
Copy link

mkeeter commented Aug 10, 2023

Problem

I've found myself in a situation where I want to

  • Build multiple packages in a single cargo invocation
  • Pass rustc flags on the command-line

The former works with cargo build, e.g. cargo build -p foo -p bar.

However, cargo build only lets you specify flags with RUSTFLAGS, which has different semantics than flags passed by cargo rustc: RUSTFLAGS are applied to all dependencies, not just the top-level package being built.

Proposed Solution

Change cargo rustc to accept multiple packages using the -p flag, matching cargo build, cargo check, etc.

Notes

Limiting cargo rustc to a single package was intentional in the original cargo rustc PR. This may be because no one was asking for multi-package support at the time?

I've implemented multi-package support in a local checkout and it seems to work fine. I'd be happy to work with the team on a PR (with updated tests, etc) if you're open to it!

@mkeeter mkeeter added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage. labels Aug 10, 2023
@epage
Copy link
Contributor

epage commented Aug 10, 2023

However, cargo build only lets you specify flags with RUSTFLAGS, which has different semantics than flags passed by cargo rustc: RUSTFLAGS are applied to all dependencies, not just the top-level package being built.

Would a more accurate title for this issue be "Allow passing rustc flags to specified packages"? That leaves the door more open for us to explore alternatives.

Also, what is your use case for wanting to pass flags only to specific packages? Knowing that can help us better evaluate what solution best fits within cargo.

@mkeeter
Copy link
Author

mkeeter commented Aug 10, 2023

Would a more accurate title for this issue be "Allow passing rustc flags to specified packages"? That leaves the door more open for us to explore alternatives.

For what I'm doing, I need the same rustc flags for every package. "Allow passing rustc flags to specified packages" sounds like a more flexible solution, so it would also work for my use case!

(the obvious downside is number of flags × number of packages scaling, since I'd need to duplicate the flags for each package)

Also, what is your use case for wanting to pass flags only to specific packages? Knowing that can help us better evaluate what solution best fits within cargo.

For my use case, controlling rustc flags is a hard requirement, and building multiple packages in a single Cargo invocation is the feature request.

I need to control rustc flags because we're building (multiple) crates for an embedded system and need custom linker flags (e.g. -C link-arg=-Tlink.x -C link-arg=-r -L path/to/linker/stuff). Because cargo rustc only accepts a single package, I can only build one crate at a time, and have to loop, calling cargo rustc on each one individually. Being able to build multiple crates with custom flags in one Cargo invocation would be significantly faster (I saw 30-40% speedups testing with my modified Cargo).


Aside on why using RUSTFLAGS doesn't work

I'm working to speed up the Hubris build system. Each image is built from a set of tasks (which are crates) and a kernel (which is also a crate). Right now, every individual task and the kernel are built in separate cargo rustc invocations, which has a lot of overhead.

There are three things that together prevent RUSTFLAGS from helping us:

  • Our desired flags change between tasks (which are built as relocatable binaries, -C link-arg=-r) and the kernel (which is not)
  • RUSTFLAGS is passed down to dependencies and included in the hash that Cargo uses for caching
  • Some of our dependencies (specifically stm32h7) are very expensive to compile

The combination of these factors means that if we use RUSTFLAGS, the stm32h7 crate is constantly recompiling (because RUSTFLAGS has changed), taking 20-30 seconds each time.

Using cargo rustc to specify the variable flags isolates them from dependencies, so stm32h7 is built once and reused.


I hope that all makes sense, and I'd be happy to answer further questions!

@epage
Copy link
Contributor

epage commented Aug 10, 2023

Ok. I think I misunderstood

However, cargo build only lets you specify flags with RUSTFLAGS, which has different semantics than flags passed by cargo rustc: RUSTFLAGS are applied to all dependencies, not just the top-level package being built.

to mean that you wanted to control which packages the flags were passed to though I think that can serve as a potential solution.

Instead, the root causes of your build performance issues includes:

  • The RUSTFLAGS is stored external to the file ("fingerprint"), rather than in the file name ("metadata"), forcing everything to rebuild as you alternate between different tasks that use different RUSTFLAGS. Reconsider RUSTFLAGS artifact caching. #8716 is tracking that
  • Link flags only affect the final binaries but RUSTFLAGS isn't granular enough for that, preventing sharing of intermediate build artifacts
    • Granted, with this solved, the need for fixing fingerprinting/metadata is greatly reduced

Does that summarize it?

@weihanglo
Copy link
Member

FWIW, looks like the use case here could be covered by rust-lang/rfcs#3310.

@mkeeter
Copy link
Author

mkeeter commented Aug 10, 2023

Ok. I think I misunderstood

However, cargo build only lets you specify flags with RUSTFLAGS, which has different semantics than flags passed by cargo rustc: RUSTFLAGS are applied to all dependencies, not just the top-level package being built.

to mean that you wanted to control which packages the flags were passed to though I think that can serve as a potential solution.

Instead, the root causes of your build performance issues includes:

  • The RUSTFLAGS is stored external to the file ("fingerprint"), rather than in the file name ("metadata"), forcing everything to rebuild as you alternate between different tasks that use different RUSTFLAGS. Reconsider RUSTFLAGS artifact caching. #8716 is tracking that
  • Link flags only affect the final binaries but RUSTFLAGS isn't granular enough for that, preventing sharing of intermediate build artifacts
    • Granted, with this solved, the need for fixing fingerprinting/metadata is greatly reduced

Does that summarize it?

Yup, that seems like an accurate summary! One more possible detail: there could be esoteric cases where flags should be set for the target package but should not be set for dependencies. I don't have a concrete example of this; the linker flags are unused-but-innocuous when passed to dependencies, but that may not always be the case...

FWIW, looks like the use case here could be covered by rust-lang/rfcs#3310.

Yes, I agree that would also be a potential solution (by letting us use cargo build --root-rustflags)

It seems like there are a bunch of different approaches that we could take here, so I appreciate all the clarifying questions.

@weihanglo
Copy link
Member

I'll mark this as needing RFC while we're waiting for rust-lang/rfcs#3310. extending cargo rustc also seems valid. Either way, I feel like the line between cargo build and cargo rustc would become vaguer in the end.

Also marked as medium level as it is not way too hard to get it done, but I expected it to take time to have a conclusion.

@rustbot label -S-triage +S-needs-rfc +E-medium +A-rustflags +Command-rustc

@rustbot rustbot added A-rustflags Area: rustflags Command-rustc E-medium Experience: Medium S-needs-rfc Status: Needs an RFC to make progress. and removed S-triage Status: This issue is waiting on initial triage. labels Aug 15, 2023
@mkeeter
Copy link
Author

mkeeter commented Aug 17, 2023

I'll mark this as needing RFC while we're waiting for rust-lang/rfcs#3310. extending cargo rustc also seems valid. Either way, I feel like the line between cargo build and cargo rustc would become vaguer in the end.

I'd be happy to do the "extending cargo rustc" implementation work and open a PR. I've already got the code changes in a branch, but would want to add more tests and update the documentation.

Does that seem reasonable, or would the team prefer one of the alternate solutions (e.g. waiting until that RFC is merged + implemented)?

For what it's worth, the changes to cargo rustc are very small:

$ git diff --stat
 src/bin/cargo/commands/rustc.rs    | 14 +++++++-------
 src/cargo/ops/cargo_compile/mod.rs | 25 ++++++-------------------
 2 files changed, 13 insertions(+), 26 deletions(-)

@epage
Copy link
Contributor

epage commented Aug 17, 2023

For me, the bigger question that I'd want from others on the cargo team is what the role of cargo rustc is compared to cargo build, to better understand the existing design principles as they can help guide whether this makes sense or not.

@rib
Copy link

rib commented Mar 22, 2024

I'm also finding myself wanting to be able to pass multiple package to cargo rustc (or pass --crate-type to cargo build) in a project that has multiple crates that can either be built as wasm modules, or alternatively built for the native target and statically linking into an application.

In this case the modules all have crate-type = ["lib"] and I'm looking to use cargo rustc so that --crate-type can be used to force a type of "cdylib" when building for wasm.

We're intentionally avoiding putting crate-type = ["cdylib"] in the Cargo.toml files since that becomes awkward when building the crates for the native target, due to unresolved symbols - and since we don't need the shared library when building for the host target.

Our build workflow / tooling is currently based on cargo build which can build multiple packages in one run, so it's awkward to now have to adapt to running cargo rustc separately for each module.

In this case it seems like it would be convenient if cargo build supported --crate-type, like cargo rustc and that would affect all the top-level packages that it builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustflags Area: rustflags C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-rustc E-medium Experience: Medium S-needs-rfc Status: Needs an RFC to make progress.
Projects
None yet
Development

No branches or pull requests

5 participants