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

Support -Zbuild-std #235

Merged
merged 2 commits into from
Jun 11, 2024
Merged

Support -Zbuild-std #235

merged 2 commits into from
Jun 11, 2024

Conversation

daxpedda
Copy link
Contributor

As discussed in #197 (comment).

Fixes #197.

@@ -78,6 +78,26 @@ fn cfgs(config: &Config) -> Result<Vec<Cfg>, Errored> {
Ok(cfgs)
}

pub(crate) fn has_build_std(info: &DependencyBuilder) -> bool {
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be simpler to have a build_std bool field on DependencyBuilder and manually inject the -Z flags instead of expecting the user to supply them and have us detect them

Copy link
Contributor Author

@daxpedda daxpedda May 26, 2024

Choose a reason for hiding this comment

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

This couldn't be a bool because sometimes its relevant to pass arguments like -Zbuild-std=std,panic_abort.
But I could instead make it an Option<String>?

I think another motivation to not add a configuration is that when rust-lang/wg-cargo-std-aware#20 is addressed, we wouldn't need a configuration either.

Let me know how you want me to proceed.

Copy link
Owner

Choose a reason for hiding this comment

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

But I could instead make it an Option<String>?

yea that seems good

I think another motivation to not add a configuration is that when rust-lang/wg-cargo-std-aware#20 is addressed, we wouldn't need a configuration either.

I'm doing regular major bumps anyway, seems fine to do another once that lands (or just mark it as deprecated then)

Copy link
Contributor Author

@daxpedda daxpedda Jun 4, 2024

Choose a reason for hiding this comment

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

Done.

After trying it out though I have to say its a bit weird, because now if the user supplies the -Zbuild-std argument without enabling the DependencyBuilder::build_std option, it won't work.

Old Version: daxpedda@72346fd (leaving this here just in case if you change your mind)

@oli-obk
Copy link
Owner

oli-obk commented May 25, 2024

is there any reasonably small test that we can add here so I don't accidentally make it stop working in the future?

@daxpedda
Copy link
Contributor Author

is there any reasonably small test that we can add here so I don't accidentally make it stop working in the future?

Would love to, but I would need some guidance to do that, would you put the test in integration or unit tests?
I see that you didn't manage adding a test in #196, which I would have drawn inspiration from.

@oli-obk
Copy link
Owner

oli-obk commented Jun 3, 2024

I see that you didn't manage adding a test in #196, which I would have drawn inspiration from.

that one was specifically for cross compilation, which is a bit annoying. Here we can just build-std for the host itself. You can probably just add a copy of the config in

with build-std enabled

@daxpedda
Copy link
Contributor Author

daxpedda commented Jun 4, 2024

I see that you didn't manage adding a test in #196, which I would have drawn inspiration from.

that one was specifically for cross compilation, which is a bit annoying. Here we can just build-std for the host itself. You can probably just add a copy of the config in


with build-std enabled

I had to use basic-bin instead, because -Zbuild-std and proc-macros are a bit weird.
The remaining issues are:

  • We need to add a nightly configuration to the CI and (done) run this test only when running nightly.
  • Other tests should not run in nightly because their output will be different.

I think it might be best to add a separate integration test entirely?

@daxpedda daxpedda requested a review from oli-obk June 4, 2024 14:54
@daxpedda daxpedda force-pushed the build-std branch 2 times, most recently from 88d7b8f to c882246 Compare June 4, 2024 15:08
@oli-obk
Copy link
Owner

oli-obk commented Jun 4, 2024

I think it might be best to add a separate integration test entirely?

oof yea 😆 that makes sense

@daxpedda daxpedda force-pushed the build-std branch 2 times, most recently from a7d5546 to 145f21f Compare June 4, 2024 15:58
@daxpedda
Copy link
Contributor Author

daxpedda commented Jun 4, 2024

I think it might be best to add a separate integration test entirely?

oof yea 😆 that makes sense

So I adjusted the integration test to take an env variable to set the folder and then just copied basic-bin with the appropriate build_std addition, which is a bit crude but it works.

I think I could instead make a simple test using ui_test without the whole machinery in place to check the Cargo output?
Let me know what you think.

@oli-obk
Copy link
Owner

oli-obk commented Jun 5, 2024

I think I could instead make a simple test using ui_test without the whole machinery in place to check the Cargo output?
Let me know what you think.

right, we only care that it works, not what the output is

@daxpedda
Copy link
Contributor Author

I think I could instead make a simple test using ui_test without the whole machinery in place to check the Cargo output?
Let me know what you think.

right, we only care that it works, not what the output is

Done, let me know if you want any other adjustments.

(sorry for the delay)

@oli-obk
Copy link
Owner

oli-obk commented Jun 11, 2024

Thanks! This is great!

@oli-obk oli-obk merged commit e336320 into oli-obk:main Jun 11, 2024
8 checks passed
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.

-Zbuild-std support
2 participants