diff --git a/app/buck2_cfg_constructor/src/registration.rs b/app/buck2_cfg_constructor/src/registration.rs index a5b05a9db673..21288b88b0ee 100644 --- a/app/buck2_cfg_constructor/src/registration.rs +++ b/app/buck2_cfg_constructor/src/registration.rs @@ -11,7 +11,6 @@ use std::sync::Arc; use allocative::Allocative; -use buck2_core::cells::cell_path::CellPathRef; use buck2_core::cells::paths::CellRelativePath; use buck2_interpreter::downstream_crate_starlark_defs::REGISTER_BUCK2_CFG_CONSTRUCTOR_GLOBALS; use buck2_interpreter_for_build::interpreter::build_context::BuildContext; @@ -43,8 +42,8 @@ use crate::CfgConstructor; #[derive(Debug, buck2_error::Error)] #[buck2(tag = Input)] enum RegisterCfgConstructorError { - #[error("`set_cfg_constructor()` can only be called from the repository root `PACKAGE` file")] - NotPackageRoot, + #[error("`set_cfg_constructor()` can only be called from the root `PACKAGE` file of a cell")] + NotCellRoot, #[error("`set_cfg_constructor()` can only be called at most once")] AlreadyRegistered, } @@ -150,7 +149,11 @@ fn make_cfg_constructor( pub(crate) fn register_set_cfg_constructor(globals: &mut GlobalsBuilder) { /// Register global cfg constructor. /// - /// This function can only be called from the repository root `PACKAGE` file. + /// This function can only be called from the root `PACKAGE` file of a cell. + /// When called from a non-root cell (e.g., an external cell), the call is + /// silently ignored. This allows repositories to include `set_cfg_constructor` + /// in their PACKAGE/BUCK_TREE files while still being consumable as external + /// cells by other repositories. /// /// Parameters: /// * `stage0`: The first cfg constructor that will be invoked before configuration rules are analyzed. @@ -172,20 +175,33 @@ pub(crate) fn register_set_cfg_constructor(globals: &mut GlobalsBuilder) { PerFileTypeContext::Package(ctx) => ctx, _ => { return Err( - buck2_error::Error::from(RegisterCfgConstructorError::NotPackageRoot).into(), + buck2_error::Error::from(RegisterCfgConstructorError::NotCellRoot).into(), ); } }; - if ctx.path.dir() - != CellPathRef::new( - build_context.cell_info.cell_resolver().root_cell(), - CellRelativePath::empty(), - ) - { + + // Check if this is being called from the root of the current cell + let current_cell = ctx.path.dir().cell(); + let is_cell_root = ctx.path.dir().path() == CellRelativePath::empty(); + + if !is_cell_root { + // Not at cell root - this is an error return Err( - buck2_error::Error::from(RegisterCfgConstructorError::NotPackageRoot).into(), + buck2_error::Error::from(RegisterCfgConstructorError::NotCellRoot).into(), ); } + + // Check if this cell is the root cell + let is_root_cell = current_cell == build_context.cell_info.cell_resolver().root_cell(); + + if !is_root_cell { + // Called from an external cell's root PACKAGE - silently ignore. + // This allows repos to have set_cfg_constructor in their PACKAGE/BUCK_TREE + // while still being usable as external cells. + return Ok(NoneType); + } + + // This is the root cell's root PACKAGE - register the cfg constructor let package_file_extra: &PackageFileExtra = PackageFileExtra::get_or_init(eval)?; if package_file_extra.cfg_constructor.get().is_some() { return Err( diff --git a/tests/core/configurations/cfg_constructor/BUCK b/tests/core/configurations/cfg_constructor/BUCK new file mode 100644 index 000000000000..54705926811e --- /dev/null +++ b/tests/core/configurations/cfg_constructor/BUCK @@ -0,0 +1,5 @@ +load("@fbcode//buck2/tests:buck_e2e.bzl", "buck2_core_tests") + +oncall("build_infra") + +buck2_core_tests() diff --git a/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor.py b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor.py new file mode 100644 index 000000000000..898c63401b3c --- /dev/null +++ b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor.py @@ -0,0 +1,41 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is dual-licensed under either the MIT license found in the +# LICENSE-MIT file in the root directory of this source tree or the Apache +# License, Version 2.0 found in the LICENSE-APACHE file in the root directory +# of this source tree. You may select, at your option, one of the +# above-listed licenses. + +# pyre-strict + + +from buck2.tests.e2e_util.api.buck import Buck +from buck2.tests.e2e_util.buck_workspace import buck_test + + +@buck_test() +async def test_external_cell_set_cfg_constructor_silently_ignored(buck: Buck) -> None: + """Test that set_cfg_constructor called from an external cell's PACKAGE is silently ignored. + + The external cell has its own set_cfg_constructor call in its PACKAGE file. + This should be silently ignored, and the root cell's cfg_constructor + should be used instead. + """ + # Query a target in the root cell - should succeed + result = await buck.cquery("root//:test") + assert "root//:test" in result.stdout + + +@buck_test() +async def test_external_cell_target_succeeds_with_cfg_constructor(buck: Buck) -> None: + """Test that targets in external cells work when the external cell has set_cfg_constructor. + + Even though the external cell has its own set_cfg_constructor in its PACKAGE, + querying targets in that cell should succeed because the call is silently ignored. + """ + # Query a target in the external cell - should succeed + # The external cell's set_cfg_constructor should be silently ignored + result = await buck.cquery("external_cell//:external_test") + assert "external_cell//:external_test" in result.stdout + # Make sure the external cell's constructor wasn't used + assert "SHOULD_NOT_BE_USED" not in result.stdout diff --git a/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/.buckconfig b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/.buckconfig new file mode 100644 index 000000000000..71eeb2c29eb0 --- /dev/null +++ b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/.buckconfig @@ -0,0 +1,7 @@ +[buildfile] +name = TARGETS.fixture + +[cells] +root = . +prelude = prelude +external_cell = external_cell diff --git a/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/.buckroot b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/.buckroot new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/PACKAGE b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/PACKAGE new file mode 100644 index 000000000000..2748d1598b00 --- /dev/null +++ b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/PACKAGE @@ -0,0 +1,3 @@ +load("//:defs.bzl", "init_cfg_constructor") + +init_cfg_constructor() diff --git a/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/TARGETS.fixture b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/TARGETS.fixture new file mode 100644 index 000000000000..eef12a0471ee --- /dev/null +++ b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/TARGETS.fixture @@ -0,0 +1,10 @@ +load(":rules.bzl", "test_platform", "test_rule") + +test_platform( + name = "platform", +) + +test_rule( + name = "test", + default_target_platform = ":platform", +) diff --git a/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/defs.bzl b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/defs.bzl new file mode 100644 index 000000000000..2729680c2c38 --- /dev/null +++ b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/defs.bzl @@ -0,0 +1,45 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is dual-licensed under either the MIT license found in the +# LICENSE-MIT file in the root directory of this source tree or the Apache +# License, Version 2.0 found in the LICENSE-APACHE file in the root directory +# of this source tree. You may select, at your option, one of the +# above-listed licenses. + +KEY = "buck.cfg_modifiers" + +def _cfg_constructor_pre_constraint_analysis( + legacy_platform, + package_modifiers: dict[str, typing.Any] | None, + target_modifiers: dict[str, typing.Any] | None, + cli_modifiers: list[str], + rule_name: str, + aliases: struct | None, + extra_data: dict[str, typing.Any] | None): + _unused = package_modifiers # buildifier: disable=unused-variable + _unused = target_modifiers # buildifier: disable=unused-variable + _unused = cli_modifiers # buildifier: disable=unused-variable + _unused = extra_data # buildifier: disable=unused-variable + _unused = aliases # buildifier: disable=unused-variable + _unused = rule_name # buildifier: disable=unused-variable + + platform = legacy_platform or PlatformInfo(label = "root_cfg_constructor", configuration = ConfigurationInfo( + constraints = {}, + values = {}, + )) + return ([], platform) + +def _cfg_constructor_post_constraint_analysis(refs: dict[str, ProviderCollection], params): + _unused = refs # buildifier: disable=unused-variable + _unused = params # buildifier: disable=unused-variable + return PlatformInfo(label = "root_post_constraint_analysis", configuration = ConfigurationInfo( + constraints = params.configuration.constraints, + values = {}, + )) + +def init_cfg_constructor(): + set_cfg_constructor( + key = KEY, + stage0 = _cfg_constructor_pre_constraint_analysis, + stage1 = _cfg_constructor_post_constraint_analysis, + ) diff --git a/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/.buckconfig b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/.buckconfig new file mode 100644 index 000000000000..ba37ff019b01 --- /dev/null +++ b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/.buckconfig @@ -0,0 +1,2 @@ +[buildfile] +name = TARGETS.fixture diff --git a/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/PACKAGE b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/PACKAGE new file mode 100644 index 000000000000..976da7fd9c54 --- /dev/null +++ b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/PACKAGE @@ -0,0 +1,5 @@ +load("//:defs.bzl", "init_cfg_constructor") + +# This call to set_cfg_constructor should be silently ignored +# because this is an external cell, not the root cell. +init_cfg_constructor() diff --git a/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/TARGETS.fixture b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/TARGETS.fixture new file mode 100644 index 000000000000..ef4684894696 --- /dev/null +++ b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/TARGETS.fixture @@ -0,0 +1,6 @@ +load(":rules.bzl", "test_rule") + +test_rule( + name = "external_test", + default_target_platform = "root//:platform", +) diff --git a/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/defs.bzl b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/defs.bzl new file mode 100644 index 000000000000..dd4c46ae2b11 --- /dev/null +++ b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/defs.bzl @@ -0,0 +1,51 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is dual-licensed under either the MIT license found in the +# LICENSE-MIT file in the root directory of this source tree or the Apache +# License, Version 2.0 found in the LICENSE-APACHE file in the root directory +# of this source tree. You may select, at your option, one of the +# above-listed licenses. + +# This file defines a cfg_constructor for the external cell. +# The set_cfg_constructor call should be silently ignored since this is not the root cell. + +KEY = "buck.cfg_modifiers" + +def _cfg_constructor_pre_constraint_analysis( + legacy_platform, + package_modifiers: dict[str, typing.Any] | None, + target_modifiers: dict[str, typing.Any] | None, + cli_modifiers: list[str], + rule_name: str, + aliases: struct | None, + extra_data: dict[str, typing.Any] | None): + _unused = package_modifiers # buildifier: disable=unused-variable + _unused = target_modifiers # buildifier: disable=unused-variable + _unused = cli_modifiers # buildifier: disable=unused-variable + _unused = extra_data # buildifier: disable=unused-variable + _unused = aliases # buildifier: disable=unused-variable + _unused = rule_name # buildifier: disable=unused-variable + + # Use a distinct label so we can detect if this constructor was used + platform = legacy_platform or PlatformInfo(label = "external_cfg_constructor_SHOULD_NOT_BE_USED", configuration = ConfigurationInfo( + constraints = {}, + values = {}, + )) + return ([], platform) + +def _cfg_constructor_post_constraint_analysis(refs: dict[str, ProviderCollection], params): + _unused = refs # buildifier: disable=unused-variable + _unused = params # buildifier: disable=unused-variable + # Use a distinct label so we can detect if this constructor was used + return PlatformInfo(label = "external_post_constraint_analysis_SHOULD_NOT_BE_USED", configuration = ConfigurationInfo( + constraints = params.configuration.constraints, + values = {}, + )) + +def init_cfg_constructor(): + # This call should be silently ignored since this PACKAGE is in an external cell + set_cfg_constructor( + key = KEY, + stage0 = _cfg_constructor_pre_constraint_analysis, + stage1 = _cfg_constructor_post_constraint_analysis, + ) diff --git a/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/rules.bzl b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/rules.bzl new file mode 100644 index 000000000000..9cb3f1bcf937 --- /dev/null +++ b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/rules.bzl @@ -0,0 +1,18 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is dual-licensed under either the MIT license found in the +# LICENSE-MIT file in the root directory of this source tree or the Apache +# License, Version 2.0 found in the LICENSE-APACHE file in the root directory +# of this source tree. You may select, at your option, one of the +# above-listed licenses. + +def _test_rule(ctx): + _unused = ctx # buildifier: disable=unused-variable + return [ + DefaultInfo(), + ] + +test_rule = rule( + impl = _test_rule, + attrs = {}, +) diff --git a/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/prelude/prelude.bzl b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/prelude/prelude.bzl new file mode 100644 index 000000000000..ca6d157ea33e --- /dev/null +++ b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/prelude/prelude.bzl @@ -0,0 +1,9 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is dual-licensed under either the MIT license found in the +# LICENSE-MIT file in the root directory of this source tree or the Apache +# License, Version 2.0 found in the LICENSE-APACHE file in the root directory +# of this source tree. You may select, at your option, one of the +# above-listed licenses. + +# Minimal prelude for testing diff --git a/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/rules.bzl b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/rules.bzl new file mode 100644 index 000000000000..c7405f18142d --- /dev/null +++ b/tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/rules.bzl @@ -0,0 +1,36 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is dual-licensed under either the MIT license found in the +# LICENSE-MIT file in the root directory of this source tree or the Apache +# License, Version 2.0 found in the LICENSE-APACHE file in the root directory +# of this source tree. You may select, at your option, one of the +# above-listed licenses. + +def _test_platform_impl(ctx): + _unused = ctx # buildifier: disable=unused-variable + return [ + DefaultInfo(), + PlatformInfo( + label = str(ctx.label.raw_target()), + configuration = ConfigurationInfo( + constraints = {}, + values = {}, + ), + ), + ] + +test_platform = rule( + impl = _test_platform_impl, + attrs = {}, +) + +def _test_rule(ctx): + _unused = ctx # buildifier: disable=unused-variable + return [ + DefaultInfo(), + ] + +test_rule = rule( + impl = _test_rule, + attrs = {}, +)