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

Let bazel_deps replace Go deps #1526

Merged
merged 4 commits into from Jun 2, 2023
Merged

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented May 4, 2023

When a Go dependency is brought in both as a bazel_dep and as a
regular Go dependency via e.g. go.mod, the bazel_dep repo should be
used instead of creating a separate go_repository, which would risk
duplicate packages.

This is realized by registering every Bazel module with a
go_deps.from_file tag as providing the Go module of the same version
with the module path given in go.mod.

Gazelle is aware of these tags and generates either a canonical label or
a label using the apparent module repo name (the latter only for the
root module and only if it directly depends on the bazel_dep).

Fixes #1525
Fixes #1543

@fmeum fmeum force-pushed the bazel_dep_override branch 5 times, most recently from dc9b88c to a6b2fbe Compare May 4, 2023 17:00
@fmeum fmeum marked this pull request as ready for review May 4, 2023 17:10
@fmeum
Copy link
Collaborator Author

fmeum commented May 4, 2023

@Wyverald @tyler-french Could you review?

I implemented two different approaches (first commit vs. both commits), with one making the association explicit and one deriving it from go_deps.from_file. I prefer the latter, but in case you see issues with this association being made implicitly, we can switch back to the first approach.

@fmeum
Copy link
Collaborator Author

fmeum commented May 4, 2023

We probably need to introduce some kind of version resolution to ensure that outdated bazel_deps do not override up-to-date go.mod deps.

@fmeum
Copy link
Collaborator Author

fmeum commented May 22, 2023

I added version resolution.

@tyler-french @seh Could you take a look if time permits?

@seh
Copy link
Contributor

seh commented May 23, 2023

Could you take a look if time permits?

I read it all, but I need to think about both the problem and the solution more before I have anything constructive to add.

@fmeum
Copy link
Collaborator Author

fmeum commented May 23, 2023

@seh Thank, that's exactly what I'm looking for. Take your time!

@tyler-french
Copy link
Contributor

Tested at Uber

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 2, 2023

I rebased onto master and pushed a new commit that adds support for the generalized semver format used by Bazel module registries.

I will wait for bazelbuild/bazel-central-registry#674 and add a test based on it before merging.

@fmeum fmeum force-pushed the bazel_dep_override branch 2 times, most recently from 03e4809 to 060f8c0 Compare June 2, 2023 13:13
fmeum added 4 commits June 2, 2023 15:15
When a Go dependency is brought in both as a `bazel_dep` and as a
regular Go dependency via e.g. go.mod, the `bazel_dep` repo should be
used instead of creating a separate `go_repository`, which would risk
duplicate packages.

This is realized by registering every Bazel module with a
`go_deps.from_file` tag as providing the Go module of the same version
with the module path given in `go.mod`.

Gazelle is aware of these tags and generates either a canonical label or
 a label using the apparent module repo name (the latter only for the
root module and only if it directly depends on the `bazel_dep`).
@fmeum
Copy link
Collaborator Author

fmeum commented Jun 2, 2023

@seh I added a more realistic example of how this would be used in the form of circl.

@seh
Copy link
Contributor

seh commented Jun 2, 2023

I added a more realistic example of how this would be used in the form of circl.

I hadn't seen a module like circl before, where there's an upstream Go module with no "awareness" of Bazel, but then we publish a Bazel module that adds in all the necessary Bazel control files to allow building and testing the library.

Now, coming back to this patch, I understand it better now, in that we're reading all the uses of the go_dep extension's nomination of Go modules from non-root/child Bazel modules and taking them into consideration when choosing the maximum version for each Go module.

The description mentions a "provides" tag on the go_dep extension. Is that implemented?

@fmeum fmeum changed the title Allow bazel_deps to replace Go deps Let bazel_deps replace Go deps Jun 2, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Jun 2, 2023

@seh Sorry, the PR description was outdated, updated it to the description of the first commit.

@seh
Copy link
Contributor

seh commented Jun 2, 2023

I hadn't seen a module like circl before, where there's an upstream Go module with no "awareness" of Bazel, but then we publish a Bazel module that adds in all the necessary Bazel control files to allow building and testing the library.

What's not clear to me yet is why one would express such a Bazel module dependency, as opposed to just letting the go.mod file express the dependency and rely on go_deps.from_file to take care of the rest. It seems like expressing the same idea twice, when our goal with the project is to eliminate as much of the redundant Bazel configuration as possible (e.g. getting rid of the need for use_repo with go_deps).

Are we getting more from that Bazel module than what Gazelle would generate on its own by reading the go.mod file?

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 2, 2023

Having to use a Bazel module as a Go module should be rare, but there are important exceptions:

When a Go module requires patches beyond Gazelle's capabilities. A go_deps.module_override can fix that, but makes it so that the Go module can no longer be used by non-root Bazel modules. We could have a separate registry for patches similar to what we do to centralize go_deps.gazelle_overrides, but that seems pretty complex to set up while the BCR provides this out of the box.

The particular case of circl is interesting here as it requires adding cc_library targets, but those will eventually have to be loaded from rules_cc, which in turn requires visibility into this Bazel module - go_deps-generated repos don't have this, only bazel_deps can declare additional dependencies in this way.

Another use case is for something like bazelbuild/buildtools, which provides both Starlark rules, Go tools and Go libraries. Most Bazel users will want the first two and thus probably a bazel_dep, but anyone who consumes the Go library wants to do so via go_deps. We should probably ensure that there aren't two different repos floating around that provide the same Go targets but potentially in different versions.

@seh
Copy link
Contributor

seh commented Jun 2, 2023

there are important exceptions:

Thank you for the detailed explanation. The dependency landscape is more complex than I had considered.

@aaronmondal
Copy link

I've just tested this for rules_ll which uses that exact circl dependency as a transitive dependency. We were previously running into an issue where users could not only not import our go tooling, but couldn't import rules_ll at all because the go toolchain misconfiguration would block the entire build.

Keeping patches like the ones required for circl maintained isn't nice if it requires custom deps and overrides. This patch fixes these issues in an IMO very elegant manner.

So this will also fix #1543 in a very satisfactory way ❤️

@fmeum fmeum merged commit 7feffe1 into bazelbuild:master Jun 2, 2023
10 checks passed
@fmeum fmeum deleted the bazel_dep_override branch June 3, 2023 05:44
renovate bot added a commit to kreempuff/rules_unreal_engine that referenced this pull request Jun 13, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [bazel_gazelle](https://togithub.com/bazelbuild/bazel-gazelle) |
http_archive | patch | `v0.31.0` -> `v0.31.1` |

---

### ⚠ Dependency Lookup Warnings ⚠

Warnings were logged while processing this repo. Please check the
Dependency Dashboard for more information.

---

### Release Notes

<details>
<summary>bazelbuild/bazel-gazelle</summary>

###
[`v0.31.1`](https://togithub.com/bazelbuild/bazel-gazelle/releases/tag/v0.31.1)

[Compare
Source](https://togithub.com/bazelbuild/bazel-gazelle/compare/v0.31.0...v0.31.1)

#### What's Changed

- point sync.Once in walkConfig by
[@&#8203;jmhodges](https://togithub.com/jmhodges) in
[bazelbuild/bazel-gazelle#1532
- add copylock vet to nogo by
[@&#8203;jmhodges](https://togithub.com/jmhodges) in
[bazelbuild/bazel-gazelle#1534
- bzlmod: Remove deprecated override attributes on `go_deps.module` by
[@&#8203;fmeum](https://togithub.com/fmeum) in
[bazelbuild/bazel-gazelle#1548
- Add default directives for github.com/envoyproxy/protoc-gen-validate
by [@&#8203;mortenmj](https://togithub.com/mortenmj) in
[bazelbuild/bazel-gazelle#1553
- cmd/gazelle: do not use the epoch as timestamp in diff output by
[@&#8203;siddharthab](https://togithub.com/siddharthab) in
[bazelbuild/bazel-gazelle#1552
- fileinfo: fix not detecting 'unix' files to be OS specific by
[@&#8203;sluongng](https://togithub.com/sluongng) in
[bazelbuild/bazel-gazelle#1554
- language/go: Emit apparent repo name of rules_go in select keys by
[@&#8203;fmeum](https://togithub.com/fmeum) in
[bazelbuild/bazel-gazelle#1555
- Let `bazel_dep`s replace Go deps by
[@&#8203;fmeum](https://togithub.com/fmeum) in
[bazelbuild/bazel-gazelle#1526

#### New Contributors

- [@&#8203;mortenmj](https://togithub.com/mortenmj) made their first
contribution in
[bazelbuild/bazel-gazelle#1553

**Full Changelog**:
bazelbuild/bazel-gazelle@v0.31.0...v0.31.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/kreempuff/rules_unreal_engine).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMTAuMCIsInVwZGF0ZWRJblZlciI6IjM1LjExMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
renovate bot added a commit to cgrindel/rules_swift_package_manager that referenced this pull request Jun 13, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [bazel_gazelle](https://togithub.com/bazelbuild/bazel-gazelle) |
http_archive | patch | `v0.31.0` -> `v0.31.1` |

---

### Release Notes

<details>
<summary>bazelbuild/bazel-gazelle</summary>

###
[`v0.31.1`](https://togithub.com/bazelbuild/bazel-gazelle/releases/tag/v0.31.1)

[Compare
Source](https://togithub.com/bazelbuild/bazel-gazelle/compare/v0.31.0...v0.31.1)

#### What's Changed

- point sync.Once in walkConfig by
[@&#8203;jmhodges](https://togithub.com/jmhodges) in
[bazelbuild/bazel-gazelle#1532
- add copylock vet to nogo by
[@&#8203;jmhodges](https://togithub.com/jmhodges) in
[bazelbuild/bazel-gazelle#1534
- bzlmod: Remove deprecated override attributes on `go_deps.module` by
[@&#8203;fmeum](https://togithub.com/fmeum) in
[bazelbuild/bazel-gazelle#1548
- Add default directives for github.com/envoyproxy/protoc-gen-validate
by [@&#8203;mortenmj](https://togithub.com/mortenmj) in
[bazelbuild/bazel-gazelle#1553
- cmd/gazelle: do not use the epoch as timestamp in diff output by
[@&#8203;siddharthab](https://togithub.com/siddharthab) in
[bazelbuild/bazel-gazelle#1552
- fileinfo: fix not detecting 'unix' files to be OS specific by
[@&#8203;sluongng](https://togithub.com/sluongng) in
[bazelbuild/bazel-gazelle#1554
- language/go: Emit apparent repo name of rules_go in select keys by
[@&#8203;fmeum](https://togithub.com/fmeum) in
[bazelbuild/bazel-gazelle#1555
- Let `bazel_dep`s replace Go deps by
[@&#8203;fmeum](https://togithub.com/fmeum) in
[bazelbuild/bazel-gazelle#1526

#### New Contributors

- [@&#8203;mortenmj](https://togithub.com/mortenmj) made their first
contribution in
[bazelbuild/bazel-gazelle#1553

**Full Changelog**:
bazelbuild/bazel-gazelle@v0.31.0...v0.31.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/cgrindel/rules_swift_package_manager).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMTAuMCIsInVwZGF0ZWRJblZlciI6IjM1LjExMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
renovate bot added a commit to cgrindel/bazel-starlib that referenced this pull request Jun 13, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [bazel_gazelle](https://togithub.com/bazelbuild/bazel-gazelle) |
http_archive | patch | `v0.31.0` -> `v0.31.1` |

---

### Release Notes

<details>
<summary>bazelbuild/bazel-gazelle</summary>

###
[`v0.31.1`](https://togithub.com/bazelbuild/bazel-gazelle/releases/tag/v0.31.1)

[Compare
Source](https://togithub.com/bazelbuild/bazel-gazelle/compare/v0.31.0...v0.31.1)

#### What's Changed

- point sync.Once in walkConfig by
[@&#8203;jmhodges](https://togithub.com/jmhodges) in
[bazelbuild/bazel-gazelle#1532
- add copylock vet to nogo by
[@&#8203;jmhodges](https://togithub.com/jmhodges) in
[bazelbuild/bazel-gazelle#1534
- bzlmod: Remove deprecated override attributes on `go_deps.module` by
[@&#8203;fmeum](https://togithub.com/fmeum) in
[bazelbuild/bazel-gazelle#1548
- Add default directives for github.com/envoyproxy/protoc-gen-validate
by [@&#8203;mortenmj](https://togithub.com/mortenmj) in
[bazelbuild/bazel-gazelle#1553
- cmd/gazelle: do not use the epoch as timestamp in diff output by
[@&#8203;siddharthab](https://togithub.com/siddharthab) in
[bazelbuild/bazel-gazelle#1552
- fileinfo: fix not detecting 'unix' files to be OS specific by
[@&#8203;sluongng](https://togithub.com/sluongng) in
[bazelbuild/bazel-gazelle#1554
- language/go: Emit apparent repo name of rules_go in select keys by
[@&#8203;fmeum](https://togithub.com/fmeum) in
[bazelbuild/bazel-gazelle#1555
- Let `bazel_dep`s replace Go deps by
[@&#8203;fmeum](https://togithub.com/fmeum) in
[bazelbuild/bazel-gazelle#1526

#### New Contributors

- [@&#8203;mortenmj](https://togithub.com/mortenmj) made their first
contribution in
[bazelbuild/bazel-gazelle#1553

**Full Changelog**:
bazelbuild/bazel-gazelle@v0.31.0...v0.31.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/cgrindel/bazel-starlib).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMTAuMCIsInVwZGF0ZWRJblZlciI6IjM1LjExMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants