Conversation
crates/driver/src/print_build.rs
Outdated
| key: &str, | ||
| labels: &Vec<String>, | ||
| extra_kv_pairs: &mut HashMap<String, Vec<String>>, | ||
| include_file_extension: bool, |
There was a problem hiding this comment.
could we avoid boolean blindness with a small Enum to represent the semantics:
enum TargetNameStrategy { Directory, DirectoryFileType }or something
**Problem** Currently it's not possible to mix languages within a package. **Solution** 1. This implements `--append` flag to allow appending contents to BUILD.bazel. 2. This also adds `include_file_extension` configuration to allow `py_test` target to keep the name of the file as the target name.
There was a problem hiding this comment.
Here's an example output of a polyglot BUILD.
| option java_multiple_files = true; | ||
|
|
||
| import "src/main/protos/com/example/aa.proto"; | ||
| import "com/example/aa.proto"; |
There was a problem hiding this comment.
No src/main/protos/ here.
crates/driver/src/print_build.rs
Outdated
| Ok(file_name.replace(".", "_")) | ||
| } else { | ||
| match Path::new(&file_name).file_stem() { | ||
| Some(s) => Ok(s.to_str().unwrap().to_string()), |
There was a problem hiding this comment.
This wont panic because file_stem is always going to return a valid OsStr, right?
There was a problem hiding this comment.
At least I'm hoping that's the case.
There was a problem hiding this comment.
I might just use to_string_lossy() here.
There was a problem hiding this comment.
we are already returning a Result. Why not pass on the original error or rebox it?
There was a problem hiding this comment.
s.to_str() returns an Option, and my guess is that it's highly unlikely that it would fail.
non
left a comment
non
left a comment
There was a problem hiding this comment.
The changes themselves seemed OK but I had a question about the expected behavior/usage of the tool.
| # ---- END BZL_GEN_BUILD_GENERATED_CODE ---- no_hash | ||
| # ---- BEGIN BZL_GEN_BUILD_GENERATED_CODE ---- no_hash | ||
| py_library(name='hello', srcs=['hello.py'], deps=['@@rules_python~0.24.0~pip~pip_39_pandas//:pkg'], visibility=['//visibility:public']) | ||
| # ---- END BZL_GEN_BUILD_GENERATED_CODE ---- no_hash |
There was a problem hiding this comment.
Is it expected to have two different generated sections with the same name?
I had thought we'd only have one of these, or else we'd use two different names for the different parts. (That way each generated section could be safely updated independently without affecting any others.)
There was a problem hiding this comment.
No the target name must be unique within a package. This package demonstrates that bzl-gen-build can be configured to produce "hello" package even though it's inside of the "example" package. Normally we make a target based on the package (directory) name not the source file name.
There was a problem hiding this comment.
Sorry i wasn't more clear. I was actually referring to the generated comment section names (i.e. "BZL_GEN_BUILD_GENERATED_CODE"). If we have two generators "sharing" a file and want them each to be able to update their own section safely they'd need to be able to distinguish those sections somehow.
It's not really a big deal, and it would be easy to change I think, I just wanted to point it out.
There was a problem hiding this comment.
At least in the way it's done right now, the polyglot support is implemented by simply appending output after another, so yea, we end up with multiple codegen regions. I guess it's something we can follow up.
example/build_tools/lang_support/create_lang_build_files/regenerate_python_build_files.sh
Show resolved
Hide resolved
non
left a comment
non
left a comment
There was a problem hiding this comment.
Looks OK to me (once main is merged). I'd like it if the polyglot appenders used unique names but that's easy to do later.
5 participants
Fixes #169
Problem
Currently it's not possible to mix languages within a package.
Solution
--appendflag to allow appending contents to BUILD.bazel.include_file_extensionconfiguration to allowpy_testtarget to keep the name of the file as the target name.