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

Add option to only run build scripts and compile proc macros #9071

Closed
wants to merge 1 commit into from
Closed

Add option to only run build scripts and compile proc macros #9071

wants to merge 1 commit into from

Conversation

dima74
Copy link

@dima74 dima74 commented Jan 13, 2021

Add option to cargo check to only run build scripts and compile proc macros. This is needed for IDEs to e.g. obtain features defined by build scripts. See #7178 for details


This option can be used on stable Rust like this:

RUSTC_BOOTSTRAP=1 cargo check -Zunstable-options --only-build-scripts-and-proc-macros

@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 Jan 13, 2021
@matklad
Copy link
Member

matklad commented Jan 14, 2021

+1, we need something like this for rust-analyzer.

@alexcrichton
Copy link
Member

I'm personally not entirely sure about what to do with options like this. They feel extremely specialized and while they may solve a use case today, I don't think there's a guarantee it'll solve much more in the future or whether the intended users (IDEs) would continue to use this option in the future (or require tweaks or something like that).

You can already effectively emulate this option by setting RUSTC and early-exiting on anything with --emit metadata (sorta, probably slightly more complicated than that). I would personally prefer if that could be made to work rather than adding such a specialized option to cargo.

One of the weaknesses of a feature like this is that it doesn't really have a clear definition. It's pretty ad-hoc and tied to exactly how Cargo implements things today. This means that future changes to Cargo may be difficult to implement an option like this since it may not have a clear mapping into a future refactoring of Cargo.

@matklad
Copy link
Member

matklad commented Jan 14, 2021

I personally am a bit more confident about the general idea :)

The feature IDE want is "run all of the $host code, so that the project can be analyzed entirely within IDE". Today this includes build scripts and proc macros. If tomorrow cargo includes support for first-class code generation, that will also to into this compilation mode. This I think is the same set of things covered by the build-override profile.

I do think this is a part of an IDE end-game package. Even in the glorious future, when rust-analyzer and rustc are one, and Rust has "edit and continue", the IDE would still like to instruct the build system: "please, build all of the proc-macros, so that I can expand them, but don't try to build/check the code, as I am better at showing the error squiggly".

But I also agree with your concerns about unclear scope here. This feels analogous to cargo metadata: today, it is clear that metadata is not really right, and that something like unit-graph would be more correct for IDEs. At the same time, it is correct enough to make both IntelliJ and ra work.

The RUSTC (probably RUSTC_WRAPPER though) work around would work I think, though it't be a bit messy to implement, as it is not super-trivial to ship an wrapper executable to the PC running the IDE. To be clear, the current work-around of running cargo check also works, it just spends a lot of CPU/latency.

@vlad20012
Copy link
Member

Also, I'd suggest not to have lengthy discussions about the option name because most likely no one will use it, except intellij and ra, and infinite unstable status is ok for us, as well as it's ok if the option will be renamed in the future.

@lnicola
Copy link
Member

lnicola commented Jan 15, 2021

as it is not super-trivial to ship an wrapper executable to the PC running the IDE

I suppose we could use the same "run self" trick as in the proc macro server.

@Eh2406
Copy link
Contributor

Eh2406 commented Jan 25, 2021

Sorry for the delay in review. Seams like the code is not the controversial part. I put discussing this on the agenda for tuesdays Cargo Meating.

@ehuss
Copy link
Contributor

ehuss commented Jan 26, 2021

Can you provide some more information about how the IDE integration works?

Do you dlopen the proc-macros and execute them directly?

Are you able to analyze a project without the dependencies? Does intellij/ra somehow find all the dependency sources and analyze them directly from source? How does it know how to set cfg and other flags?

Also, to be clear, from an implementation standpoint this is probably not an approach we would like to go with. Overriding the jobs at such a low level is a bit too much of a hack for my taste. Ideally it would only generate a unit graph for the pieces it needs. It would be good to discuss before venturing more into implementation, though.

@vlad20012
Copy link
Member

vlad20012 commented Jan 27, 2021

Can you provide some more information about how the IDE integration works?

Sure. Both intellij and ra analyze a project and dependencies directly from sources. "dependencies" also includes the standard library. Without dependencies/stdlib information IDE lowers to a simple editor. We use cargo metadata command to retrieve a Cargo package graph and rustc --print sysroot to retrieve the stdlib source location (previously installed with rustup component add rust-src).

Then we need cfg options values. We collect them from different places. cargo metadata output contains cfg features (#[cfg(feature = "foo")]) values for each package. Standard compiler options values are retrieved from rustc --print cfg. The rest of the options are generated by buildscripts. We use cargo check --message-format json - the output contains JSON messages with reason build-script-executed with cfgs, env and out_dir fields (we also use env and out_dir for env!() macro evaluation). We could use cargo build --message-format json instead, but cargo check works faster and returns the same JSON messages for buildscripts. This is a place where this PR is needed - we want to make this step even faster.

Proc macro dlls are retrieved with the same cargo check approach. Yes, we link them using dlopen and execute them directly.

@lnicola
Copy link
Member

lnicola commented Jan 27, 2021

Off-topic: @vlad20012 how do you handle the different proc macro ABI versions? We only support the latest stable one, but figuring out the corresponding compiler versions across beta and stable builds is pretty annoying.

@vlad20012
Copy link
Member

We're evaluating a RUSTC_WRAPPER approach (thanks @alexcrichton for the suggestion!) intellij-rust/intellij-rust#6735. At first glance, it looks like it works. But I think the option is a more robust solution anyway.

@lnicola for now we do it exactly as rust-analyzer does. We even use ra_ap_proc_macro_srv crate. And we build it only on the stable toolchain as well. This is very experimental, though. I'm not sure how we will solve it in the future. How often does the ABI change?

@lnicola
Copy link
Member

lnicola commented Jan 28, 2021

I'm not sure how we will solve it in the future. How often does the ABI change?

Once every couple of months (2-3 releases), it seems.

We even use ra_ap_proc_macro_srv crate.

Oh 😳.

bors bot added a commit to intellij-rust/intellij-rust that referenced this pull request Feb 2, 2021
6735: Compile only buildscripts and proc macros when fetching build scripts r=undin a=vlad20012

Trying suggestion from rust-lang/cargo#9071 (comment)

Co-authored-by: vlad20012 <beskvlad@gmail.com>
@bors
Copy link
Collaborator

bors commented Feb 22, 2021

☔ The latest upstream changes (presumably #9161) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss
Copy link
Contributor

ehuss commented Aug 6, 2021

I'm gonna close this due to inactivity. Generally we prefer new features to have some level of discussion before moving directly to a PR. It is not entirely clear how this should work, and looks like it needs more consideration than can be handled on a PR. More information about the process of adding new features can be found at https://doc.crates.io/contrib/process/index.html. Thanks!

@RalfJung
Copy link
Member

RalfJung commented Aug 9, 2022

You can already effectively emulate this option by setting RUSTC and early-exiting on anything with --emit metadata (sorta, probably slightly more complicated than that). I would personally prefer if that could be made to work rather than adding such a specialized option to cargo.

FWIW this is what IDEs do now and it leads to all sorts of problems. :/
See rust-lang/rust-analyzer#12973.

I think some sort of help from cargo will be needed here, the crude hacks that are used to work around the absence of cargo support are falling apart quickly.

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.

None yet

10 participants