Ignore set_cfg_constructor() in external cells

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(

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()

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

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

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()

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",
+)

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,
+    )

tests/core/configurations/cfg_constructor/test_external_cell_cfg_constructor_data/external_cell/.buckconfig

@@ -0,0 +1,2 @@
+[buildfile]
+name = TARGETS.fixture

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()

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",
+)

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,
+    )

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 = {},
+)

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

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 = {},
+)