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

Reinstate CARGO_PRIMARY_PACKAGE #7205

Closed
wants to merge 1 commit into from
Closed

Conversation

nhynes
Copy link

@nhynes nhynes commented Aug 2, 2019

#7069 removed CARGO_PRIMARY_PACKAGE since it's no longer needed by Clippy. It is, however, useful for other rustc wrappers. It'd be great to have this back.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 2, 2019
@alexcrichton
Copy link
Member

This was added a long time ago in #5824 for cargo fix, so it wasn't really intended to be a first-class feature of Cargo per se. Could you elaborate on what you're thinking this would be useful for?

@nhynes
Copy link
Author

nhynes commented Aug 5, 2019

Could you elaborate on what you're thinking this would be useful for?

It's for when you have a rustc wrapper that you want to run only for the primary package--just like Clippy. Detecting this without CARGO_PRIMARY_PACKAGE is rather application specific and depends on yet other implementation details like the ___ proc macro crate and the name of build_script_build.

@alexcrichton
Copy link
Member

Hm yes that was the old purpose of this env var but can you elaborate what the specific use cases you're thinking of are? The idea of a "primary package" is pretty fuzzy and changes quite a bit over time as we add features and fix things, so it's not clear that there's something we could commit to stabilizing.

@nhynes
Copy link
Author

nhynes commented Aug 9, 2019

Sorry, let me be less vague.

My specific use case is to generate something akin to protobuf from the types and impls in what was previously called the primary package. I don't want to run the generator on dependencies because that would incur unnecessary computation or, otherwise, complicate the generator logic (e.g., "is something missing because the user forgot it, or because it's running on a dependency?").

As far as I can tell, Clippy used it similarly to ensure that users don't get errors and warnings from code they didn't write.

In my mind, a primary package is a stable concept:

  • a binary or library
  • not a dependency, build script, or test/bench harness
  • the thing that the user has in mind when they cargo build -p

Strictly speaking, this can be worked around--even somewhat stably by reading the Cargo.toml---but it's unfortunate to re-implement what the compiler is already determining for itself.

Maybe the right answer isn't to put back CARGO_PRIMARY_PACKAGE, but at least here's a vote for something like it. There probably aren't enough users of rustc_driver::Callbacks to make a well informed decision either, I guess. Please feel free to close this issue.

@alexcrichton
Copy link
Member

Egad sorry for the delay @nhynes!

Can you also clarify how you're overriding rustc to see this? That seems like it would be difficult to do from crates.io as a dependency.

In terms of an unstable concept what I'm thinking about is things like what do we do with workspaces, what do we do with multiple -p arguments on the command line, things like test binaries vs test targets, etc. The concept of a "primary package" has evolved in Cargo itself over time to adapt to new features we've added to Cargo and otherwise just rationalizing how all the features fit together.

I'm not necessarily saying we can't have a stable concept of a primary package, what I'm saying is that we probably shouldn't just take exactly what's there today and call it stable. Rather we should probably sit down and think about this and see if it's the right thing to stabilize.

@nhynes
Copy link
Author

nhynes commented Aug 20, 2019

Egad sorry for the delay

Hey no worries! You've only got, what? The entire rest of the Rust ecosystem to look at :)

Can you also clarify how you're overriding rustc to see this

Oh, it's just a RUSTC_WRAPPER that uses rustc_driver. Nothing complicated.

what do we do with workspaces, what do we do with multiple -p arguments on the command line, things like test binaries vs test targets

That's a great point. The idea of "primary package" is application specific. In the case of bins and libs, it's fairly straightforward: whatever is (implicitly) passed to -p depending on whether there's a --bin(s) or --lib flag. For test and bench targets it gets a bit more complicated since one could think of either the object-under-test or the test driver as primary.

From a rustc_wrapper implementer's point of view, the only difficult thing to detect is the lib specified by -p since the dependencies are also libs and share the same args. The simplest "fix" would be IS_TERMINAL_LIB, but that's messy.

Ideally, one would parse the result of cargo b --build-plan, but that destroys incremental build, for some reason. I should probably look into that...

In any case, I'll close this for now and hope that someone else with a similar problem comes along someday :)

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants