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
Simplify v1 Go buildgen's use of source roots. #9694
Conversation
[ci skip-rust-tests] [ci skip-jvm-tests]
c10a797
to
9e844f4
Compare
@@ -215,6 +221,15 @@ def register_options(cls, register): | |||
help="An optional extension for all materialized BUILD files (should include the .)", | |||
) | |||
|
|||
register( | |||
"--remote-root", | |||
default=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it would be good to establish (or keep) a precedent instead of forcing every go user with external deps to add a pants.toml value for this. In fact, this is a breaking change as is with no deprecation cycle so I think adding the likely most common default - probably 3rdparty/go
, at least with a dep cycle before removing, is required. This still breaks anyone out there who relied on implicit 3rd_party/go
, thirdparty/go
or third_party/go
though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an attempt to infer which of the default options it is. It'll still break if someone set a custom remote root in [source]
, but I'm skeptical anyone has. And at least there's a sensible error message.
[ci skip-rust-tests] [ci skip-jvm-tests]
[ci skip-rust-tests] [ci skip-jvm-tests]
v1 Go buildgen is the only thing in the codebase that uses the fact that source roots know the language they are a root of, and the source/test/3rdparty distinction.
We'd like to greatly simplify source roots by getting rid of this extra data, which is not only not useful, but not always coherent (e.g., we can put multiple langs, and tests and sources, in the same source root).
To facilitate this simplification we get rid of the one place we did use that data. Instead we infer the (single) local Go source root, and require the remote root to be provided explicitly as a flag, if needed.
[ci skip-rust-tests]
[ci skip-jvm-tests]