Ensure all tests in the workspace are run and that const extern fn is always enabled#4151
Merged
tgross35 merged 3 commits intorust-lang:mainfrom Nov 26, 2024
Merged
Conversation
Specifically, this should ensure that unit tests in the `libc` crate don't get missed.
Collaborator
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use |
4f22eb8 to
6d00234
Compare
Contributor
Author
369a795 to
b346c41
Compare
const extern fn is always enabled
tamird
reviewed
Nov 26, 2024
| // cfg completely. | ||
| // FIXME(ctest): ctest can't handle `$(,)?` so we use `$(,)*` which isn't quite correct. | ||
| cfg_if! { | ||
| if #[cfg(feature = "const-extern-fn")] { |
Contributor
There was a problem hiding this comment.
consider removing the feature from the default features (in this commit, so it is backported) and also updating the comment in src/macros.rs in this commit (so it no longer mentions this feature, also so it is backported).
Contributor
Author
There was a problem hiding this comment.
Reasonable 👍 changed, thanks for taking a look.
Contributor
In [1] this conditional was dropped in favor of a Cargo feature, which was turned on by default in [2]. However, this did not help the case where `--no-default-features` is passed. Unfortunately we still can't drop this config entirely since `ctest` cannot parse the syntax, so change back to useing a `cfg` to control constness rather than a Cargo feature. Additionally, remove a portion of the macro's comment that is no longer relevant. Fixes: rust-lang#4149 [1]: rust-lang#4105 [2]: rust-lang#4134
Since moving from a feature to a `cfg`, this feature is unneeded. Remove it in 1.0. Not intended for backport.
b346c41 to
e6d7b51
Compare
tgross35
added a commit
to tgross35/rust-libc
that referenced
this pull request
Nov 26, 2024
Specifically, this should ensure that unit tests in the `libc` crate don't get missed. (backport <rust-lang#4151>) (cherry picked from commit f553033)
tgross35
added a commit
to tgross35/rust-libc
that referenced
this pull request
Nov 26, 2024
In [1] this conditional was dropped in favor of a Cargo feature, which was turned on by default in [2]. However, this did not help the case where `--no-default-features` is passed. Unfortunately we still can't drop this config entirely since `ctest` cannot parse the syntax, so change back to useing a `cfg` to control constness rather than a Cargo feature. Additionally, remove a portion of the macro's comment that is no longer relevant. Fixes: rust-lang#4149 [1]: rust-lang#4105 [2]: rust-lang#4134 (backport <rust-lang#4151>) (cherry picked from commit e18ee8c)
tgross35
added a commit
to tgross35/rust-libc
that referenced
this pull request
Nov 26, 2024
Specifically, this should ensure that unit tests in the `libc` crate don't get missed. (backport <rust-lang#4151>) (cherry picked from commit f553033)
tgross35
added a commit
to tgross35/rust-libc
that referenced
this pull request
Nov 26, 2024
In [1] this conditional was dropped in favor of a Cargo feature, which was turned on by default in [2]. However, this did not help the case where `--no-default-features` is passed. Unfortunately we still can't drop this config entirely since `ctest` cannot parse the syntax, so change back to useing a `cfg` to control constness rather than a Cargo feature. Additionally, remove a portion of the macro's comment that is no longer relevant. Fixes: rust-lang#4149 [1]: rust-lang#4105 [2]: rust-lang#4134 (backport <rust-lang#4151>) (cherry picked from commit e18ee8c)
Contributor
Author
|
Backport in #4152 |
tamird
approved these changes
Nov 26, 2024
| // Specifically, moving the 'cfg_if' into the macro body will *not* work. Doing so would cause the | ||
| // '#[cfg(libc_const_extern_fn)]' to be emitted into user code. The 'cfg' gate will not stop Rust | ||
| // from trying to parse the 'pub const unsafe extern fn', so users would get a compiler error even | ||
| // when the 'libc_const_extern_fn' feature is disabled. |
Contributor
There was a problem hiding this comment.
as i mentioned in the backport PR, this is no longer a feature. i'm noticing now that this is also an inconsistent use of quotes vs ticks below
Contributor
Author
There was a problem hiding this comment.
Thanks for noticing, but I'm not worried about it - this comment is preexisting and will hopefully go away in the near future once ctest gets fixed.
Contributor
|
Thanks for fixing. Please remember to yank the broken releases. |
Contributor
Author
|
There was some other unintentional breakage in the huge cleanup refactoring, it's already yanked. |
Contributor
|
0.2.164 remains unyanked. |
Contributor
Author
|
That version didn't update this feature 0.2.163...0.2.164 |
Contributor
|
Ah, thanks for clarifying. I thought the regression was first noticed after release but it was before. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In 1 this conditional was dropped in favor of a Cargo feature, which was turned on by default in 2. However, this did not help the case where
--no-default-featuresis passed.Unfortunately we still can't drop this config entirely since
ctestcannot parse the syntax, so change back to useing acfgto control constness rather than a Cargo feature.Fixes: #4149