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

Don't allow default deps in planning of platform deps. #437

Merged
merged 4 commits into from
Aug 22, 2021

Conversation

sayrer
Copy link
Contributor

@sayrer sayrer commented Aug 18, 2021

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.

@sayrer
Copy link
Contributor Author

sayrer commented Aug 18, 2021

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.

@sayrer
Copy link
Contributor Author

sayrer commented Aug 18, 2021

It looks like most of the other problems of this kind I'm encountering are due to bazelbuild/bazel#13785

@illicitonion
Copy link
Collaborator

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 always be exactly one select, and to flatten/duplicate all of the default deps into all branches. So instead of:

    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...

@sayrer
Copy link
Contributor Author

sayrer commented Aug 19, 2021

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.

@dfreese
Copy link
Collaborator

dfreese commented Aug 19, 2021

I'm generally good with the approach. Along the lines of what's being done in #415, it does seem imply
dependencies, proc_macro_dependencies, build_dependencies, build_proc_macro_dependencies, and dev_dependencies in CrateDependenciesContext should be sets of some kind, probably BTreeSet, rather than hand implementing set subtraction.

I fixed the clippy and rustfmt errors upstream.

@sayrer
Copy link
Contributor Author

sayrer commented Aug 21, 2021

This seems to address the comments so far. If you comment out the line that calls subtract, the new test will fail.

Copy link
Collaborator

@dfreese dfreese left a 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.

@dfreese dfreese merged commit 8443486 into google:main Aug 22, 2021
@sayrer sayrer deleted the dedupe_deps branch August 22, 2021 09:18
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.

BUG: flate2 (1.0.20) deps are incorrectly generated
3 participants