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

sluongng/upgrade rules go #3699

Closed
wants to merge 4 commits into from
Closed

sluongng/upgrade rules go #3699

wants to merge 4 commits into from

Conversation

sluongng
Copy link
Contributor

@sluongng sluongng commented Apr 1, 2023

  • Upgraded rules_go to v0.39.0
  • Upgraded gazelle to v0.30.0
    • Patch visibility of @gazelle//internal/module to build bb CLI
  • Upgrade several golang.org/x/... packages

This also required us to change how we are registering go_sdk. An issue
was reported upstream here bazel-contrib/rules_go#3510

Then, run the new gazelle over the repo.

Fix all the visibility scope issues that were reported in
bazel-contrib/bazel-gazelle#1467

Please let me know if this PR is too big, I could split it into 2-3 smaller PRs


Version bump: Patch

- Upgraded rules_go to v0.39.0
- Upgraded gazelle to v0.30.0
  + Patch visibility of @gazelle//internal/module to build `bb` CLI
- Upgraded go to 1.20.2
- Upgrade several golang.org/x/... packages

This also required us to change how we are registering go_sdk. An issue
was reported upstream here bazel-contrib/rules_go#3510
@sluongng sluongng force-pushed the sluongng/upgrade-rules-go branch 2 times, most recently from c28a07f to e02ec33 Compare April 1, 2023 05:54
@sluongng
Copy link
Contributor Author

sluongng commented Apr 1, 2023

Avoided upgrading go version to 1.20.2. There seem to be several breakages with the new go version.

@sluongng sluongng marked this pull request as ready for review April 1, 2023 06:27
@sluongng sluongng requested review from tempoz and bduffany April 1, 2023 06:27
@sluongng
Copy link
Contributor Author

sluongng commented Apr 1, 2023

Not sure how to get past the lint check as we seem to be using an older release of bb CLI.

@bduffany do you know if we should build the bb CLI from HEAD to run checkstyle?

WORKSPACE Outdated
@@ -50,36 +50,9 @@ load("@io_bazel_rules_go//go:deps.bzl", "go_download_sdk", "go_register_toolchai

go_rules_dependencies()

Copy link
Member

@bduffany bduffany Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there was some reason we wanted to use go_download_sdk instead of go_register_toolchains, maybe related to cross-compilation. Maybe @siggisim remembers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we want to make sure to be able to download both the linux and mac sdks regardless of host (the default rules only download the SDK from your host machine). This allows you to kick of linux remote execution builds from a mac host, i.e. https://www.buildbuddy.dev/debugging#run-buildbuddy-test-suite-on-linux-remote-ex-from-a-mac

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a fix in bazel-contrib/rules_go#3511 and patched our rules_go with it in the latest version.

Since the rules_go PR is merged, it should be a short-lived patch.

@sluongng
Copy link
Contributor Author

@bduffany wdyt about the Check Style failure here? We are using a pinned version of bb CLI instead of building //cli from source. It made the check using the old gazelle package and failed.

Should the fix be to build //cli from source?

or should we upgrade with old style -> release bb -> bump bb version + fix style? 🤔

@bduffany
Copy link
Member

bduffany commented Apr 12, 2023

upgrade with old style -> release bb -> bump bb version + fix style

this is probably the best option for now. Eventually we want to move checkstyle to bb workflows, and it'll be more feasible to build bb from source. Building bb from source on GH actions is probably too slow.

@sluongng
Copy link
Contributor Author

close in favor of #3768

@sluongng sluongng closed this Apr 15, 2023
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.

3 participants