-
Notifications
You must be signed in to change notification settings - Fork 13k
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
tidy: remove filtering for wasm32 deps, as this don't work #113449
Conversation
r? @ozkanonur (rustbot has picked a reviewer for you, use r? to override) |
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
!target.matches( | ||
"wasm32-unknown-unknown", | ||
&[ | ||
Cfg::from_str("target_arch=\"wasm32\"").unwrap(), | ||
Cfg::from_str("target_os=\"unknown\"").unwrap(), | ||
], | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why, but one of suspects is https://docs.rs/cargo-platform/0.1.2/cargo_platform/enum.Platform.html#method.matches
It says "Returns whether the Platform matches the given target and cfg.", but what means "matches"?
Lets skip target part and look at cfg.
There can be 2 situations:
cfg(not(windows))
matchescfg(target_arch="wasm32")
(matches currently): which can be understand as: if X can be Y? Butcfg(not(windows))
for example can bex86_64-unknown-linux-gnu
which actually not matchescfg(target_arch="wasm32")
.- If X has cfgs which is Y?
cfg(all(target_arch="wasm32",target_os="unknown"))
matchescfg(target_arch="wasm32")
becausetarget_arch
is "wasm32".
But 2. can be broken again if cfg(any(target_arch="wasm32", target_arch="x86_64"))
matched against cfg(target_arch="wasm32")
, as it can be x86_64-unknown-linux-gnu
again, which is not cfg(target_arch="wasm32")
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, looks like this don't work as i expected:
Name("wasm32-wasi").matches("", &[Cfg::from_str("target_arch=\"wasm32\"").unwrap(),]) == false
, while should return true (ignore part that Platform::matches checks target or cfg).
cargo_platform
works on strings and don't know to parse triples or if one cfg turn on other cfgs (for example target_env="msvc" should enable target_os="windows").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments are messy (for me too, in month later), so, please ask question questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should try to make this work instead of getting rid of it entirely. If we really want to remove it, we can revert the changes that were made, but I don't believe this is the best solution for this situation.
@klensy i assume this is ready for review? can you mark it as ready for review {which will no longer keep it as a draft} |
While technically it's ready, i'm tried to get some discussion how |
I ignore the PR completely since it appears to be in draft. Will review it in couple of days. |
☔ The latest upstream changes (presumably #116508) made this pull request unmergeable. Please resolve the merge conflicts. |
@klensy does this still need to be a draft? looks like it is waiting for review which generally means the pr is ready for review :P |
I left a review note at #113449 (comment) |
☔ The latest upstream changes (presumably #117372) made this pull request unmergeable. Please resolve the merge conflicts. |
@rustbot author (due to #113449 (comment) and conflicts) |
will be removed in #123788 |
This was added in #106470, from that time something changed and now it don't work as expected.