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

Fix use of //go with Bzlmod by removing dependency on @org_golang_x_sys #3509

Closed
wants to merge 0 commits into from
Closed

Conversation

bricedp
Copy link
Contributor

@bricedp bricedp commented Apr 1, 2023

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Running bazel run //go (or bazel run @rules_go//go from another module) fails when using Bzlmod because the @org_golang_x_sys repository is not visible. Without Bzlmod, the repository is loaded through a WORKSPACE dependency and the error does not happen.

Related to these recent changes:

Two-step minimal reproduction from root of this repo:

  1. touch WORKSPACE.bzlmod
  2. bazel run --enable_bzlmod //go

Here's the error that produces:

$ bazel run --enable_bzlmod //go
DEBUG: [email protected]/MODULE.bazel:7:6: WARNING: The bazel_gazelle Bazel module is still highly experimental and subject to change at any time. Only use it to try out bzlmod for now.
WARNING: For repository 'com_google_protobuf', the root module requires module version [email protected], but got [email protected] in the resolved dependency graph.
ERROR: /tmp/rules_go/go/tools/go_bin_runner/BUILD.bazel:11:11: no such package '@[unknown repo 'org_golang_x_sys' requested from @]//unix': The repository '@[unknown repo 'org_golang_x_sys' requested from @]' could not be resolved: No repository visible as '@org_golang_x_sys' from main repository and referenced by '//go/tools/go_bin_runner:go_bin_runner_lib'
ERROR: Analysis of target '//go:go' failed; build aborted: 
INFO: Elapsed time: 3.043s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (45 packages loaded, 865 targets configured)
    currently loading: go/platform
ERROR: Build failed. Not running target

Here's the point in the BUILD file where this repository is being referenced:

"//conditions:default": ["@org_golang_x_sys//unix"],

Which issues(s) does this PR fix?

None

Other notes for review

@fmeum
Copy link
Collaborator

fmeum commented Apr 1, 2023

Thanks for the report and fix.

This dependency is indeed missing. In fact, even with your fix, we only pick it up indirectly from Gazelle as it's not declared via go_deps in rules_go's MODULE.bazel file.

Given that golang.org/x/sys isn't promising backwards compatibility, I think that we may just be better of getting rid of this direct dependency - it isn't even clear that it is better than the pure Go alternative.

@bricedp Could you try deleting process_unix.go and the references to it and golang.org/x/sys in

"//conditions:default": ["process_unix.go"],
? If that works, I think I would prefer that fix.

@bricedp
Copy link
Contributor Author

bricedp commented Apr 1, 2023

Thanks @fmeum, I agree that's much cleaner. I've removed process_unix.go and updated the BUILD file — I'm able to run //go without issue.

If ever needed for the future, I think the @org_golang_x_sys I had added was being brought in by the explicit call to go_rules_dependencies, which appears to be a newer version than the indirect dependency in go.mod.

go_rules_dependencies(force = True)

wrapper(
http_archive,
name = "org_golang_x_sys",
# v0.6.0, latest as of 2023-03-27
urls = [
"https://mirror.bazel.build/github.com/golang/sys/archive/refs/tags/v0.6.0.zip",
"https://github.com/golang/sys/archive/refs/tags/v0.6.0.zip",
],
sha256 = "7f2399398b2eb4f1f495cc754d6353566e0ad934ee0eb46505e55162e0def56d",
strip_prefix = "sys-0.6.0",
patches = [
# releaser:patch-cmd gazelle -repo_root . -go_prefix golang.org/x/sys -go_naming_convention import_alias
Label("//third_party:org_golang_x_sys-gazelle.patch"),
],
patch_args = ["-p1"],
)

@bricedp bricedp changed the title Fix use of //go by exposing @org_golang_x_sys when using Bzlmod Fix use of //go with Bzlmod by removing dependency on @org_golang_x_sys Apr 1, 2023
Copy link
Collaborator

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

I will try to add a simple sh_test to tests/bcr that runs go version so that we don't regress this functionality.

@fmeum fmeum closed this Apr 2, 2023
@fmeum
Copy link
Collaborator

fmeum commented Apr 2, 2023

@bricedp Sorry, I wanted to add a commit to your branch, but unfortunately pushed the master branch of rules_go rather than my checkout of your fork to it, thus closing the PR. I can no longer push to it to clean up the mess. Could you restore the commits from https://github.com/fmeum/rules_go/tree/bricedp/master and open a new PR?

@bricedp
Copy link
Contributor Author

bricedp commented Apr 2, 2023

Thanks @fmeum for the tests, opened #3512 with our changes.

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.

2 participants