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

Allow test to be built with build-std #97037

Closed
wants to merge 1 commit into from

Conversation

jschwe
Copy link
Contributor

@jschwe jschwe commented May 14, 2022

test requires the feature restricted_std to be enabled in order to be built with build-std.
For me this solves issue rust-lang/wg-cargo-std-aware#72, at least in so far as that building the test crate with build-std is successfull.

I am not familiar with restricted_std, but enabling it doesn't seem to cause any test failures. Would this be acceptable, or is there something else I'm not aware of, that makes adding restricted_std not desirable?

`test` requires the feature `restricted_std` to be enabled in order to
be built with build-std.
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 14, 2022
@rust-highfive
Copy link
Collaborator

r? @thomcc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2022
@bjorn3
Copy link
Member

bjorn3 commented May 14, 2022

I believe restricted_std indicates that there is no real libstd implementation for the target and all functions are implemented as dummies or always erroring or panicking functions. This means that libtest won't be able to print out any test results at all, not will it be able to spawn new threads to run the tests on. If you want #[test] to work on uefi you should use the custom test framework feature instead. https://os.phil-opp.com/testing/ is an example of a project using it.

@jschwe
Copy link
Contributor Author

jschwe commented May 14, 2022

I am actually interested in using test in conjunction with features from custom_test_framework. In particular all I want from test is #[test] gathering the test functions in an array and providing the TestDescAndFn type, which to my knowledge does not require runtime support of std. I would provide the actual test runner myself. But for this I need the test crate to compile with build-std.

The test_case macro is rather limited.

@bjorn3
Copy link
Member

bjorn3 commented May 14, 2022

The test_case macro is rather limited.

You can write a proc macro expanding to #[test_case]. That is pretty much what #[test] does internally too.

@thomcc
Copy link
Member

thomcc commented May 15, 2022

I'm not familiar with restricted_std. Given @bjorn3's comments, and that you mentioned not being familiar with it either, I'm not inclined to accept it, but I'll punt to someone else in case.

r? rust-lang/libs

@rust-highfive rust-highfive assigned joshtriplett and unassigned thomcc May 15, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2022
@jyn514
Copy link
Member

jyn514 commented Dec 27, 2022

I don't think this alone will do what you want. rust-lang/wg-cargo-std-aware#72 (comment)

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 27, 2022
@jyn514
Copy link
Member

jyn514 commented Dec 27, 2022

Oh, and for context it turns out restricted_std was originally introduced specifically for build-std, so this PR is ~harmless to land: #74033. but I am little wary of doing that before someone confirms they can actually get cargo test -Zbuild-std working without further changes.

@thomcc
Copy link
Member

thomcc commented Dec 28, 2022

I'm a lot more familiar with restricted_std now. I agree with the consensus that this will likely not solve the issue, but I'll take a closer look (after the new year).

r? @thomcc

@rustbot rustbot assigned thomcc and unassigned joshtriplett Dec 28, 2022
@thomcc thomcc added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 28, 2022
@thomcc
Copy link
Member

thomcc commented Jan 4, 2023

I don't mind adding #![feature(restricted_std)] here, but I don't think this approach will work as-is. Much of std is broken at runtime under restricted_std, so it building is not an indication of very much. I believe that libtest needs to know if we're targetting a restricted_std target (it won't know this with this implementation), and will likely need to use approaches like it already uses on certain limited platforms (the support may need to be extended too).

IOW, please actually test this on a restricted_std target -- it compiling successfully is a guarantee of very little. We also will need to have a t-libs meeting to discuss this probably, but I'd like to see what it takes to support these targets -- at the moment we don't have enough info for me to feel like that would be very productive.

(Important note for future readers: I believe this doesn't change our stable support, even if -Zbuild-std stabilizes: In order to actually use this to run tests on a restricted_std target, I believe the user's crate will likely need a #![feature(restricted_std)] declaration, and std will need to be built with --features=restricted_std, which is already something we need to prevent stable code from ever doing)

Anyway, please do the testing, and then reassign for my review with at-rustbot ready.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2023
@Dylan-DPC
Copy link
Member

@jschwe any updates on this?

@jschwe
Copy link
Contributor Author

jschwe commented May 14, 2023

I haven't had the time to further pursue this, so I'll be closing this PR for now.

@jschwe jschwe closed this May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants