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

[rfc] Allow repository rules to lazily declare environment variable deps #20787

Conversation

rbeasley
Copy link
Contributor

@rbeasley rbeasley commented Jan 8, 2024

This commit adds a new repository context method, getenv, which allows Starlark repository rule implementations to lazily declare environment variable dependencies. This is intended to allow repository rules to establish dependencies on environment variables whose names aren't known in advance.

This is work towards #19511.

Notes

  • I don't speak Java, so expect funny smells and copypasta.
  • rctx.getenv behaves similarly to Python's os.getenv.
  • rctx.getenv is to environment variables what rctx.path(Label) is to files.

Future work

  • Implement bzlmod support.

@rbeasley rbeasley requested a review from lberki as a code owner January 8, 2024 14:10
@github-actions github-actions bot added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels Jan 8, 2024
@rbeasley
Copy link
Contributor Author

rbeasley commented Jan 8, 2024

I reviewed the test failures, and they don't appear to be related to my changes. I don't think my change introduced thread/synchronization bugs, but I need to defer to the experts.

@Wyverald
Copy link
Member

Wyverald commented Jan 8, 2024

Thanks for the PR! I'll take a look in a minute. Anyhow the test failures are definitely unrelated; see #20790

This commit adds a new repository context method, `getenv`, which allows
Starlark repository rule implementations to lazily declare environment
variable dependencies.  This is intended to allow repository rules to
establish dependencies on environment variables whose names aren't known
in advance.

This is work towards bazelbuild#19511.

Notes
=====
- I don't speak Java, so expect funny smells and copypasta.
- `rctx.getenv` behaves similarly to Python's `os.getenv`.
- `rctx.getenv` is to environment variables what `rctx.path(Label)` is to
  files.

Future work
===========
- Implement bzlmod support.
@rbeasley rbeasley force-pushed the topic/beasleyr-z01/reporuleenv-rebase branch from 04f5939 to 7202924 Compare January 11, 2024 00:51
@rbeasley rbeasley requested a review from Wyverald January 11, 2024 01:35
Copy link
Member

@Wyverald Wyverald left a comment

Choose a reason for hiding this comment

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

thanks!

@Wyverald Wyverald added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jan 17, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jan 18, 2024
@brentleyjones
Copy link
Contributor

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jan 19, 2024
@Wyverald
Copy link
Member

@bazel-io fork 7.1.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jan 19, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jan 19, 2024
…riable deps (#20944)

This commit adds a new repository context method, `getenv`, which allows
Starlark repository rule implementations to lazily declare environment
variable dependencies. This is intended to allow repository rules to
establish dependencies on environment variables whose names aren't known
in advance.

This is work towards #19511.

Notes
=====
- I don't speak Java, so expect funny smells and copypasta.
- `rctx.getenv` behaves similarly to Python's `os.getenv`.
- `rctx.getenv` is to environment variables what `rctx.path(Label)` is
to files.

Future work
===========
- Implement bzlmod support.

RELNOTES: Added a new method `repository_ctx.getenv`, which allows
Starlark repository rules to declare environment variable dependencies
during the fetch, instead of upfront using `repository_rule.environ`.

Closes #20787.
Commit
c230e39

PiperOrigin-RevId: 599568358
Change-Id: I2c1948cd23643d28bf1d41e9baaf98a902112cc7

Co-authored-by: Ryan Beasley <beasleyr@vmware.com>
@lberki
Copy link
Contributor

lberki commented Jan 24, 2024

What happens if one downgrades the Bazel version in an existing output base from one that supports the ENV: tag in marker files to one that doesn't?

@rbeasley
Copy link
Contributor Author

What happens if one downgrades the Bazel version in an existing output base from one that supports the ENV: tag in marker files to one that doesn't?

@lberki Just to make sure I understand correctly, the ENV: tag already exists before this change and is populated according to repository_rule's environ attr. My change overloaded the ENV: tag to also support lazy env var deps via rctx.getenv.

With that, consider the following sequence of events:

  • User upgrades to Bazel 7.1.0 (or some release containing this feature).
  • User writes or updates a repository rule that uses rctx.env where ENV:key is added to the marker file lazily.

If the user downgrades Bazel to an older version, they'll have to revert their repository rule definition (because rctx.getenv doesn't exist in the older Bazel version). Since this involves changing the contents of the .bzl file where the rule is defined, the corresponding "rule key" would change (as pointed out by @Wyverald above at #20787 (comment)), voiding the marker entirely.

Does that sound right? Am I missing anything?

@lberki
Copy link
Contributor

lberki commented Jan 26, 2024

No, you are not missing anything -- I didn't realize in my (very literal) fever dream that resulted in the above comment that ENV: was already written to the marker files before this pull request. Sorry for the noise and carry on!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants