Skip to content

Conversation

@arvi18
Copy link

@arvi18 arvi18 commented Apr 15, 2025

The flag flip forces loads in main repository when using Bazel@HEAD. Flipping it now, so that we can disable autoloads by release of Bazel 9.

PiperOrigin-RevId: 745526309
Change-Id: I7204470c08a66a15b28bc0b0c48f41d69e1694c8

The flag flip forces loads in main repository when using Bazel@HEAD. Flipping it now, so that we can disable autoloads by release of Bazel 9.

PiperOrigin-RevId: 745526309
Change-Id: I7204470c08a66a15b28bc0b0c48f41d69e1694c8
@visz11
Copy link
Collaborator

visz11 commented Aug 7, 2025

/refacto-test

@refacto-test
Copy link

refacto-test bot commented Aug 7, 2025

Refacto is reviewing this PR. Please wait for the review comments to be posted.

@refacto-test
Copy link

refacto-test bot commented Aug 7, 2025

Enabling Autoloads Restriction in Main Repository

👍 Well Done
Consistent Implementation Across Codebase

The PR comprehensively updates all affected files to comply with the new autoloads restriction, ensuring consistent implementation.

Proper Documentation Updates

The change includes updating the flag's default value in BuildLanguageOptions.java, maintaining consistency between code and documentation.

Explicit Rule Loading Implementation

The PR correctly implements explicit rule loading across multiple file types, improving code clarity and dependency tracking.

📌 Files Processed
  • examples/py/BUILD
  • examples/py_native/BUILD
  • examples/py_native/fibonacci/BUILD
  • examples/shell/BUILD
  • src/BUILD
  • src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java
  • src/main/java/com/google/devtools/build/skyframe/BUILD
  • src/main/starlark/tests/builtins_bzl/builtin_test_setup.sh
  • src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test/BUILD.builtin_test
  • src/main/starlark/tests/builtins_bzl/cc/cc_static_library/test/BUILD.builtin_test
  • src/test/java/com/google/devtools/build/lib/blackbox/tests/PythonBlackBoxTest.java
  • src/test/py/bazel/bazel_external_repository_test.py
  • src/test/py/bazel/launcher_test.py
  • src/test/py/bazel/py_test.py
  • src/test/py/bazel/query_test.py
  • src/test/py/bazel/runfiles_test.py
  • src/test/shell/bazel/BUILD
  • src/test/shell/bazel/bazel_build_event_stream_test.sh
  • src/test/shell/bazel/bazel_coverage_cc_test_gcc.sh
  • src/test/shell/bazel/bazel_coverage_cc_test_llvm.sh
  • src/test/shell/bazel/bazel_coverage_java_test.sh
  • src/test/shell/bazel/bazel_example_test.sh
  • src/test/shell/bazel/bazel_hermetic_sandboxing_test.sh
  • src/test/shell/bazel/bazel_java_test.sh
  • src/test/shell/bazel/bazel_java_test_defaults.sh
  • src/test/shell/bazel/bazel_localtest_test.sh
  • src/test/shell/bazel/bazel_proto_library_test.sh
  • src/test/shell/bazel/bazel_random_characters_test.sh
  • src/test/shell/bazel/bazel_rules_java_override_test.sh
  • src/test/shell/bazel/bazel_rules_java_test.sh
  • src/test/shell/bazel/bazel_rules_test.sh
  • src/test/shell/bazel/bazel_sandboxing_test.sh
  • src/test/shell/bazel/bazel_symlink_test.sh
  • src/test/shell/bazel/bazel_test_test.sh
  • src/test/shell/bazel/remote/remote_build_event_uploader_test.sh
  • src/test/shell/bazel/remote/remote_execution_http_test.sh
  • src/test/shell/bazel/remote/remote_execution_test.sh
  • src/test/shell/testenv.sh
📝 Additional Comments
src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java (1)
Potential for Broken Backward Compatibility

Changing the default value of a flag from 'false' to 'true' can break backward compatibility for existing builds that rely on the previous behavior. This change forces all repositories to explicitly load rules, which might cause build failures in projects that haven't been updated to accommodate this change. While this improves security and clarity, it requires careful migration planning.

// TODO: After a deprecation period (e.g., next major release), change default to true
@Option(
    name = "incompatible_disable_autoloads_in_main_repo",
    defaultValue = "false",
    documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
    effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
    metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
    help = "If true, disables the use of implicit file loads in the main repository...")

Standards:

  • Backward Compatibility Principles
  • Software Versioning Best Practices

Comment on lines 952 to +953
public static final String INCOMPATIBLE_DISABLE_AUTOLOADS_IN_MAIN_REPO =
"-incompatible_disable_autoloads_in_main_repo";
"+incompatible_disable_autoloads_in_main_repo";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent Flag Setting in Command Line Options

The PR changes the default value of 'incompatible_disable_autoloads_in_main_repo' to true but also modifies the command line flag from a negative prefix ('-') to a positive prefix ('+'). This creates inconsistency between the flag's default value and its command line representation. When a flag has a '+' prefix, it means the flag is enabled by default and can be disabled with '-'. This inconsistency could lead to confusion and potential security issues if developers mistakenly believe they've disabled autoloads when they haven't.

Suggested change
public static final String INCOMPATIBLE_DISABLE_AUTOLOADS_IN_MAIN_REPO =
"-incompatible_disable_autoloads_in_main_repo";
"+incompatible_disable_autoloads_in_main_repo";
public static final String INCOMPATIBLE_DISABLE_AUTOLOADS_IN_MAIN_REPO =
"-incompatible_disable_autoloads_in_main_repo";
Standards
  • CWE-1173
  • Secure Configuration Management

@visz11
Copy link
Collaborator

visz11 commented Aug 9, 2025

/refacto-test

@refacto-test
Copy link

refacto-test bot commented Aug 9, 2025

Refacto is reviewing this PR. Please wait for the review comments to be posted.

@arvi18 arvi18 closed this Aug 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants