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

[Feature] bzlmod & go.work #1731

Merged

Conversation

stefanpenner
Copy link
Contributor

@stefanpenner stefanpenner commented Feb 6, 2024

What type of PR is this?
Feature

What package or component does this PR mostly affect?
language/go

What does this PR do? Why is it needed?

Explores a possible approach for adding go.work support to the go bzlmod extension.

  1. adds go_deps.from_file(go_work = "//:go.work")
  2. adds ability to op-out of version conflict failures go_deps.from_file(go_work = "//:go.work, fail_on_version_conflict = False)
  3. parses go.work files to discover used go modules and replace statements
  4. generates repos for all deps specified in go.mod's referenced by the //:go.work
  5. handle replace statements (both at the go.work and the go.mod level)
  6. Errors with actionable error message if duplicate dependencies with differing versions arise (Question: can we do better?)

Which issues(s) does this PR fix?

Fixes #1720 #1549

Other notes for review

Although this does work, it should be considered a prototype intended for discussion and feedback. If we decide to pursue this or something similar, expect this PR to evolve rapidly. Although it has some tests, it will require more. Additionally, some architectural changes may improve readability.


An additional consideration, a complementary PR to teach gazelle about go.work + go.mods would help mitigate the dependency bleed problem this PR has. We should discuss this as well.

Copy link

google-cla bot commented Feb 6, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@stefanpenner
Copy link
Contributor Author

stefanpenner commented Feb 6, 2024

Although I have signed the CLA already, I have two user accounts in the commits. Let me fix that...

@stefanpenner stefanpenner changed the title [Exploration] bzlmod & go.work [For Discussion] bzlmod & go.work Feb 6, 2024
MODULE.bazel Outdated Show resolved Hide resolved
@stefanpenner stefanpenner force-pushed the hack-duplicate-go-deps-from-file branch 4 times, most recently from e522ebb to 86da7c5 Compare February 7, 2024 17:57
@stefanpenner
Copy link
Contributor Author

stefanpenner commented Feb 7, 2024

TODO (from call with team):

  • only support from_file(go_work = ...) from root modules. (check is_root property of the module)
  • double check that multiple from_file calls is disallowed
  • mandate that from_file is called with only one go_work, or go_mod attribute, and not none.
  • make tests/bcr use go.work
  • add additional unit test support
  • tidy-up

@stefanpenner stefanpenner force-pushed the hack-duplicate-go-deps-from-file branch 5 times, most recently from 82b70b3 to f7a7d32 Compare February 8, 2024 17:30
@stefanpenner
Copy link
Contributor Author

stefanpenner commented Feb 9, 2024

@stefanpenner stefanpenner force-pushed the hack-duplicate-go-deps-from-file branch 5 times, most recently from 30af2af to dc31480 Compare February 15, 2024 21:59
.bazelci/presubmit.yml Outdated Show resolved Hide resolved
.bazelversion Outdated Show resolved Hide resolved
@stefanpenner stefanpenner force-pushed the hack-duplicate-go-deps-from-file branch from dc31480 to d0dc074 Compare February 15, 2024 22:19
@stefanpenner
Copy link
Contributor Author

I've been thinking about how the resolutions work, and I want to confirm a few more more advanced scenarios. Let's hold off on merging for the moment.

I should be able to finish the testing by tomorrow, and report back. :)

internal/bzlmod/go_deps.bzl Outdated Show resolved Hide resolved
internal/bzlmod/go_mod.bzl Outdated Show resolved Hide resolved
internal/bzlmod/go_mod.bzl Outdated Show resolved Hide resolved
internal/bzlmod/go_mod.bzl Outdated Show resolved Hide resolved
internal/bzlmod/go_mod.bzl Outdated Show resolved Hide resolved
internal/bzlmod/go_mod.bzl Outdated Show resolved Hide resolved
@fmeum
Copy link
Member

fmeum commented Apr 16, 2024

Sounds good, remaining comments are all just nits in changed code.

@stefanpenner
Copy link
Contributor Author

I believe I have a reasonable resolution to the issue I noticed, working to implement the fix now. Let's see how it goes :)

* adds go_deps.from_file(go_work = "//:go.work")
* adds ability to op-out of version conflict failures go_deps.from_file(go_work = "//:go.work, fail_on_version_conflict = False)
* parses go.work files to discover used go modules and replace statements
* generates repos for all deps specified in go.mod's referenced by the //:go.work
* handle replace statements (both at the go.work and the go.mod level)
* Errors with actionable error message if duplicate dependencies with differing versions arise (Question: can we do better?)
* expand BCR test coverage to include both go.work and go.mod scenarios

Caveats:
* allows for dependency bleed between different go modules in the workspace - this is sorta by design for this stepping stone, but is ideal
* doesn't support replace statements in go.work yet (but that shouldn't be to hard to add in a follow up)
@stefanpenner stefanpenner force-pushed the hack-duplicate-go-deps-from-file branch from ae2935a to c292df3 Compare April 18, 2024 18:01
@stefanpenner
Copy link
Contributor Author

stefanpenner commented Apr 18, 2024

I believe I have a reasonable resolution to the issue I noticed, working to implement the fix now. Let's see how it goes :)

TL;DR: Within a go.work, if module A depends on an external module B (not in go.work), and B in turn depends on module C which is included in the go.work, then the version of C referenced by B will differ from the version of C that is directly included in the go.work.

Our team has been describing this the "the sandwich problem".

In general, a well formed workspace must not contain the the sandwich problem as even in a perfectly functioning system this scenario will be a source of headaches and confusion.

I did spend some time seeing if we can easily address this but with my limited understanding of gazelle I couldn't come up with a reasonable solution that didn't result in a whole new feature. That being said, someone with more expertise may have a good idea here that I'm missing.

Given this, my recommendation is:

  1. proceed with this PR
  2. separately explore how to enable this capability within gazelle and then add this capability in the future.

@fmeum
Copy link
Member

fmeum commented Apr 18, 2024

Agreed, let's merge this and as a follow-up:

  1. Resolve the problem you mentioned.
  2. Add docs to our Bzlmod guide.

@fmeum
Copy link
Member

fmeum commented Apr 18, 2024

Thanks for working on this large project!

@fmeum fmeum merged commit 908ba74 into bazel-contrib:master Apr 18, 2024
15 checks passed
stefanpenner added a commit to stefanpenner/rules_go that referenced this pull request May 6, 2024
stefanpenner added a commit to stefanpenner/rules_go that referenced this pull request May 6, 2024
stefanpenner added a commit to stefanpenner/rules_go that referenced this pull request May 6, 2024
stefanpenner added a commit to stefanpenner/rules_go that referenced this pull request May 6, 2024
stefanpenner added a commit to stefanpenner/rules_go that referenced this pull request May 7, 2024
stefanpenner added a commit to stefanpenner/rules_go that referenced this pull request May 7, 2024
stefanpenner added a commit to stefanpenner/rules_go that referenced this pull request May 7, 2024
stefanpenner added a commit to stefanpenner/rules_go that referenced this pull request May 7, 2024
stefanpenner added a commit to stefanpenner/rules_go that referenced this pull request May 7, 2024
stefanpenner added a commit to stefanpenner/rules_go that referenced this pull request May 7, 2024
stefanpenner added a commit to stefanpenner/rules_go that referenced this pull request May 7, 2024
stefanpenner added a commit to stefanpenner/rules_go that referenced this pull request May 7, 2024
stefanpenner added a commit to stefanpenner/rules_go that referenced this pull request May 7, 2024
stefanpenner added a commit to stefanpenner/rules_go that referenced this pull request May 7, 2024
fmeum pushed a commit to bazel-contrib/rules_go that referenced this pull request May 7, 2024
renovate bot referenced this pull request in kreempuff/rules_unreal_engine Aug 1, 2024
[![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 | minor | `v0.36.0` -> `v0.38.0` |

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>bazelbuild/bazel-gazelle (bazel_gazelle)</summary>

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

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

#### What's Changed

- Add support for `include()` in `MODULE.bazel` by
[@&#8203;fmeum](https://togithub.com/fmeum) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1810](https://togithub.com/bazelbuild/bazel-gazelle/pull/1810)
- feat: gazelle_test test rule by
[@&#8203;hunshcn](https://togithub.com/hunshcn) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1785](https://togithub.com/bazelbuild/bazel-gazelle/pull/1785)
- Handle arm64 host platform for MacOS by
[@&#8203;smocherla-brex](https://togithub.com/smocherla-brex) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1817](https://togithub.com/bazelbuild/bazel-gazelle/pull/1817)
- go_repository: add 'clean' build_file_generation by
[@&#8203;TvdW](https://togithub.com/TvdW) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1802](https://togithub.com/bazelbuild/bazel-gazelle/pull/1802)
- fix: support leading ./ in .bazelignore by
[@&#8203;jbedard](https://togithub.com/jbedard) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1828](https://togithub.com/bazelbuild/bazel-gazelle/pull/1828)
- Restore compatibility with Go 1.18 by
[@&#8203;fmeum](https://togithub.com/fmeum) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1833](https://togithub.com/bazelbuild/bazel-gazelle/pull/1833)
- Remove reliance on specific canonical repo name scheme by
[@&#8203;fmeum](https://togithub.com/fmeum) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1835](https://togithub.com/bazelbuild/bazel-gazelle/pull/1835)
- temporarily disable `//internal:bazel_test` on Mac to fix CI by
[@&#8203;tyler-french](https://togithub.com/tyler-french) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1842](https://togithub.com/bazelbuild/bazel-gazelle/pull/1842)
- update readmes for latest release by
[@&#8203;tyler-french](https://togithub.com/tyler-french) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1840](https://togithub.com/bazelbuild/bazel-gazelle/pull/1840)
- \[Gazelle] Fix Duplicate Load Bug by
[@&#8203;ckilian867](https://togithub.com/ckilian867) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1841](https://togithub.com/bazelbuild/bazel-gazelle/pull/1841)
- \[Proto] Require space between 'service' and service name in regex
matching by [@&#8203;ckilian867](https://togithub.com/ckilian867) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1845](https://togithub.com/bazelbuild/bazel-gazelle/pull/1845)
- \[Proto] Keep track of the names of Services, Messages, and Enums by
[@&#8203;ckilian867](https://togithub.com/ckilian867) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1844](https://togithub.com/bazelbuild/bazel-gazelle/pull/1844)
- Always check files in generation tests by
[@&#8203;Whoaa512](https://togithub.com/Whoaa512) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1847](https://togithub.com/bazelbuild/bazel-gazelle/pull/1847)
- Support label using regexp in directive `gazelle:resolve_regexp` by
[@&#8203;lkassar-stripe](https://togithub.com/lkassar-stripe) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1822](https://togithub.com/bazelbuild/bazel-gazelle/pull/1822)
- Add `external/...` prefix to `${SRCDIR}` in external repos by
[@&#8203;fmeum](https://togithub.com/fmeum) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1850](https://togithub.com/bazelbuild/bazel-gazelle/pull/1850)
- feat(tools): add a tool to automate the generation of go_deps
overrides by [@&#8203;tyler-french](https://togithub.com/tyler-french)
in
[https://github.com/bazelbuild/bazel-gazelle/pull/1677](https://togithub.com/bazelbuild/bazel-gazelle/pull/1677)
- prepare release 0.38 by
[@&#8203;tyler-french](https://togithub.com/tyler-french) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1849](https://togithub.com/bazelbuild/bazel-gazelle/pull/1849)

#### New Contributors

- [@&#8203;smocherla-brex](https://togithub.com/smocherla-brex) made
their first contribution in
[https://github.com/bazelbuild/bazel-gazelle/pull/1817](https://togithub.com/bazelbuild/bazel-gazelle/pull/1817)
- [@&#8203;TvdW](https://togithub.com/TvdW) made their first
contribution in
[https://github.com/bazelbuild/bazel-gazelle/pull/1802](https://togithub.com/bazelbuild/bazel-gazelle/pull/1802)
- [@&#8203;ckilian867](https://togithub.com/ckilian867) made their first
contribution in
[https://github.com/bazelbuild/bazel-gazelle/pull/1841](https://togithub.com/bazelbuild/bazel-gazelle/pull/1841)
- [@&#8203;lkassar-stripe](https://togithub.com/lkassar-stripe) made
their first contribution in
[https://github.com/bazelbuild/bazel-gazelle/pull/1822](https://togithub.com/bazelbuild/bazel-gazelle/pull/1822)

**Full Changelog**:
bazel-contrib/bazel-gazelle@v0.37.0...v0.38.0

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

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

#### What's Changed

- Apply map_kind to args as well as rule kinds by
[@&#8203;illicitonion](https://togithub.com/illicitonion) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1722](https://togithub.com/bazelbuild/bazel-gazelle/pull/1722)
- Add a pointer to bzlmod guide by
[@&#8203;sluongng](https://togithub.com/sluongng) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1781](https://togithub.com/bazelbuild/bazel-gazelle/pull/1781)
- \[Extraction] prep for go.mod & go.work FilePath ReplaceDirective work
by [@&#8203;stefanpenner](https://togithub.com/stefanpenner) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1780](https://togithub.com/bazelbuild/bazel-gazelle/pull/1780)
- \[cmd/fetch_repo] make cache corruption failures more clear by
[@&#8203;tyler-french](https://togithub.com/tyler-french) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1782](https://togithub.com/bazelbuild/bazel-gazelle/pull/1782)
- Nit: pass -modcacherw in exec.Command. by
[@&#8203;hauserx](https://togithub.com/hauserx) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1784](https://togithub.com/bazelbuild/bazel-gazelle/pull/1784)
- Mention JS extension in Aspect CLI by
[@&#8203;alexeagle](https://togithub.com/alexeagle) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1646](https://togithub.com/bazelbuild/bazel-gazelle/pull/1646)
- \[Feature] bzlmod & go.work by
[@&#8203;stefanpenner](https://togithub.com/stefanpenner) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1731](https://togithub.com/bazelbuild/bazel-gazelle/pull/1731)
- Add GIT_CONFIG_\* env vars to go_repository allow-list by
[@&#8203;mortenmj](https://togithub.com/mortenmj) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1791](https://togithub.com/bazelbuild/bazel-gazelle/pull/1791)
- Reformat with latest buildifier by
[@&#8203;fmeum](https://togithub.com/fmeum) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1792](https://togithub.com/bazelbuild/bazel-gazelle/pull/1792)
- \[Feature] go.mod FilePath ReplaceDirective Support by
[@&#8203;stefanpenner](https://togithub.com/stefanpenner) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1776](https://togithub.com/bazelbuild/bazel-gazelle/pull/1776)
- Fix README.rst by
[@&#8203;AugustKarlstedt](https://togithub.com/AugustKarlstedt) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1795](https://togithub.com/bazelbuild/bazel-gazelle/pull/1795)
- Update README.rst by
[@&#8203;AugustKarlstedt](https://togithub.com/AugustKarlstedt) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1796](https://togithub.com/bazelbuild/bazel-gazelle/pull/1796)
- Normalise newlines on Windows by
[@&#8203;illicitonion](https://togithub.com/illicitonion) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1798](https://togithub.com/bazelbuild/bazel-gazelle/pull/1798)
- Fix go.work use ROOT moddir by
[@&#8203;hunshcn](https://togithub.com/hunshcn) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1800](https://togithub.com/bazelbuild/bazel-gazelle/pull/1800)
- allow go_visibility directive to change command package's visibility
by [@&#8203;hunshcn](https://togithub.com/hunshcn) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1794](https://togithub.com/bazelbuild/bazel-gazelle/pull/1794)
- Ensure the Gazelle binary is built for the right platform by
[@&#8203;EdSchouten](https://togithub.com/EdSchouten) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1803](https://togithub.com/bazelbuild/bazel-gazelle/pull/1803)
- Add support for `debug_mode` option to `go_deps` by
[@&#8203;davidbyttow](https://togithub.com/davidbyttow) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1806](https://togithub.com/bazelbuild/bazel-gazelle/pull/1806)
- Remove special resolution of go_proto imports by
[@&#8203;linzhp](https://togithub.com/linzhp) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1807](https://togithub.com/bazelbuild/bazel-gazelle/pull/1807)
- address nogo complaints about variable shadowing by
[@&#8203;pmenglund](https://togithub.com/pmenglund) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1808](https://togithub.com/bazelbuild/bazel-gazelle/pull/1808)
- Make `# gazelle:proto file` work without needing to set different
`option go_package` in .proto files by
[@&#8203;jeromep-stripe](https://togithub.com/jeromep-stripe) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1765](https://togithub.com/bazelbuild/bazel-gazelle/pull/1765)
- go_deps: ignore go.work toolchain directive by
[@&#8203;malt3](https://togithub.com/malt3) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1809](https://togithub.com/bazelbuild/bazel-gazelle/pull/1809)
- prepare release 0.37.0 by
[@&#8203;tyler-french](https://togithub.com/tyler-french) in
[https://github.com/bazelbuild/bazel-gazelle/pull/1812](https://togithub.com/bazelbuild/bazel-gazelle/pull/1812)

#### New Contributors

- [@&#8203;stefanpenner](https://togithub.com/stefanpenner) made their
first contribution in
[https://github.com/bazelbuild/bazel-gazelle/pull/1780](https://togithub.com/bazelbuild/bazel-gazelle/pull/1780)
- [@&#8203;AugustKarlstedt](https://togithub.com/AugustKarlstedt) made
their first contribution in
[https://github.com/bazelbuild/bazel-gazelle/pull/1795](https://togithub.com/bazelbuild/bazel-gazelle/pull/1795)
- [@&#8203;hunshcn](https://togithub.com/hunshcn) made their first
contribution in
[https://github.com/bazelbuild/bazel-gazelle/pull/1800](https://togithub.com/bazelbuild/bazel-gazelle/pull/1800)
- [@&#8203;EdSchouten](https://togithub.com/EdSchouten) made their first
contribution in
[https://github.com/bazelbuild/bazel-gazelle/pull/1803](https://togithub.com/bazelbuild/bazel-gazelle/pull/1803)
- [@&#8203;davidbyttow](https://togithub.com/davidbyttow) made their
first contribution in
[https://github.com/bazelbuild/bazel-gazelle/pull/1806](https://togithub.com/bazelbuild/bazel-gazelle/pull/1806)
- [@&#8203;pmenglund](https://togithub.com/pmenglund) made their first
contribution in
[https://github.com/bazelbuild/bazel-gazelle/pull/1808](https://togithub.com/bazelbuild/bazel-gazelle/pull/1808)
- [@&#8203;jeromep-stripe](https://togithub.com/jeromep-stripe) made
their first contribution in
[https://github.com/bazelbuild/bazel-gazelle/pull/1765](https://togithub.com/bazelbuild/bazel-gazelle/pull/1765)

**Full Changelog**:
bazel-contrib/bazel-gazelle@v0.36.0...v0.37.0

</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 was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job
log](https://developer.mend.io/github/kreempuff/rules_unreal_engine).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zNjguMTAiLCJ1cGRhdGVkSW5WZXIiOiIzNy40NDAuNyIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->
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.

Support Golang Workspaces in Gazelle
5 participants