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

Improve safety of using environments during BUILD evaluation time #17354

Open
Tracked by #17355
stuhood opened this issue Oct 26, 2022 · 1 comment
Open
Tracked by #17355

Improve safety of using environments during BUILD evaluation time #17354

stuhood opened this issue Oct 26, 2022 · 1 comment

Comments

@stuhood
Copy link
Member

stuhood commented Oct 26, 2022

Currently, build graph calculations and BUILD file evaluation are pinned to the __local__ environment (see #17129), meaning that it will not run in a docker, or remote environment. This is accomplished by "masking" the environment at graph and BUILD file evaluation boundaries, and then explicit injecting the ChosenLocalEnvironmentName as the current EnvironmentName.

While that process is enforced (by the masked_type annotations) and fairly safe, it is not type safe for @rule authors who might think that they can use the Platform or EnvironmentVars during BUILD file evaluation time or dependency inference. Because they can access those types in those locations, but they will not get the appropriate values to use at runtime: they will instead get the values for the __local__ environment.

If you know that, then you can manually avoid consuming environment-specific types in those @rules, and defer the work to, say, codegen (as @thejcannon demonstrates here), but making this type safe instead would definitely be preferable.


One way to help guide users would be to overload all of the environment-aware intrinsics (Process running, Platform calculation, EnvironmentVars calculation, etc) to consume either:

  • an EnvironmentName (as they do today in 2.15.x), representing the runtime environment of a target
  • a type specific to build graph evaluation, or explicitly for the local environment, maybe called LocalEnvironmentName or BuildGraphEnvironmentName.

The latter type would be available to @rules which were part of BUILD file evaluation and dependency calculation, while the former type would be available to codegen and etc. This would bifurcate @rules by type, and maybe help to guide @rule authors to avoid consuming the "wrong" Platform. But because dependency inference still needs to be able to run processes, it would not represent true type safety.

@stuhood stuhood changed the title Improve type safety of using environments during BUILD evaluation time Improve safety of using environments during BUILD evaluation time Oct 26, 2022
@stuhood
Copy link
Member Author

stuhood commented Oct 26, 2022

The latter type would be available to @rules which were part of BUILD file evaluation and dependency calculation, while the former type would be available to codegen and etc. This would bifurcate @rules by putting constraints on what could be done in each subgraph, but it would prevent accidentally consuming the "wrong" Platform.

Mm... unfortunately, this would still only act as a guideline/hint not to produce platform-specific outputs in these subgraphs, rather than providing actual type safety. Dependency calculations need to be able to run processes (and locate interpreters, etc), so they would need to be able to consume all of the platform-specific types. But they would need to guarantee that they produced platform-independent/agnostic outputs, because otherwise the computed dependencies could be unintentionally different on different __local__ platforms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

1 participant