Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce visibility on select() keys #12877

Closed
wants to merge 5 commits into from

Commits on Jan 26, 2021

  1. Enforce visibility on select() keys.

    Example:
    
    my_rule(
      name = "buildme",
      deps = select({
        "//other/package:some_config": [":mydeps"]
      }))
    
    Today, //other/package:some_config is exempt from visibility checking, even
    though it's technically a target dep of "buildme". While this dep is "special"
    vs. other deps in various ways, there's no obvious reason why it needs to
    be special in this way. It adds an unclear corner case exception to
    visibility's API.
    
    Implementation:
    ---------------
    select() keys are not "normal" dependencies and don't generally follow the same code path. Hence them not being automatically visibility checked like others.
    
    In particular, normal dependencies are found in ConfiguredTargetFunction and validity-checked in RuleContext.Builder. select() keys' only purpose is to figure out which other normal dependencies should exist. So there's generally no need to pass them to RuleContext.Builder. Instead, Blaze passes their ConfigMatchingProviders, which remain useful for analysis phase attribute lookups.
    
    RuleContext.Builder needs a ConfiguredTargetAndData to do validity-checking.  This patch passes that information along for select() keys too.
    
    We could alternatively refactor the validity checking logic. But that's an even more invasive change. Or do ad hoc validity checking directly in ConfiguredTargetFunction. But that's duplicating logic we really don't want to keep consistent.
    
    Backward compatibility:
    -----------------------
    This has a very good chance of breaking builds, since all targets default to private visibility and it's unlikely existing config_settings have changed that. Pushing this forward requires an incompatible change flag.
    
    PiperOrigin-RevId: 353131136
    Change-Id: Ia5056745d135732bd35ba0ab099800d982f33f74
    gregestren committed Jan 26, 2021
    Configuration menu
    Copy the full SHA
    520f30a View commit details
    Browse the repository at this point in the history

Commits on Jan 27, 2021

  1. Review feedback.

    gregestren committed Jan 27, 2021
    Configuration menu
    Copy the full SHA
    337d983 View commit details
    Browse the repository at this point in the history
  2. Update doc feedback.

    gregestren committed Jan 27, 2021
    Configuration menu
    Copy the full SHA
    39bbfba View commit details
    Browse the repository at this point in the history
  3. Minor formatting fix.

    gregestren committed Jan 27, 2021
    Configuration menu
    Copy the full SHA
    290c26c View commit details
    Browse the repository at this point in the history
  4. Doc update.

    gregestren committed Jan 27, 2021
    Configuration menu
    Copy the full SHA
    3b9f2e2 View commit details
    Browse the repository at this point in the history