Skip to content

Commit

Permalink
Fix apply_binaries for Google3 (#243)
Browse files Browse the repository at this point in the history
**Problem**
When using `--no-aggregate-source` option with py_binary,
I noticed that we end up generating py_binary twice,
first time for main, and second time for test.

**Solution**
This implements `TargetEntries::combine(ts1, ts2)`, which
can dedupliate the target entries based on the name.
There's an example in `examples/com` demonstrating the usage.
  • Loading branch information
eed3si9n authored Apr 9, 2024
1 parent 1cde81f commit 9ccc02d
Show file tree
Hide file tree
Showing 6 changed files with 106 additions and 35 deletions.
94 changes: 61 additions & 33 deletions crates/driver/src/print_build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,21 @@ impl TargetEntries {

Ok(PythonProgram { body: program })
}

fn combine(t1: TargetEntries, t2: TargetEntries) -> TargetEntries {
let mut entries: Vec<TargetEntry> = Vec::default();
let mut names: HashSet<String> = HashSet::default();
for entry in t1.entries {
names.insert(entry.name.clone());
entries.push(entry);
}
for entry in t2.entries {
if !names.contains(&entry.name) {
entries.push(entry);
}
}
TargetEntries { entries: entries }
}
}

async fn generate_targets<F, R>(
Expand Down Expand Up @@ -391,7 +406,7 @@ where

let mut parent_include_src = Vec::default();

apply_binaries(&mut t, &graph_node.node_metadata, module_config, &element)?;
apply_binaries(&mut t, &graph_node.node_metadata, module_config, &target_name)?;
apply_manual_refs(&mut extra_kv_pairs, &graph_node.node_metadata);
apply_attr_string_lists(&mut extra_kv_pairs, &graph_node.node_metadata);
// before we give extra_kv_pairs away to make the main target,
Expand Down Expand Up @@ -568,7 +583,7 @@ where
target_entries: &mut TargetEntries,
node_metadata: &GraphNodeMetadata,
module_config: &ModuleConfig,
target_path: &str,
lib_target: &str,
) -> Result<()> {
if !node_metadata.binary_refs.is_empty() {
let build_config = match &module_config.build_config.binary_application {
Expand Down Expand Up @@ -599,7 +614,7 @@ where
k_strs.push(("binary_refs_value".to_string(), ep.to_string()));
}

k_strs.push(("owning_library".to_string(), format!("//{}", target_path)));
k_strs.push(("owning_library".to_string(), format!(":{}", lib_target.to_string())));

target_entries.entries.push(TargetEntry {
name: bin.binary_refs.binary_name.clone(),
Expand Down Expand Up @@ -771,7 +786,7 @@ async fn print_file(
let target_folder = opt.working_directory.join(&element);
let target_file = target_folder.join("BUILD.bazel");
emitted_files.push(target_file.clone());
let t = generate_targets(
let t1 = generate_targets(
opt,
project_conf,
SourceConfig::Main,
Expand Down Expand Up @@ -803,6 +818,7 @@ async fn print_file(
},
)
.await?;
let t = TargetEntries::combine(t1, t2);
let handle = concurrent_io_operations.acquire().await?;
let write_mode = WriteMode::new(opt.append);
match write_mode {
Expand All @@ -821,13 +837,6 @@ async fn print_file(
format!("Attempting to write file data to {:?}", target_file)
})?;
}
if !t2.entries.is_empty() {
file.write(t2.emit_build_file()?.as_bytes())
.await
.with_context(|| {
format!("Attempting to write file data to {:?}", target_file)
})?;
}
}
WriteMode::Overwrite => {
let mut file = tokio::fs::OpenOptions::new()
Expand All @@ -843,13 +852,6 @@ async fn print_file(
format!("Attempting to write file data to {:?}", target_file)
})?;
}
if !t2.entries.is_empty() {
file.write(t2.emit_build_file()?.as_bytes())
.await
.with_context(|| {
format!("Attempting to write file data to {:?}", target_file)
})?;
}
}
}
drop(handle);
Expand Down Expand Up @@ -1224,15 +1226,31 @@ scala_tests(
};

let mut entries = Vec::default();
entries.push(make_target_entry("scala_extractor"));
let target_entries = TargetEntries { entries };

let generated_s = target_entries.emit_build_file().unwrap();

let parsed_from_generated_string =
PythonProgram::parse(generated_s.as_str(), "tmp.py").unwrap();

assert_eq!(
parsed_from_embed_string.to_string(),
parsed_from_generated_string.to_string(),
"\n\nExpected:\n{}\n\nGenerated:\n{}\n",
parsed_from_embed_string,
parsed_from_generated_string
);
}

fn make_target_entry(name: &str) -> TargetEntry {
let mut required_load = HashMap::new();
required_load.insert(
Arc::new("//build_tools/lang_support/scala/test:scalatest.bzl".to_string()),
vec![Arc::new("scala_tests".to_string())],
);

entries.push(TargetEntry {
name: "scala_extractor".to_string(),
TargetEntry {
name: name.to_string(),
extra_kv_pairs: vec![(
"deps".to_string(),
vec![
Expand All @@ -1249,21 +1267,31 @@ scala_tests(
}),
target_type: Arc::new("scala_tests".to_string()),
extra_k_strs: Vec::default(),
});
}
}

let target_entries = TargetEntries { entries };
#[test]
fn test_combine() {
let mut e1 = Vec::default();
e1.push(make_target_entry("t1"));
let ts1 = TargetEntries { entries: e1 };

let generated_s = target_entries.emit_build_file().unwrap();
let mut e2 = Vec::default();
e2.push(make_target_entry("t2"));
let ts2 = TargetEntries { entries: e2 };

let parsed_from_generated_string =
PythonProgram::parse(generated_s.as_str(), "tmp.py").unwrap();
let mut e3 = Vec::default();
e3.push(make_target_entry("t1"));
let ts3 = TargetEntries { entries: e3 };

assert_eq!(
parsed_from_embed_string.to_string(),
parsed_from_generated_string.to_string(),
"\n\nExpected:\n{}\n\nGenerated:\n{}\n",
parsed_from_embed_string,
parsed_from_generated_string
);
let mut e4 = Vec::default();
e4.push(make_target_entry("t1"));
let ts4 = TargetEntries { entries: e4 };

let actual1 = TargetEntries::combine(ts1, ts2);
assert_eq!(actual1.entries.len(), 2);

let actual2 = TargetEntries::combine(ts3, ts4);
assert_eq!(actual2.entries.len(), 1);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@
"headers": [],
"function_name": "py_test",
"target_name_strategy": "source_file_stem"
},
"binary_application": {
"headers": [
{
"load_from": "//build_tools/lang_support/python:py_binary.bzl",
"load_value": "py_binary"
}
],
"function_name": "py_binary"
}
},
"main_roots": [
Expand Down
Empty file.
33 changes: 33 additions & 0 deletions example/build_tools/lang_support/python/py_binary.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
def py_binary(
name = None,
binary_refs_value = None,
owning_library = None,
entity_path = None,
visibility = None,
**kwargs):
if name == None:
fail("Need to specify name")
if owning_library == None:
fail("Need to specify owning_library")
if entity_path == None:
fail("Need to specify entity_path")
if visibility == None:
fail("Need to specify visibility")
idx = entity_path.rindex("/")
relative_entity_path = entity_path
if idx >= 0:
relative_entity_path = entity_path[idx + 1:]

# buildifier: disable=native-python
native.py_binary(
name = name,
main = relative_entity_path,
legacy_create_init = 1,
deps = [
owning_library,
],
srcs = [
relative_entity_path,
],
visibility = visibility,
)
4 changes: 2 additions & 2 deletions example/com/example/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ java_proto_library(name='aa_proto_java', deps=[':aa_proto'], visibility=['//visi
py_proto_library(name='aa_proto_py', deps=[':aa_proto'], visibility=['//visibility:public'])
# ---- END BZL_GEN_BUILD_GENERATED_CODE ---- no_hash
# ---- BEGIN BZL_GEN_BUILD_GENERATED_CODE ---- no_hash
load('//build_tools/lang_support/python:py_binary.bzl', 'py_binary')
py_binary(name='bin', entity_path='com/example/hello.py', owning_library=':hello', visibility=['//visibility:public'])
py_library(name='hello', srcs=['hello.py'], deps=['@@//com/example:aa_proto_py', '@@rules_python~0.24.0~pip~pip_39_pandas//:pkg'], visibility=['//visibility:public'])
# ---- END BZL_GEN_BUILD_GENERATED_CODE ---- no_hash
# ---- BEGIN BZL_GEN_BUILD_GENERATED_CODE ---- no_hash
py_test(name='hello_test', srcs=['hello_test.py'], deps=['//com/example:hello'], visibility=['//visibility:public'])
# ---- END BZL_GEN_BUILD_GENERATED_CODE ---- no_hash
1 change: 1 addition & 0 deletions example/com/example/hello.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
def some_data():
return pd.DataFrame()

# bzl_gen_build:binary_generate:bin
def main():
print("Hello World!")

Expand Down

0 comments on commit 9ccc02d

Please sign in to comment.