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.

Sign up for GitHub

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

Jump to bottom

Pass compile mode to the custom build script #10126

Closed
wants to merge 5 commits into from
Closed

Pass compile mode to the custom build script #10126

wants to merge 5 commits into from

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Nov 27, 2021 •
edited
Loading

Closes #4001

This PR adds code to track and pass compile mode to the build scripts. Since the unit's mode is always RunCustomBuild, I had to add tracking for the parent unit's compile mode, and pass it around.

TODO:

  • env var naming. I used CARGO_MODE, but perhaps another name is better? (e.g. CARGO_CMD, CARGO_COMMAND, ... ?)
  • env var values naming. I used: test, build, check, bench, doc, doctest, and docscrape. The last two might need to be substituted with doc, or panic on them?
  • Should the bool values in Check and Doc modes be shown? Both values are currently ignored.
  • figure out why cargo bench sets env var to test
  • figure out why unit tests are failing - doesn't seem to be related to changes.
  • add unit tests

@nyurik
Pass compile mode to the build script
b07a196 
Closes rust-lang#4001

This PR adds code to track and pass compile mode to the build scripts. Since the unit's mode is always `RunCustomBuild`, I had to add tracking for the parent's compile mode, and pass it around.

  - [ ] env var naming. I used `CARGO_MODE`, but perhaps another name is better?
  - [ ] env var values naming. I used: `Test`, `Build`, `Check_test/Check`, `Bench`, `Doc_with_deps/Doc`, `Doctest`, `Docscrape`, `RunCustomBuild`
  - [ ] how to represent bool values for `Check` and `Doc`. I created two separate values, but perhaps the `test` and `deps` values should be ignored?
  - [ ] figure out why `cargo bench` sets env var to `Test`
  - [ ] add unit tests
@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.

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 Nov 27, 2021
@nyurik nyurik mentioned this pull request Nov 27, 2021
Open
@jyn514
Copy link
Member

jyn514 commented Nov 27, 2021

Test, Build, Check_test/Check, Bench, Doc_with_deps/Doc, Doctest, Docscrape, RunCustomBuild

nit: I think it would be better to use snake_case for these, the same way that they're named on the command line.

I also think there doesn't need to be so many modes; just check, build, test, and doc seem enough. doc_scrape in particular shouldn't be passed because it's unstable and build scripts shouldn't depend on it.

jyn514
jyn514 reviewed Nov 27, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

I left a few suggestions, but I defer to Ehuss's expertise 😁 I did see that this doesn't have any dependency tracking - I think there needs to be some logic to rerun the build script when the root_mode changes.

src/cargo/core/compiler/custom_build.rs Outdated
}
CompileMode::Bench => "Bench",
CompileMode::Doc { deps } => {
if deps {
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't matter to the build script, no dependencies will be built after it runs.

src/cargo/core/compiler/unit.rs Outdated
@@ -52,6 +52,8 @@ pub struct UnitInner {
pub kind: CompileKind,
/// The "mode" this unit is being compiled for. See [`CompileMode`] for more details.
pub mode: CompileMode,
/// The "mode" of the root unit. Required when unit's mode is `CompileMode::RunCustomBuild`
pub root_mode: CompileMode,
Copy link
Member

Choose a reason for hiding this comment

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

Rather than adding an additional field you don't always need, I think it might make sense to add a field to the RunCustomBuild unit:

enum CompileMode {
    RunCustomBuild { root_mode: Box<CompileMode> },
}

Copy link
Contributor Author

@nyurik nyurik Nov 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@jyn514 that's an awesome idea, thanks! One issue -- the enum implements Copy trait which cannot be auto-implemented with the Box<>. I wonder if i can get away with the Arc instead? Apparently Arc doesn't support it either... Any suggestions?

@nyurik
Copy link
Contributor Author

nyurik commented Nov 27, 2021

I left a few suggestions, but I defer to Ehuss's expertise grin I did see that this doesn't have any dependency tracking - I think there needs to be some logic to rerun the build script when the root_mode changes.

@jyn514 thx again for the feedback, I switched to &'static str instead of the Box - this way Copy trait continues to work. Also renamed the strings. There is still a question of what to do with Doctest and Docscrape (see code)

@bors
Copy link
Collaborator

bors commented Feb 22, 2022

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

@bors
Copy link
Collaborator

bors commented Feb 28, 2022

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

@ehuss
Copy link
Contributor

ehuss commented Mar 23, 2022

Closing as this PR has become a bit stale. Also, this appears to be a pretty major change that will be quite difficult to make as it will have an impact on every project using build scripts. Unfortunately the Cargo team doesn't have the capacity to accept new features of this nature at this time.

@ehuss ehuss closed this Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

Assignees

@Eh2406 Eh2406

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inform build scripts whether cargo is checking or building
6 participants
@nyurik @rust-highfive @jyn514 @bors @ehuss @Eh2406