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

repository_macro directive for referencing macros from WORKSPACE #493

Merged
merged 4 commits into from
Apr 17, 2019

Conversation

blico
Copy link
Contributor

@blico blico commented Apr 2, 2019

Addresses #483

Added the repository_macro directive with the following format:
# gazelle:repository_macro repos.bzl%go_repositories

Gazelle only recognizes this directive when reading from the WORKSPACE file.

fix-update uses the directive to know about rules defined in a macro file, so they can be used during dependency resolution.

update-repos can now update repository rules in the corresponding macros without additional command line arguments instead of writing copies to WORKSPACE.

The to-macro flag functionality has been modified to only write new repository rules to the corresponding macro, and to update existing rules in macros and the WORKSPACE file. However, duplicates will still be written to the macro if the repository rule is not a go_repository rule.

@jayconrod
Copy link
Contributor

Thanks for working on this! I'll review as soon as I can, but it might be a couple days. Sorry for the latency -- a lot is happening right now.

Copy link
Contributor

@jayconrod jayconrod left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Sorry for the review latency. Here's a first pass of comments.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
cmd/gazelle/integration_test.go Outdated Show resolved Hide resolved
cmd/gazelle/update-repos.go Outdated Show resolved Hide resolved
cmd/gazelle/update-repos.go Outdated Show resolved Hide resolved
cmd/gazelle/update-repos.go Outdated Show resolved Hide resolved
repo/repo.go Outdated Show resolved Hide resolved
cmd/gazelle/update-repos.go Outdated Show resolved Hide resolved
applyBuildAttributes(c, rules[i])
}
merger.MergeFile(f, nil, rules, merger.PreResolve, kinds)
if err := f.Save(f.Path); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the same file contains multiple macros? It seems like earlier changes would be overwritten here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My latest change handles this case in MergeRules by calling rule.SyncMacroFile. It is not the greatest solution, so I am open to other ideas.

repo/repo.go Outdated Show resolved Hide resolved
@blico blico force-pushed the repositoryMacroDirective branch from 02ee6b2 to 1122b00 Compare April 8, 2019 21:35
@blico blico force-pushed the repositoryMacroDirective branch from 1122b00 to e48258e Compare April 8, 2019 21:39
@jayconrod
Copy link
Contributor

(Once again, I apologize for review latency. I'll look at the updates as soon as I can.)

Copy link
Contributor

@jayconrod jayconrod left a comment

Choose a reason for hiding this comment

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

A few more comments. Sorry for the latency and thanks for working on this.

Also it looks like the linker in panicking in CI? Does it do that locally with bazel test ... or go test ./...? Very strange.

cmd/gazelle/fix-update.go Outdated Show resolved Hide resolved
repo/repo.go Show resolved Hide resolved
repo/repo.go Outdated Show resolved Hide resolved
rule/rule.go Show resolved Hide resolved
repo/repo.go Outdated Show resolved Hide resolved
@blico
Copy link
Contributor Author

blico commented Apr 13, 2019

Also it looks like the linker in panicking in CI? Does it do that locally with bazel test ... or go test ./...? Very strange.

Looks like it was an import cycle in merger_test.go caused by repo adding merger as a dependency. I fixed this by making merger_test a different package. As an fyi one side effect is that I exported merger.Match

Copy link
Contributor

@jayconrod jayconrod 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, seems to work well. Thanks for doing this!

@jayconrod jayconrod merged commit 4f524f2 into bazel-contrib:master Apr 17, 2019
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