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

feat: allow customizing the command for running build scripts #11956

Merged
merged 1 commit into from
Apr 13, 2022

Conversation

fee1-dead
Copy link
Member

@fee1-dead fee1-dead commented Apr 11, 2022

I have tested this locally and it fixed #9201 with some small changes on the compiler side with suggestions from #9201 (comment).

I have also added an environment variable IS_RA_BUILDSCRIPT_CHECK for crates to detect that it is a check for buildscripts, and allows defaulting to bogus values for expected environment variables.

@fee1-dead
Copy link
Member Author

@Veykril this helps with properly expanding procedural macros in rust-lang/rust. Would you like to review this?

@Veykril
Copy link
Member

Veykril commented Apr 13, 2022

I have also added an environment variable IS_RA_BUILDSCRIPT_CHECK for crates to detect that it is a check for buildscripts, and allows defaulting to bogus values for expected environment variables.

Is this required? I am a bit reluctant to setting that env var since we don't really want crates to specialize for the case where r-a builds them.

@fee1-dead
Copy link
Member Author

Is this required?

Not really. rust-lang/rust requires a few env vars to be set as can be seen with eddyb's comment: CFG_RELEASE= CFG_RELEASE_CHANNEL= RUSTC_INSTALL_BINDIR=. I'm not sure which is better: adding a configuration for rust-analyzer setting env vars when checking for buildscripts, or just use that env var. Not doing anything needs rustc to always default to bogus values if those variables are not set, which should really be treated as errors unless r-a is building them.

@flodiebold
Copy link
Member

flodiebold commented Apr 13, 2022

I wonder if this is the right solution. Usually, if the project is a normal cargo workspace there should be no reason why we can't cargo check everything (especially with the rustc wrapper). If the project is not a normal cargo workspace, cargo check is most likely to not be the right thing to call at all. So to me it would make more sense to make the "run build scripts and compile proc macros" command completely customizable?

@fee1-dead
Copy link
Member Author

So to me it would make more sense to make the "run build scripts and compile proc macros" command completely customizable?

cargo check -p rustc_driver works with the rustc wrapper. But x.py check does not. So the best way is still using cargo check.

@flodiebold
Copy link
Member

flodiebold commented Apr 13, 2022

Sure, but making it fully configurable wouldn't mean that it can't be configured to still use cargo check though -- or to use some x.py variant that either doesn't use the rustc wrapper or does work with it.

(I don't want to let the perfect be the enemy of the good though.)

@fee1-dead
Copy link
Member Author

Yeah. That is why I pushed the fully configurable version.

/// Advanced option, fully override the command rust-analyzer uses for
/// checking buildscripts and procedural macros. The command should
/// include `--message-format=json` or similar option.
checkBuildScriptOnSave_overrideCommand: Option<Vec<String>> = "null",
Copy link
Member

@flodiebold flodiebold Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we call this rust-analyzer.cargo.runBuildScriptsCommand (maybe runBuildScripts.command would be ideal if we didn't already have runBuildScripts as a boolean option). (The point of running cargo check here is not to check the build scripts, it's to have their results and the proc macros available.) For the description how about something like this:

Advanced option, fully override the command rust-analyzer uses to run build scripts and build procedural macros. The command should include `--message-format=json` or a similar option.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@fee1-dead fee1-dead force-pushed the master branch 2 times, most recently from e83eb26 to 2a3b5ea Compare April 13, 2022 13:33
@bjorn3
Copy link
Member

bjorn3 commented Apr 13, 2022

Maybe update the PR title and commit message?

@fee1-dead fee1-dead changed the title feat: allow selecting which packages to check for buildscripts feat: allow customizing the command for running build scripts Apr 13, 2022
@flodiebold
Copy link
Member

bors r+

@Veykril
Copy link
Member

Veykril commented Apr 13, 2022

bors doesn't work right now because of the repo move

@fee1-dead
Copy link
Member Author

fee1-dead commented Apr 13, 2022

Are you working on the PRs to migrate to homu? I would like to help.

@Veykril
Copy link
Member

Veykril commented Apr 13, 2022

Ye I was about to look into that, help with would be appreciated :)

@Veykril
Copy link
Member

Veykril commented Apr 13, 2022

@bors ping

@Veykril
Copy link
Member

Veykril commented Apr 13, 2022

@bors r=flodiebold

@bors
Copy link
Contributor

bors commented Apr 13, 2022

📌 Commit 73a033e has been approved by flodiebold

@bors
Copy link
Contributor

bors commented Apr 13, 2022

⌛ Testing commit 73a033e with merge 15844bf...

@bors
Copy link
Contributor

bors commented Apr 13, 2022

☀️ Test successful - checks-actions
Approved by: flodiebold
Pushing 15844bf to master...

@bors bors merged commit 15844bf into rust-lang:master Apr 13, 2022
@lf-
Copy link
Contributor

lf- commented Apr 13, 2022

Can we submit a change to the rustc contributor docs to say how to enable this feature?

@fee1-dead
Copy link
Member Author

Can we submit a change to the rustc contributor docs to say how to enable this feature?

It shouldn't be changed until another nightly is released, rust-analyzer bumped in rust-lang/rust. But in the mean time I will write the instructions up and prepare a PR to rustc-dev-guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rust-lang/rust: guide recommends disabling proc macro server. we should make it work
6 participants