Skip to content

Commit 283f87b

Browse files
Ian Childsfacebook-github-bot
Ian Childs
authored andcommitted
Allow outputs to declare themselves to use content-based path hashing
Summary: As discussed in https://fb.workplace.com/groups/buck2dev/posts/3975810866040289/, we tie whether or not we're using content-based path hashing to an output, rather than to an action. I am using `uses_experimental_content_based_path_hashing` for now because I need people to know it is experimental (right now, if they turn it on it won't work correctly, but I need the option available for tests etc). Reviewed By: JakobDegen Differential Revision: D72457376 fbshipit-source-id: f5ac91310b02273791ff23a1dc8e6ae91571a52a
1 parent 036aed5 commit 283f87b

File tree

7 files changed

+81
-12
lines changed

7 files changed

+81
-12
lines changed

app/buck2_action_impl/src/context/unsorted.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ use buck2_build_api::interpreter::rule_defs::context::AnalysisActions;
1414
use buck2_build_api::interpreter::rule_defs::digest_config::StarlarkDigestConfig;
1515
use buck2_build_api::interpreter::rule_defs::transitive_set::FrozenTransitiveSetDefinition;
1616
use buck2_build_api::interpreter::rule_defs::transitive_set::TransitiveSet;
17+
use buck2_core::fs::buck_out_path::BuckOutPathKind;
1718
use buck2_execute::execute::request::OutputType;
1819
use starlark::environment::MethodsBuilder;
1920
use starlark::eval::Evaluator;
@@ -50,6 +51,7 @@ pub(crate) fn analysis_actions_methods_unsorted(builder: &mut MethodsBuilder) {
5051
#[starlark(require = pos)] prefix: &str,
5152
#[starlark(require = pos)] filename: Option<&str>,
5253
#[starlark(require = named, default = false)] dir: bool,
54+
#[starlark(require = named, default = false)] uses_experimental_content_based_path_hashing: bool,
5355
eval: &mut Evaluator<'v, '_, '_>,
5456
) -> starlark::Result<StarlarkDeclaredArtifact> {
5557
// We take either one or two positional arguments, namely (filename) or (prefix, filename).
@@ -65,11 +67,17 @@ pub(crate) fn analysis_actions_methods_unsorted(builder: &mut MethodsBuilder) {
6567
} else {
6668
OutputType::FileOrDirectory
6769
};
70+
let path_resolution_method = if uses_experimental_content_based_path_hashing {
71+
BuckOutPathKind::ContentHash
72+
} else {
73+
BuckOutPathKind::Configuration
74+
};
6875
let artifact = this.state()?.declare_output(
6976
prefix,
7077
filename,
7178
output_type,
7279
eval.call_stack_top_location(),
80+
path_resolution_method,
7381
)?;
7482

7583
Ok(StarlarkDeclaredArtifact::new(

app/buck2_action_impl/src/context/write.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ use buck2_build_api::interpreter::rule_defs::cmd_args::WriteToFileMacroVisitor;
2323
use buck2_build_api::interpreter::rule_defs::cmd_args::value::CommandLineArg;
2424
use buck2_build_api::interpreter::rule_defs::context::AnalysisActions;
2525
use buck2_build_api::interpreter::rule_defs::resolved_macro::ResolvedMacro;
26+
use buck2_core::fs::buck_out_path::BuckOutPathKind;
2627
use buck2_execute::execute::request::OutputType;
2728
use dupe::Dupe;
2829
use either::Either;
@@ -253,6 +254,7 @@ pub(crate) fn analysis_actions_methods_write(methods: &mut MethodsBuilder) {
253254
&format!("{}/{}.macro", &macro_directory_path, i),
254255
OutputType::File,
255256
eval.call_stack_top_location(),
257+
BuckOutPathKind::default(),
256258
)?;
257259
written_macro_files.insert(macro_file);
258260
}

app/buck2_build_api/src/actions/registry.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ use buck2_artifact::artifact::build_artifact::BuildArtifact;
2020
use buck2_core::category::Category;
2121
use buck2_core::deferred::key::DeferredHolderKey;
2222
use buck2_core::execution_types::execution::ExecutionPlatformResolution;
23+
use buck2_core::fs::buck_out_path::BuckOutPathKind;
2324
use buck2_core::fs::buck_out_path::BuildArtifactPath;
2425
use buck2_core::fs::paths::forward_rel_path::ForwardRelativePath;
2526
use buck2_core::fs::paths::forward_rel_path::ForwardRelativePathBuf;
@@ -167,13 +168,18 @@ impl ActionsRegistry {
167168
path: ForwardRelativePathBuf,
168169
output_type: OutputType,
169170
declaration_location: Option<FileSpan>,
171+
path_resolution_method: BuckOutPathKind,
170172
) -> buck2_error::Result<DeclaredArtifact> {
171173
let (path, hidden) = match prefix {
172174
None => (path, 0),
173175
Some(prefix) => (prefix.join(path), prefix.iter().count()),
174176
};
175177
self.claim_output_path(&path, declaration_location)?;
176-
let out_path = BuildArtifactPath::with_dynamic_actions_action_key(self.owner.dupe(), path);
178+
let out_path = BuildArtifactPath::with_dynamic_actions_action_key(
179+
self.owner.dupe(),
180+
path,
181+
path_resolution_method,
182+
);
177183
let declared = DeclaredArtifact::new(out_path, output_type, hidden);
178184
if !self.artifacts.insert(declared.dupe()) {
179185
panic!("not expected duplicate artifact after output path was successfully claimed");

app/buck2_build_api/src/analysis/registry.rs

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ use buck2_artifact::artifact::build_artifact::BuildArtifact;
2222
use buck2_core::deferred::base_deferred_key::BaseDeferredKey;
2323
use buck2_core::deferred::key::DeferredHolderKey;
2424
use buck2_core::execution_types::execution::ExecutionPlatformResolution;
25+
use buck2_core::fs::buck_out_path::BuckOutPathKind;
2526
use buck2_core::fs::paths::forward_rel_path::ForwardRelativePath;
2627
use buck2_core::fs::paths::forward_rel_path::ForwardRelativePathBuf;
2728
use buck2_error::BuckErrorContext;
@@ -148,6 +149,7 @@ impl<'v> AnalysisRegistry<'v> {
148149
filename: &str,
149150
output_type: OutputType,
150151
declaration_location: Option<FileSpan>,
152+
path_resolution_method: BuckOutPathKind,
151153
) -> buck2_error::Result<DeclaredArtifact> {
152154
// We don't allow declaring `` as an output, although technically there's nothing preventing
153155
// that
@@ -160,8 +162,13 @@ impl<'v> AnalysisRegistry<'v> {
160162
None => None,
161163
Some(x) => Some(ForwardRelativePath::new(x)?.to_owned()),
162164
};
163-
self.actions
164-
.declare_artifact(prefix, path, output_type, declaration_location)
165+
self.actions.declare_artifact(
166+
prefix,
167+
path,
168+
output_type,
169+
declaration_location,
170+
path_resolution_method,
171+
)
165172
}
166173

167174
/// Takes a string or artifact/output artifact and converts it into an output artifact
@@ -186,8 +193,13 @@ impl<'v> AnalysisRegistry<'v> {
186193
let heap = eval.heap();
187194
let declared_artifact = match value {
188195
OutputArtifactArg::Str(path) => {
189-
let artifact =
190-
self.declare_output(None, path, output_type, declaration_location.dupe())?;
196+
let artifact = self.declare_output(
197+
None,
198+
path,
199+
output_type,
200+
declaration_location.dupe(),
201+
BuckOutPathKind::default(),
202+
)?;
191203
heap.alloc_typed(StarlarkDeclaredArtifact::new(
192204
declaration_location,
193205
artifact,

app/buck2_build_api_tests/src/actions/registry.rs

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ use buck2_core::deferred::key::DeferredHolderKey;
2424
use buck2_core::execution_types::execution::ExecutionPlatform;
2525
use buck2_core::execution_types::execution::ExecutionPlatformResolution;
2626
use buck2_core::execution_types::executor_config::CommandExecutorConfig;
27+
use buck2_core::fs::buck_out_path::BuckOutPathKind;
2728
use buck2_core::fs::buck_out_path::BuildArtifactPath;
2829
use buck2_core::fs::paths::forward_rel_path::ForwardRelativePathBuf;
2930
use buck2_core::target::configured_target_label::ConfiguredTargetLabel;
@@ -46,20 +47,38 @@ fn declaring_artifacts() -> anyhow::Result<()> {
4647
);
4748
let out1 = ForwardRelativePathBuf::unchecked_new("bar.out".into());
4849
let buckout1 = BuildArtifactPath::new(base.dupe(), out1.clone());
49-
let declared1 = actions.declare_artifact(None, out1.clone(), OutputType::File, None)?;
50+
let declared1 = actions.declare_artifact(
51+
None,
52+
out1.clone(),
53+
OutputType::File,
54+
None,
55+
BuckOutPathKind::default(),
56+
)?;
5057
declared1
5158
.get_path()
5259
.with_full_path(|p| assert_eq!(p, buckout1.path()));
5360

5461
let out2 = ForwardRelativePathBuf::unchecked_new("bar2.out".into());
5562
let buckout2 = BuildArtifactPath::new(base, out2.clone());
56-
let declared2 = actions.declare_artifact(None, out2, OutputType::File, None)?;
63+
let declared2 = actions.declare_artifact(
64+
None,
65+
out2,
66+
OutputType::File,
67+
None,
68+
BuckOutPathKind::default(),
69+
)?;
5770
declared2
5871
.get_path()
5972
.with_full_path(|p| assert_eq!(p, buckout2.path()));
6073

6174
if actions
62-
.declare_artifact(None, out1, OutputType::File, None)
75+
.declare_artifact(
76+
None,
77+
out1,
78+
OutputType::File,
79+
None,
80+
BuckOutPathKind::default(),
81+
)
6382
.is_ok()
6483
{
6584
panic!("should error due to duplicate artifact")
@@ -129,7 +148,13 @@ fn register_actions() -> anyhow::Result<()> {
129148
ExecutionPlatformResolution::unspecified(),
130149
);
131150
let out = ForwardRelativePathBuf::unchecked_new("bar.out".into());
132-
let declared = actions.declare_artifact(None, out, OutputType::File, None)?;
151+
let declared = actions.declare_artifact(
152+
None,
153+
out,
154+
OutputType::File,
155+
None,
156+
BuckOutPathKind::default(),
157+
)?;
133158

134159
let inputs = indexset![ArtifactGroup::Artifact(
135160
BuildArtifact::testing_new(
@@ -177,7 +202,13 @@ fn finalizing_actions() -> anyhow::Result<()> {
177202
),
178203
);
179204
let out = ForwardRelativePathBuf::unchecked_new("bar.out".into());
180-
let declared = actions.declare_artifact(None, out, OutputType::File, None)?;
205+
let declared = actions.declare_artifact(
206+
None,
207+
out,
208+
OutputType::File,
209+
None,
210+
BuckOutPathKind::default(),
211+
)?;
181212

182213
let inputs = indexset![ArtifactGroup::Artifact(
183214
BuildArtifact::testing_new(

app/buck2_build_api_tests/src/interpreter/rule_defs/artifact/testing.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ use buck2_core::deferred::key::DeferredHolderKey;
3030
use buck2_core::execution_types::execution::ExecutionPlatformResolution;
3131
use buck2_core::execution_types::executor_config::PathSeparatorKind;
3232
use buck2_core::fs::artifact_path_resolver::ArtifactFs;
33+
use buck2_core::fs::buck_out_path::BuckOutPathKind;
3334
use buck2_core::fs::buck_out_path::BuckOutPathResolver;
3435
use buck2_core::fs::paths::abs_norm_path::AbsNormPathBuf;
3536
use buck2_core::fs::paths::forward_rel_path::ForwardRelativePathBuf;
@@ -115,6 +116,7 @@ pub(crate) fn artifactory(builder: &mut GlobalsBuilder) {
115116
ForwardRelativePathBuf::try_from(path.to_owned()).unwrap(),
116117
OutputType::File,
117118
None,
119+
BuckOutPathKind::default(),
118120
)?;
119121
Ok(StarlarkDeclaredArtifact::new(
120122
None,
@@ -138,6 +140,7 @@ pub(crate) fn artifactory(builder: &mut GlobalsBuilder) {
138140
ForwardRelativePathBuf::try_from(path.to_owned()).unwrap(),
139141
OutputType::File,
140142
None,
143+
BuckOutPathKind::default(),
141144
)?;
142145
let outputs = indexset![artifact.as_output()];
143146
registry.register(

app/buck2_core/src/fs/buck_out_path.rs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,17 +99,22 @@ pub struct BuildArtifactPath(Arc<BuildArtifactPathData>);
9999

100100
impl BuildArtifactPath {
101101
pub fn new(owner: BaseDeferredKey, path: ForwardRelativePathBuf) -> Self {
102-
Self::with_dynamic_actions_action_key(DeferredHolderKey::Base(owner), path)
102+
Self::with_dynamic_actions_action_key(
103+
DeferredHolderKey::Base(owner),
104+
path,
105+
BuckOutPathKind::default(),
106+
)
103107
}
104108

105109
pub fn with_dynamic_actions_action_key(
106110
owner: DeferredHolderKey,
107111
path: ForwardRelativePathBuf,
112+
path_resolution_method: BuckOutPathKind,
108113
) -> Self {
109114
BuildArtifactPath(Arc::new(BuildArtifactPathData {
110115
owner,
111116
path: path.into_box(),
112-
path_resolution_method: BuckOutPathKind::Configuration,
117+
path_resolution_method,
113118
}))
114119
}
115120

@@ -411,6 +416,7 @@ mod tests {
411416
use crate::deferred::dynamic::DynamicLambdaResultsKey;
412417
use crate::deferred::key::DeferredHolderKey;
413418
use crate::fs::artifact_path_resolver::ArtifactFs;
419+
use crate::fs::buck_out_path::BuckOutPathKind;
414420
use crate::fs::buck_out_path::BuckOutPathResolver;
415421
use crate::fs::buck_out_path::BuckOutScratchPath;
416422
use crate::fs::buck_out_path::BuildArtifactPath;
@@ -552,6 +558,7 @@ mod tests {
552558
DynamicLambdaIndex::new(17),
553559
))),
554560
ForwardRelativePathBuf::unchecked_new("quux".to_owned()),
561+
BuckOutPathKind::Configuration,
555562
);
556563
let resolved_gen_path = path_resolver.resolve_gen(&path)?;
557564

0 commit comments

Comments
 (0)