-
Notifications
You must be signed in to change notification settings - Fork 105
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
Don't allow default deps in planning of platform deps. #437
Conversation
Looks like there are a bunch of Clippy errors in code I didn't change. I'll follow up on that if this patch should move forward. I'm not going to change them now, because that would make the substantive change difficult to see in the diff. |
It looks like most of the other problems of this kind I'm encountering are due to bazelbuild/bazel#13785 |
This looks reasonable to me. FWIW I think a way to work around bazelbuild/bazel#13785 would be to stop concatenating selects - to make deps = [
"@raze__cfg_if__1_0_0//:cfg_if",
"@raze__crc32fast__1_2_1//:crc32fast",
"@raze__libc__0_2_86//:libc",
"@raze__miniz_oxide__0_4_4//:miniz_oxide",
] + selects.with_or({
# cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))
(
"@rules_rust//rust/platform:wasm32-unknown-unknown",
"@rules_rust//rust/platform:wasm32-wasi",
): [
"@raze__miniz_oxide__0_4_4//:miniz_oxide",
],
"//conditions:default": [],
}), we would generate deps = selects.with_or({
(
"@rules_rust//rust/platform:whatever",
): [
"@raze__cfg_if__1_0_0//:cfg_if",
"@raze__crc32fast__1_2_1//:crc32fast",
"@raze__libc__0_2_86//:libc",
],
(
"@rules_rust//rust/platform:whatever_else",
): [
"@raze__cfg_if__1_0_0//:cfg_if",
"@raze__crc32fast__1_2_1//:crc32fast",
"@raze__libc__0_2_86//:libc",
],
(
"@rules_rust//rust/platform:wasm32-unknown-unknown",
"@rules_rust//rust/platform:wasm32-wasi",
): [
"@raze__cfg_if__1_0_0//:cfg_if",
"@raze__crc32fast__1_2_1//:crc32fast",
"@raze__libc__0_2_86//:libc",
"@raze__miniz_oxide__0_4_4//:miniz_oxide",
],
"//conditions:default": [],
}), Bazel today reasons about single selects fine, it's just the concatenation that it runs into trouble with. It's less documenting in what it generates, but it's generated code anyway, so... We could generate comments if we really wanted to make it easier to read... |
Hmm. While that solution would function, it wouldn't be great for my use case. I've added local crate and test rendering in my fork, patch for that coming later. My goal is to produce readable output, so that a project can convert to Bazel and drop Cargo--mostly for private monorepo-style things, where publishing crates is not a goal. Also, the comments in the code from bazelbuild/bazel#13785 seem to admit it is unsound, which seems bad. In any case, it looks like we can discuss bazelbuild/bazel#13785 elsewhere without blocking this PR. |
I'm generally good with the approach. Along the lines of what's being done in #415, it does seem imply I fixed the clippy and rustfmt errors upstream. |
This seems to address the comments so far. If you comment out the line that calls |
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.
Looks great. Thanks for running with that comment.
I'll add some tests if folks think this is the right approach. It doesn't fix every edge case I'm aware of, but adding coverage for this very common case would help move things forward. It's very easy to hit this bug if you enable a lot of platforms in raze settings. For example, this patch fixes #389 for me.