Write rust phonies to ninja files for multi-file depsets When the user writes `depset([ctx.files.foo, ctx.files.bar])`, this generates a phony containing both of those files. Bug: 528225104 Change-Id: Idbf063332e0b3fbeb7ecc80f21cd21e26a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27420 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: Matt Stark <msta@google.com>
diff --git a/integration_tests/starlark/BUILD.gn b/integration_tests/starlark/BUILD.gn index fef2e75..e4c79a9 100644 --- a/integration_tests/starlark/BUILD.gn +++ b/integration_tests/starlark/BUILD.gn
@@ -5,9 +5,19 @@ load(":filegroup.scl", "filegroup", "filegroups") filegroup("filegroup") { - srcs = ["first.txt"] + srcs = [ "first.txt" ] +} + +filegroup("multiple") { + srcs = [ + "second.txt", + "third.txt", + ] } filegroups("filegroups") { - filegroups = [":filegroup"] + filegroups = [ + ":filegroup", + ":multiple", + ] }
diff --git a/integration_tests/starlark/goldens/build.ninja b/integration_tests/starlark/goldens/build.ninja index 9c07e2b..c24491f 100644 --- a/integration_tests/starlark/goldens/build.ninja +++ b/integration_tests/starlark/goldens/build.ninja
@@ -12,12 +12,15 @@ subninja toolchain.ninja build filegroup: phony ../first.txt -build filegroups: phony ../first.txt +build filegroups: phony phony/$:filegroups_0 +build multiple: phony phony/$:multiple_0 build $:filegroup: phony ../first.txt -build $:filegroups: phony ../first.txt +build $:filegroups: phony phony/$:filegroups_0 +build $:multiple: phony phony/$:multiple_0 build all: phony $ ../first.txt $ - ../first.txt + phony/$:filegroups_0 $ + phony/$:multiple_0 default all
diff --git a/integration_tests/starlark/goldens/obj/multiple/multiple.manifest b/integration_tests/starlark/goldens/obj/multiple/multiple.manifest new file mode 100644 index 0000000..c821e82 --- /dev/null +++ b/integration_tests/starlark/goldens/obj/multiple/multiple.manifest
@@ -0,0 +1,3 @@ +manifest for //:multiple +../second.txt +../third.txt \ No newline at end of file
diff --git a/integration_tests/starlark/goldens/toolchain.ninja b/integration_tests/starlark/goldens/toolchain.ninja index 8c5fe95..87c9cb7 100644 --- a/integration_tests/starlark/goldens/toolchain.ninja +++ b/integration_tests/starlark/goldens/toolchain.ninja
@@ -28,3 +28,5 @@ command = stamp ${out} description = STAMP ${out} +build phony/$:filegroups_0: phony ../first.txt phony/$:multiple_0 +build phony/$:multiple_0: phony ../second.txt ../third.txt
diff --git a/integration_tests/starlark/second.txt b/integration_tests/starlark/second.txt new file mode 100644 index 0000000..e69de29 --- /dev/null +++ b/integration_tests/starlark/second.txt
diff --git a/integration_tests/starlark/third.txt b/integration_tests/starlark/third.txt new file mode 100644 index 0000000..e69de29 --- /dev/null +++ b/integration_tests/starlark/third.txt
diff --git a/src/gn/ffi/bridge.cc b/src/gn/ffi/bridge.cc index ca7cf84..4625cf1 100644 --- a/src/gn/ffi/bridge.cc +++ b/src/gn/ffi/bridge.cc
@@ -1161,6 +1161,7 @@ #define CXXBRIDGE1_STRUCT_RustTarget struct RustTarget final : public ::rust::Opaque { ::rust::Str execute_rule_impl(::Session const &session, ::Err &err) const noexcept; + void write_phonies(::StringOutputBuffer &buffer) const noexcept; ~RustTarget() = delete; private: @@ -1518,6 +1519,8 @@ ::std::size_t cxxbridge1$196$RustTarget$operator$alignof() noexcept; ::rust::repr::Fat cxxbridge1$196$RustTarget$execute_rule_impl(::RustTarget const &self, ::Session const &session, ::Err &err) noexcept; + +void cxxbridge1$196$RustTarget$write_phonies(::RustTarget const &self, ::StringOutputBuffer &buffer) noexcept; ::std::size_t cxxbridge1$196$Session$operator$sizeof() noexcept; ::std::size_t cxxbridge1$196$Session$operator$alignof() noexcept; @@ -1554,6 +1557,10 @@ return ::rust::impl<::rust::Str>::new_unchecked(cxxbridge1$196$RustTarget$execute_rule_impl(*this, session, err)); } +void RustTarget::write_phonies(::StringOutputBuffer &buffer) const noexcept { + cxxbridge1$196$RustTarget$write_phonies(*this, buffer); +} + ::std::size_t Session::layout::size() noexcept { return cxxbridge1$196$Session$operator$sizeof(); }
diff --git a/src/gn/ffi/bridge.h b/src/gn/ffi/bridge.h index 6b5d96f..53e265a 100644 --- a/src/gn/ffi/bridge.h +++ b/src/gn/ffi/bridge.h
@@ -1079,6 +1079,7 @@ #define CXXBRIDGE1_STRUCT_RustTarget struct RustTarget final : public ::rust::Opaque { ::rust::Str execute_rule_impl(::Session const &session, ::Err &err) const noexcept; + void write_phonies(::StringOutputBuffer &buffer) const noexcept; ~RustTarget() = delete; private:
diff --git a/src/gn/ninja_target_writer.cc b/src/gn/ninja_target_writer.cc index 9459b5c..7bc39a9 100644 --- a/src/gn/ninja_target_writer.cc +++ b/src/gn/ninja_target_writer.cc
@@ -12,6 +12,7 @@ #include "gn/config_values_extractors.h" #include "gn/err.h" #include "gn/escape.h" +#include "gn/ffi/bridge.h" #include "gn/filesystem_utils.h" #include "gn/general_tool.h" #include "gn/ninja_action_target_writer.h" @@ -107,7 +108,7 @@ target->label().GetUserVisibleName(false)); trace.SetToolchain(settings->toolchain_label()); - if (g_scheduler->verbose_logging()) + if (g_scheduler && g_scheduler->verbose_logging()) g_scheduler->Log("Computing", target->label().GetUserVisibleName(true)); if (delayed_generated_file_write) @@ -196,6 +197,10 @@ WritePublicInputsStampOrPhony(target, resolved, storage); WriteHardDepsStampOrPhony(target, resolved, storage); + if (const auto* rust = target->rust_target()) { + rust->write_phonies(storage); + } + if (needs_file_write) { // Write the ninja file. SourceFile ninja_file = GetNinjaFileForTarget(target);
diff --git a/src/gn/starlark/crates/ffi/src/bridge.rs b/src/gn/starlark/crates/ffi/src/bridge.rs index 762a6da..96a2c16 100644 --- a/src/gn/starlark/crates/ffi/src/bridge.rs +++ b/src/gn/starlark/crates/ffi/src/bridge.rs
@@ -353,6 +353,7 @@ session: &'static Session, err: Pin<&mut Err>, ) -> &'static str; + fn write_phonies(self: &Target, buffer: Pin<&mut StringOutputBuffer>); type Session;
diff --git a/src/gn/starlark/crates/ffi/src/target.rs b/src/gn/starlark/crates/ffi/src/target.rs index 6ac4dc5..7db3379 100644 --- a/src/gn/starlark/crates/ffi/src/target.rs +++ b/src/gn/starlark/crates/ffi/src/target.rs
@@ -5,13 +5,15 @@ use std::cell::OnceCell; use providers::Providers; +use types::Phony; -use crate::{TargetRef, eval_context::EvalContext}; +use crate::{StringOutputBuffer, TargetRef, escape::Ninja, eval_context::EvalContext}; /// The results of running the rule implementation on a target. #[derive(Debug)] pub(crate) struct EvaluatedTarget { pub(crate) providers: Providers, + pub(crate) phonies: Vec<Phony>, } pub struct Target { @@ -38,15 +40,17 @@ let res = rule::run(&target_ref, |t| { crate::eval_context::EvalContext::new_rule_impl(session, *t) }) - .and_then(providers::Providers::try_from); - if let Some(providers) = err.handle(res) { + .and_then(|(phonies, frozen)| { + providers::Providers::try_from(frozen).map(|providers| (phonies, providers)) + }); + if let Some((phonies, providers)) = err.handle(res) { let phony = providers .outputs_phony .as_ref() .map(|f| f.as_str()) .unwrap_or_default(); self.evaluated - .set(EvaluatedTarget { providers }) + .set(EvaluatedTarget { providers, phonies }) .expect("Rules can only be executed once"); phony } else { @@ -54,6 +58,20 @@ } } + /// Writes out Ninja phony rules declared by this target into the provided buffer. + pub fn write_phonies(&self, buffer: std::pin::Pin<&mut crate::bridge::StringOutputBuffer>) { + if let Some(evaluated) = self.evaluated.get() { + let mut buf = StringOutputBuffer::new(buffer); + for phony in &evaluated.phonies { + buf.append(("build ", Ninja(phony.phony.as_str()), ": phony")); + for dep in &phony.expansion { + buf.append((b' ', Ninja(dep.as_str()))); + } + buf.append(b'\n'); + } + } + } + pub fn providers(&self) -> &Providers { &self .evaluated
diff --git a/src/gn/starlark/crates/rule/src/implementation.rs b/src/gn/starlark/crates/rule/src/implementation.rs index 4e2dc3d..464a52b 100644 --- a/src/gn/starlark/crates/rule/src/implementation.rs +++ b/src/gn/starlark/crates/rule/src/implementation.rs
@@ -18,7 +18,7 @@ pub fn run<C: EvalContextAttrExt + crate::CtxMethods>( target: &<C::Session as Session>::TargetRef, create_context: impl FnOnce(&<C::Session as Session>::TargetRef) -> C, -) -> starlark::Result<OwnedFrozen<Value<'static>>> +) -> starlark::Result<(Vec<types::Phony>, OwnedFrozen<Value<'static>>)> where <C::Session as Session>::TargetRef: TargetAttrExt<Rule = Rule<'static, C>, Session = C::Session>, @@ -27,51 +27,45 @@ let rule_context = create_context(target); Module::with_temp_heap(|module| { - if rule.is_builtin() { + let res = if rule.is_builtin() { // The other branch would also work, but this allows us to skip several // allocations (eg. ctx.attr.*) and spinning up a starlark // interpreter. let eval = Evaluator::new(&module); let heap = eval.heap(); - let state_cell = rule_context.require_rule_impl()?; - let res = { - let mut state = state_cell.borrow_mut(); - rule_context - .session() - .run_builtin_rule(&heap, target, &mut state) - }; - state_cell.borrow().rule_impl_complete()?; - module.set_extra_value(res); + let mut state = rule_context.require_rule_impl()?.borrow_mut(); + rule_context + .session() + .run_builtin_rule(&heap, target, &mut state) } else { // When the module is frozen, only things transitively required by extra_value // are kept. - module.set_extra_value({ - let mut eval = Evaluator::new(&module); - eval.set_context(&rule_context); - let rule = HeapEdge::immortal().rebrand_ref(rule); - let ctx = eval.heap().alloc(Ctx::<C>::new( - rule.schema.create_ctx_fields( - target.attrs(), - rule_context.session(), - &rule_context.current_toolchain(), - rule.output_type, - target.builtin_attrs(rule_context.session(), &eval.heap()), - &eval.heap(), - )?, - rule, - eval.heap().alloc(target.label().to_owned()), - eval.heap().alloc(crate::actions::Actions::<C>::default()), - )); - let res = eval.eval_function(rule.implementation, &[ctx], &[])?; - rule_context - .require_rule_impl()? - .borrow() - .rule_impl_complete()?; - res - }); - } + let mut eval = Evaluator::new(&module); + eval.set_context(&rule_context); + let rule = HeapEdge::immortal().rebrand_ref(rule); + let ctx = eval.heap().alloc(Ctx::<C>::new( + rule.schema.create_ctx_fields( + target.attrs(), + rule_context.session(), + &rule_context.current_toolchain(), + rule.output_type, + target.builtin_attrs(rule_context.session(), &eval.heap()), + &eval.heap(), + )?, + rule, + eval.heap().alloc(target.label().to_owned()), + eval.heap().alloc(crate::actions::Actions::<C>::default()), + )); + eval.eval_function(rule.implementation, &[ctx], &[])? + }; + + let phonies = rule_context + .require_rule_impl()? + .borrow_mut() + .rule_impl_complete()?; + module.set_extra_value(res); let frozen = module.freeze()?; - Ok(frozen.extra_value().unwrap()) + Ok((phonies, frozen.extra_value().unwrap())) }) }
diff --git a/src/gn/starlark/crates/rule/tests/actions.rs b/src/gn/starlark/crates/rule/tests/actions.rs index 571af1a..7b500bd 100644 --- a/src/gn/starlark/crates/rule/tests/actions.rs +++ b/src/gn/starlark/crates/rule/tests/actions.rs
@@ -123,7 +123,7 @@ match expected { Ok((defaultinfo_phony, files)) => { - let rooted = res.expect(target_name); + let (_phonies, rooted) = res.expect(target_name); let providers = providers::Providers::try_from(rooted).expect(target_name); if defaultinfo_phony.is_empty() {
diff --git a/src/gn/starlark/crates/rule/tests/ctx_super.rs b/src/gn/starlark/crates/rule/tests/ctx_super.rs index 2148819..7f5af78 100644 --- a/src/gn/starlark/crates/rule/tests/ctx_super.rs +++ b/src/gn/starlark/crates/rule/tests/ctx_super.rs
@@ -35,7 +35,7 @@ .get_target(label.as_ref(), context.session.default_toolchain.as_ref()); let session = assert.session(); - let res = run(&target, move |t: &testutils::FakeTargetRef| { + let (_phonies, res) = run(&target, move |t: &testutils::FakeTargetRef| { testutils::FakeEvalContext::rule_impl(session.clone(), t.clone()) }) .unwrap();
diff --git a/src/gn/starlark/crates/types/src/ctx_state.rs b/src/gn/starlark/crates/types/src/ctx_state.rs index 20bf6a9..8584fe3 100644 --- a/src/gn/starlark/crates/types/src/ctx_state.rs +++ b/src/gn/starlark/crates/types/src/ctx_state.rs
@@ -108,13 +108,13 @@ Ok(()) } - /// Verifies that all declared outputs were generated. - pub fn rule_impl_complete(&self) -> starlark::Result<()> { + /// Verifies that all declared outputs were generated, and returns the generated phonies. + pub fn rule_impl_complete(&mut self) -> starlark::Result<Vec<Phony>> { match self.unused_declared_outputs.first() { Some(unused) => { Err(crate::errors::Error::DeclaredOutputNeverGenerated(unused.clone()).into()) }, - None => Ok(()), + None => Ok(std::mem::take(&mut self.phonies)), } } }
diff --git a/src/gn/target.h b/src/gn/target.h index 6e520bb..5fec078 100644 --- a/src/gn/target.h +++ b/src/gn/target.h
@@ -376,6 +376,9 @@ bool has_rust_values() const { return rust_values_.get(); } const RustTarget& rust_target(const Session& session) const; + const RustTarget* rust_target() const { + return rust_target_.load(std::memory_order_acquire); + } void set_rust_target(const RustTarget& rust_target) const { rust_target_.store(&rust_target, std::memory_order_release); }