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

Consolidate duplicated targeted dependencies. #512

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

sayrer
Copy link
Contributor

@sayrer sayrer commented Jun 23, 2022

This should actually fix #451.

@sayrer
Copy link
Contributor Author

sayrer commented Jun 24, 2022

Hmm, I guess this will fix the problem, but it will create more select blocks than necessary. It will end up looking pretty similar to the bottom of features.rs, I'll see if it's worth making it generic.

@sayrer sayrer force-pushed the consolidate_targeted_deps branch from 7328caf to 455761f Compare June 26, 2022 19:23
impl/src/context.rs Outdated Show resolved Hide resolved
@sayrer
Copy link
Contributor Author

sayrer commented Jun 27, 2022

Most of the changes after bootstrapping are just innocuous alphabetization. But there are a few problems still there:

third_party/cargo/remote/BUILD.async-std-1.9.0.bazel (wasm32-wasi is newly missing)
third_party/cargo/remote/BUILD.rand-0.8.3.bazel (libc is missing)
third_party/cargo/remote/BUILD.reqwest-0.11.2.bazel (windows reqwest dependencies)

After those are resolved, it will make sense to eliminate platform select blocks with no members (this can happen if common dependencies are hoisted out of selects in an effort to avoid repetition)

@sayrer
Copy link
Contributor Author

sayrer commented Jun 28, 2022

This latest revision fixed all of the bootstrapping problems I found above.

@gferon
Copy link

gferon commented Sep 26, 2022

I'm having the same issue as described #483 with quanta. @sayrer Is there anyway I can help with pushing this PR to the finish line? (more testing for example).

@sayrer
Copy link
Contributor Author

sayrer commented Sep 28, 2022

I'm having the same issue as described #483 with quanta. @sayrer Is there anyway I can help with pushing this PR to the finish line? (more testing for example).

I think we're mostly looking at reviewer bandwidth at the moment. I've been talking it over and have a way forward.

@UebelAndre UebelAndre removed their request for review October 14, 2022 17:36
dmitrii-ubskii added a commit to typedb/dependencies that referenced this pull request Nov 23, 2022
## What is the goal of this PR?

We add the cucumber testing framework crate to our pinned rust
dependencies.

## What are the changes implemented in this PR?

While making this change, we encountered an issue in bazel
(bazelbuild/bazel#13785) that made it throw an
error because, in essence, libc was included in two out of three
conditions, which was regarded as a dependency duplication.

Cargo-raze is aware of this issue
(google/cargo-raze#451) and has their own
workaround in the works (google/cargo-raze#512)
which is not being reviewed or merged due to reviewer bandwidth issues.

As our own workaround, we separate the inclusion of libc into its own
condition, overriding raze and pacifying bazel. This change is stored
separately from the razed crates and reapplied when
`library/crates/update.sh` is run.
jamesreprise pushed a commit to jamesreprise/vaticle-dependencies that referenced this pull request Dec 5, 2022
## What is the goal of this PR?

We add the cucumber testing framework crate to our pinned rust
dependencies.

## What are the changes implemented in this PR?

While making this change, we encountered an issue in bazel
(bazelbuild/bazel#13785) that made it throw an
error because, in essence, libc was included in two out of three
conditions, which was regarded as a dependency duplication.

Cargo-raze is aware of this issue
(google/cargo-raze#451) and has their own
workaround in the works (google/cargo-raze#512)
which is not being reviewed or merged due to reviewer bandwidth issues.

As our own workaround, we separate the inclusion of libc into its own
condition, overriding raze and pacifying bazel. This change is stored
separately from the razed crates and reapplied when
`library/crates/update.sh` is run.
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.

Duplicated deps when multiple targets share dependencies
2 participants