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

New env() BUILD file function. #17652

Merged
merged 9 commits into from
Jan 11, 2023

Conversation

kaos
Copy link
Member

@kaos kaos commented Nov 27, 2022

Closes #12797

Make BUILD file parsing a two pass operation, where the first pass is just walking the AST to fish out any env(...) calls so we can provide the requested env vars for the second pass.

Example:

# src/python/some/BUILD
python_distribution(
  name="my-dist",
  description=env("DESC_TEXT", "demo the `env()` function, with default value."),
  provides=python_artifact(
    name="my-dist",
    version=env("MY_DIST_VERSION"),
  )
)

@kaos kaos marked this pull request as draft November 28, 2022 14:02
@kaos kaos force-pushed the issue/12797-env-vars-in-BUILD-files branch from 3e3be08 to 8231bce Compare January 7, 2023 02:49
@kaos kaos marked this pull request as ready for review January 7, 2023 02:58
@kaos kaos requested a review from stuhood January 7, 2023 02:58
@kaos
Copy link
Member Author

kaos commented Jan 7, 2023

I think it may be good enough to only be able to use the env vars of the local system, even when using remote execution--as long as it's a known and documented limitation.

At least I've not been able to figure out how to make it work otherwise.. ;)

@kaos kaos requested a review from thejcannon January 7, 2023 14:53
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Great idea!

src/python/pants/engine/internals/build_files.py Outdated Show resolved Hide resolved
@kaos kaos enabled auto-merge (squash) January 10, 2023 20:45
@kaos kaos merged commit d9315ca into pantsbuild:main Jan 11, 2023
@kaos kaos deleted the issue/12797-env-vars-in-BUILD-files branch January 11, 2023 21:01
@kaos
Copy link
Member Author

kaos commented Jan 11, 2023

@benjyw 🎉 your aws s3 fix seems to have helped :)

Environment variables
---------------------

BUILD files are very hermetic in nature with no support for using `import` or other I/O operations. In order to have dynamic data in BUILD files, you may inject values from the local environment using the `env()` function. It takes the variable name and optional default value as arguments.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to snipe this review after merge. Can we document whether sensitive env vars are safe to load? Why/why not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds like a good idea. I don't know the answer though.. :)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I mean... it just depends where they are being used, I suppose? Technically you could use this to interpolate an env var into a target name or something, at which point your secret would leak via ./pants list.

So it's very use-site specific.

@TalAmuyal
Copy link
Contributor

Joining the snipe here, can you document whether or not the env-var value is part of the cache key?
I'd like to use it for the package-version, since it is computed (in our case) based on the build-number.

In this case, in each new build (even for the same commit), the version would change and I wouldn't want it to invalidate the package.

@kaos
Copy link
Member Author

kaos commented Jan 18, 2023

Joining the snipe here, can you document whether or not the env-var value is part of the cache key? I'd like to use it for the package-version, since it is computed (in our case) based on the build-number.

In this case, in each new build (even for the same commit), the version would change and I wouldn't want it to invalidate the package.

When packaging a target, the hash value of that target will be part of the cache key, and if any of the target's fields values change the hash value change.

In other words, yes the env-var value will be part of the cache key--as will it be if injected by any other means using the SetupKwargs plugin API for instance.

Your use case is a bit tricky, as you change some input which has a noticeable effect on the output but wish it to not trigger a rebuild. For that to work, we'd need to have a way to mark values with a constant hash regardless of its underlying value in order to not invalidate previous work when changed.

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

Successfully merging this pull request may close these issues.

Support for Environment variables in BUILD files.
4 participants