-
Notifications
You must be signed in to change notification settings - Fork 387
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
update-repos sets the name argument in lowercase. #117
Comments
I read through the workspace naming guidelines again. I couldn't find anything that dictates that workspace names should be lowercase, but it certainly seems to be conventional. Gazelle has been generating lowercase names for a while, and I'm concerned that changing it could break something. Could you try manually changing the name of the |
Yes, manually changing the name works. The problem is the inconsistency between running The dependencies in the |
I couldn't reproduce this. Could you show me a small test case where this happens using Gazelle at tip? In a minimal repo with a file that just imports this library, if I run
and this library rule:
If I capitalize In both cases, the same code is used to generate the external repository name. However, there's some new functionality that reads repository names and import paths from |
I made a test repo here: https://github.com/acehko/gazelle-test. go get -u github.com/bazelbuild/bazel-gazelle/cmd/gazelle
dep ensure
gazelle update-repos -from_file=Gopkg.lock
gazelle -go_prefix=github.com/acehko/gazelle-test |
Still works for me (mostly). Only difference was that I removed the vendor directory after running dep. From WORKSPACE:
From BUILD.bazel:
I am seeing a related issue in the build though:
|
I figured out what's happening with that error message. It looks like I added the lower casing in #47 (then forgot that it changed). rules_go is still pointing to an older version of Gazelle from before this change. So This will be fixed when rules_go updates to a newer version of Gazelle. PR #1279 does this but has been blocked on the removal of wtool.
|
Hey! I'm actually seeing this issue with I'm using the latest versions of both
The failure is (redacted a bit):
The correct path would be
|
Hmm, I can't reproduce this with the commits you mentioned. Could you maybe create a small example repo that demonstrates this? Here are the files I'm trying with: WORKSPACE
BUILD.bazel
lib.go
Running |
Thanks @jayconrod for looking into this! Strange, I can reproduce it consistently internally. |
Here's an example that shows the issue: https://github.com/astratto/test-case-gazelle Let me know if you need more info or if I'm doing something absolutely stupid 😄 |
Thanks, this makes more sense now. I think Gazelle is working as intended here, but I can see room for some improvements. When Gazelle generates a rule for
One way to fix this is by running I think a needed improvement here is the ability to tell Gazelle about a repository declared in a macro. I've filed #132 to track that. |
Thanks for the exhaustive explanation Jay, this is very precious info! I might have misunderstood, but the complexity of instructing Gazelle about repositories declared in a macro (i.e., I mean, my current workflow with Gazelle 0.8.0 is: [1] I'm over-simplifying a bit here, there's still some tweaks required to add information to the |
So basically, Gazelle needs some way to discover external repositories. We basically need a mapping between repository names and import paths. Currently, Gazelle just builds that mapping by parsing (not evaluating) WORKSPACE and looking for What I was thinking about with #132 would be a way to write comments in WORKSPACE that informs Gazelle of other repositories. You would probably need to write these by hand, at least in cases where Gazelle's default repository name differs from other tools (i.e., capitalization). This is only important for differing repository names now, but in the future, Gazelle will index libraries in external repositories and use that index to resolve dependencies. So it will be more important to have complete information when that's working. |
Okay, that makes sense. Thanks! My 0.02$: I think annotations in WORKSPACE for corner cases not detected might work nicely enough in my case. |
We are also seeing this issue. We generate the WORKSPACE file so our tooling currently has to perform a separate string replacement to get this right. The workflow we use is to generate a The example we have is the
Notice the |
@neakor The case of generated names is intended to be lower case. At one point there was a stronger Bazel convention around that. In any case, changing it now would create inconsistencies that would pretty thoroughly break You can change the names manually if you prefer. Later invocations of Closing this issue since there's nothing left to do here. |
For example, a program imports
github.com/Shopify/sarama
.Gopkg.tml:
BUILD.bazel:
WORKSPACE:
Currently the
update-repos
command sets thename
argument in lowercase.It should be
com_github_Shopify_sarama
.The text was updated successfully, but these errors were encountered: