Conversation
These are marked as Bazel, because there's no particular reason to treat them as anything else. Additionally, this expands the bazel-verse by adding a Starlark type: https://starlark-lang.org/ which is the language used by Copybara, Bazel, and Buck2. This lets us annotate non-Bazel Starlark as being Starlark but not Bazel, while Bazel Starlark is also Starlark. .sky is used by Copybara (actually more specifically .bara.sky). .star I've seen used in the test suite of starlark-rust, and it seems harmless to add. lib.star is mentioned as a filename in https://starlark-lang.org/spec.html. Reference: - https://buck2.build/docs/bxl/ - https://buck2.build/docs/rule_authors/package_files/
asottile
left a comment
asottile
left a comment
There was a problem hiding this comment.
rejecting this for now
I think there needs to be smaller more targetted efforts here. it's difficult to reason about all the things that were changed here
| 'config.ru': EXTENSIONS['rb'], | ||
| 'Containerfile': {'text', 'dockerfile'}, | ||
| 'CONTRIBUTING': EXTENSIONS['txt'], | ||
| 'copy.bara.sky': EXTENSIONS['bzl'], |
There was a problem hiding this comment.
this at the very least is a breaking change
There was a problem hiding this comment.
no it's not, except, perhaps for someone expecting copy.bara.sky to match as bazel rather than starlark.
i added sky as an extension for starlark, because copybara actually uses any .bara.sky file if you load() them (and indeed .sky is a legacy extension for starlark in general), so my change in this respect is a bug fix.
i want more direction here: how afraid are we of minor breaking changes to the (very rare) users of copybara? should i leave this as is and add a comment that says we're leaving this incorrectly marked as bazel for compatibility reasons, while having other non-bazel starlark just marked as starlark?
what's the general attitude towards breaking changes?
There was a problem hiding this comment.
what's the general attitude towards breaking changes?
we try pretty hard to not have breaking changes -- especially because they would be extremely subtle and difficult to track down. this library is really meant to be something that can be blindly updated at will
this is a breaking change in that someone using types: [bazel] to target starlark files would suddenly not be targetting this particular file (because it lost its bazel tag). and in the other case it's the opposite problem -- one may have been targetting types: [bazel] to format starlark and now they're receiving a .bxl file which isn't (?) starlark? -- and actually that file extension itself is ambiguous (a quick search of leads to a bunch of other things that aren't bazel so we can't accept it anyway)
There was a problem hiding this comment.
wait what, bxl isn't unique to buck? that's really surprising, i didn't know that.
bxl as used by buck is starlark.
There was a problem hiding this comment.
https://github.com/search?q=path%3A*.bxl&type=code ok here's a global search of all bxl files on GitHub. 100% of these are, based on my knowledge as a buck user, buck related, and are thus all starlark.
| 'bz2': {'binary', 'bzip2'}, | ||
| 'bz3': {'binary', 'bzip3'}, | ||
| 'bzl': {'text', 'bazel'}, | ||
| 'bxl': {'text', 'bazel'}, |
There was a problem hiding this comment.
prior to this, tools used bazel to target starlark-like files -- I'd be afraid this would be an incompatible change of the file type
There was a problem hiding this comment.
is this not a set of things? ie, this is just adding things that are matched? anyway i do see that i made a mistake where bxl should have been bazel.
lf-
left a comment
lf-
left a comment
There was a problem hiding this comment.
reading your review, I'm confused how to proceed. I need clarification on the following:
- how concerned are we about back compat? is it a Huge Problem to break copybara, something which we already had buggy support for and nobody noticed?
- do you think it's a good idea to distinguish bazel related starlark from non-bazel starlark? as a user, i do think so, because my linter (buildifier) would apply different lint rules to bazel flavoured starlark (actually its categories are BUILD, bzl, and generic starlark).
| 'bz2': {'binary', 'bzip2'}, | ||
| 'bz3': {'binary', 'bzip3'}, | ||
| 'bzl': {'text', 'bazel'}, | ||
| 'bxl': {'text', 'bazel'}, |
There was a problem hiding this comment.
is this not a set of things? ie, this is just adding things that are matched? anyway i do see that i made a mistake where bxl should have been bazel.
| 'config.ru': EXTENSIONS['rb'], | ||
| 'Containerfile': {'text', 'dockerfile'}, | ||
| 'CONTRIBUTING': EXTENSIONS['txt'], | ||
| 'copy.bara.sky': EXTENSIONS['bzl'], |
There was a problem hiding this comment.
no it's not, except, perhaps for someone expecting copy.bara.sky to match as bazel rather than starlark.
i added sky as an extension for starlark, because copybara actually uses any .bara.sky file if you load() them (and indeed .sky is a legacy extension for starlark in general), so my change in this respect is a bug fix.
i want more direction here: how afraid are we of minor breaking changes to the (very rare) users of copybara? should i leave this as is and add a comment that says we're leaving this incorrectly marked as bazel for compatibility reasons, while having other non-bazel starlark just marked as starlark?
what's the general attitude towards breaking changes?
2 participants
These are marked as Bazel, because there's no particular reason to treat them as anything else.
Additionally, this expands the bazel-verse by adding a Starlark type: https://starlark-lang.org/ which is the language used by Copybara, Bazel, and Buck2.
This lets us annotate non-Bazel Starlark as being Starlark but not Bazel, while Bazel Starlark is also Starlark.
.sky is used by Copybara (actually more specifically .bara.sky). .star I've seen used in the test suite of starlark-rust, and it seems harmless to add. lib.star is mentioned as a filename in
https://starlark-lang.org/spec.html.
Reference: