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

Create Gazelle language for Starlark #251

Merged
merged 9 commits into from
Jun 26, 2020
Merged

Conversation

achew22
Copy link
Member

@achew22 achew22 commented May 28, 2020

This is a companion to #250 to provide a strawman for a Gazelle language extension.

Callouts:

  • I ran this on the main repo and it created the following targets that did not exist before. Please carefully review that I matched the repo's intent.
  • There are a couple of places where I wanted to print out warning messages to the user but I didn't see a way to do that that was super obvious. @jayconrod can you tell me how to better do that?
  • Added some e2e tests for the language extension for the edgecases I could think of. They are documented by gazelle/testdata/README.md. This is intended to be pretty comprehensive, but I wouldn't be surprised if there are additional oddities that I couldn't think of in the heat of the moment. If you're aware of additional edge cases I am happy to test them out.
  • There is no external repo indexing, so I make assumptions about the target that provides external imports. This is likely a bad assumption and it is better to fail by not adding the deps entry, but I am interested in input from both of you about the tradeoffs there.
  • I added a dependency on Alan Donovan's go.starlark.net (not @mentioned unless one of you thinks it's necessary) in the root WORKSPACE. I'm 100% sure that is wrong and I figured I would just ask to figure out the right way to do this since there are a lot of moving parts around bazel_federation.

CC: @aiuto @jayconrod

@achew22
Copy link
Member Author

achew22 commented May 28, 2020

Looking over the buildkite results, 2 things stand out:

  1. There is some kind of issue in the way I'm setting visibility related to OS differences between Ubuntu and Windows/Mc (not the normal pairing) that were not surfaced in developing on my Linux box. I have a Mac and will investigate.
  2. Looks like I need to fix the linter errors in the .bzl files I had in the test data directories. That's fair I guess.

Overall I don't think these are review blockers since this is intended to advance conversation and would still appreciate, if you're comfortable, you taking a look at the PR. If you're not, can you please leave a comment informing me what preconditions will need to be met before you'll review? Thanks!

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 comments on a quick first pass. Generally looks good though.

WORKSPACE Outdated Show resolved Hide resolved
gazelle/gazelle.go Outdated Show resolved Hide resolved
gazelle/gazelle.go Outdated Show resolved Hide resolved
gazelle/gazelle.go Outdated Show resolved Hide resolved
gazelle/gazelle.go Outdated Show resolved Hide resolved
gazelle/gazelle.go Outdated Show resolved Hide resolved
BUILD Outdated Show resolved Hide resolved
@laurentlb
Copy link
Contributor

Thanks for the PR, @achew22!
This is a large change and I don't think we'll be able to maintain this (and deal with the bug reports and incoming PRs).

Would you like to be a maintainer for this directory?

@achew22
Copy link
Member Author

achew22 commented Jun 4, 2020

@laurentlb, yes I would be happy to be a maintainer

@laurentlb
Copy link
Contributor

I gave you write access to the repository. Can you update the CodeOwners file in the same PR?

@aiuto, @jayconrod, other code review comments?

gazelle/gazelle.go Outdated Show resolved Hide resolved
gazelle/BUILD Outdated Show resolved Hide resolved
gazelle/gazelle.go Outdated Show resolved Hide resolved
gazelle/gazelle.go Outdated Show resolved Hide resolved
gazelle/gazelle.go Outdated Show resolved Hide resolved
gazelle/gazelle.go Outdated Show resolved Hide resolved
gazelle/gazelle.go Show resolved Hide resolved
gazelle/gazelle_test.go Show resolved Hide resolved
@achew22
Copy link
Member Author

achew22 commented Jun 10, 2020

@jayconrod PTAL, I believe I've addressed all your comments, but historically I've found this hard in GitHub as opposed to critique. Additionally, I've added you to the CODEOWNERS in order to satisfy the two OWNERS rules.

@laurentlb, I've included myself as a CODEOWNERS. Could you help me understand what to do with the remaining lint errors that are preventing submission?

I don't believe it is possible for anyone to submit a PR to this repo right now. Is there a way to configure your Copybara exporter to run BuildKite before pushing? I've opened an issue with the continuous-integration repo to track this in addition. What is the best way for me to proceed here?

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.

Changes look good. Couple more small comments. Otherwise LGTM.

(Completely agree that GitHub reviews are difficult.)

gazelle/gazelle.go Outdated Show resolved Hide resolved
gazelle/gazelle.go Show resolved Hide resolved
@achew22
Copy link
Member Author

achew22 commented Jun 19, 2020

@jayconrod thanks for your review! Once #254 is merged, I believe all the lint errors are fixed which means this CL will now pass CI and can be merged.

@achew22
Copy link
Member Author

achew22 commented Jun 23, 2020

@jayconrod and @aiuto, now that bazelbuild/buildtools#870 is submitted, once the ci server is updated this should be entirely green. PTAL.

Thanks!

@achew22 achew22 closed this Jun 24, 2020
@achew22 achew22 reopened this Jun 24, 2020
@jayconrod
Copy link
Contributor

LGTM, though I don't have submit powers in this repo. I think this would be fine to merge after the conflict in CODEOWNERS is fixed.

@achew22
Copy link
Member Author

achew22 commented Jun 24, 2020

@laurentlb looks like we are good to go then. PTAL and thanks for your help

@achew22
Copy link
Member Author

achew22 commented Jun 25, 2020

@aiuto or @laurentlb, friendliest of pings

Copy link
Contributor

@laurentlb laurentlb left a comment

Choose a reason for hiding this comment

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

lgtm

@laurentlb laurentlb merged commit d35e8d7 into bazelbuild:master Jun 26, 2020
@achew22 achew22 deleted the generator branch June 26, 2020 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants