Skip to content

Commit c230e39

Browse files
rbeasleycopybara-github
rbeasley
authored and
copybara-github
committed
[rfc] Allow repository rules to lazily declare environment variable deps
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. Closes #20787. PiperOrigin-RevId: 599568358 Change-Id: I2c1948cd23643d28bf1d41e9baaf98a902112cc7
1 parent 1536e62 commit c230e39

7 files changed

Lines changed: 187 additions & 10 deletions

File tree

‎site/en/extending/repo.md‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -108,12 +108,11 @@ following things changes:
108108

109109
* The attributes passed to the repo rule invocation.
110110
* The Starlark code comprising the implementation of the repo rule.
111-
* The value of any environment variable declared with the `environ`
112-
attribute of the [`repository_rule`](/rules/lib/globals/bzl#repository_rule).
113-
The values of these environment variables can be hard-wired on the command
114-
line with the
115-
[`--action_env`](/reference/command-line-reference#flag--action_env)
116-
flag (but this flag will invalidate every action of the build).
111+
* The value of any environment variable passed to `repository_ctx`'s
112+
`getenv()` method or declared with the `environ` attribute of the
113+
[`repository_rule`](/rules/lib/globals/bzl#repository_rule). The values
114+
of these environment variables can be hard-wired on the command line with the
115+
[`--repo_env`](/reference/command-line-reference#flag--repo_env) flag.
117116
* The content of any file passed to the `read()`, `execute()` and similar
118117
methods of `repository_ctx` which is referred to by a label (for example,
119118
`//mypkg:label.txt` but not `mypkg/label.txt`)

‎src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ java_library(
4040
"//src/main/java/com/google/devtools/build/lib/rules:repository/workspace_attribute_mapper",
4141
"//src/main/java/com/google/devtools/build/lib/rules:repository/workspace_file_helper",
4242
"//src/main/java/com/google/devtools/build/lib/shell",
43+
"//src/main/java/com/google/devtools/build/lib/skyframe:action_environment_function",
4344
"//src/main/java/com/google/devtools/build/lib/skyframe:ignored_package_prefixes_value",
4445
"//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
4546
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository",

‎src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import com.google.common.base.Strings;
2323
import com.google.common.collect.ImmutableList;
2424
import com.google.common.collect.ImmutableMap;
25+
import com.google.common.collect.ImmutableSet;
2526
import com.google.common.collect.ImmutableSortedMap;
2627
import com.google.common.collect.Maps;
2728
import com.google.common.util.concurrent.Futures;
@@ -51,6 +52,7 @@
5152
import com.google.devtools.build.lib.runtime.ProcessWrapper;
5253
import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor;
5354
import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor.ExecutionResult;
55+
import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction;
5456
import com.google.devtools.build.lib.util.OsUtils;
5557
import com.google.devtools.build.lib.util.io.OutErr;
5658
import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -77,6 +79,7 @@
7779
import java.util.Arrays;
7880
import java.util.Base64;
7981
import java.util.HashMap;
82+
import java.util.HashSet;
8083
import java.util.List;
8184
import java.util.Map;
8285
import java.util.Optional;
@@ -89,6 +92,7 @@
8992
import net.starlark.java.annot.StarlarkMethod;
9093
import net.starlark.java.eval.Dict;
9194
import net.starlark.java.eval.EvalException;
95+
import net.starlark.java.eval.NoneType;
9296
import net.starlark.java.eval.Printer;
9397
import net.starlark.java.eval.Sequence;
9498
import net.starlark.java.eval.Starlark;
@@ -137,6 +141,7 @@ private interface AsyncTask {
137141
@Nullable private final ProcessWrapper processWrapper;
138142
protected final StarlarkSemantics starlarkSemantics;
139143
private final HashMap<Label, String> accumulatedFileDigests = new HashMap<>();
144+
private final HashSet<String> accumulatedEnvKeys = new HashSet<>();
140145
private final RepositoryRemoteExecutor remoteExecutor;
141146
private final List<AsyncTask> asyncTasks;
142147

@@ -193,6 +198,11 @@ public ImmutableMap<Label, String> getAccumulatedFileDigests() {
193198
return ImmutableMap.copyOf(accumulatedFileDigests);
194199
}
195200

201+
/** Returns set of environment variable keys encountered so far. */
202+
public ImmutableSet<String> getAccumulatedEnvKeys() {
203+
return ImmutableSet.copyOf(accumulatedEnvKeys);
204+
}
205+
196206
protected void checkInOutputDirectory(String operation, StarlarkPath path)
197207
throws RepositoryFunctionException {
198208
if (!path.getPath().getPathString().startsWith(workingDirectory.getPathString())) {
@@ -1054,6 +1064,47 @@ public void createFile(
10541064
}
10551065
}
10561066

1067+
// Move to a common location like net.starlark.java.eval.Starlark?
1068+
@Nullable
1069+
private static <T> T nullIfNone(Object object, Class<T> type) {
1070+
return object != Starlark.NONE ? type.cast(object) : null;
1071+
}
1072+
1073+
@StarlarkMethod(
1074+
name = "getenv",
1075+
doc =
1076+
"Returns the value of an environment variable <code>name</code> as a string if exists, "
1077+
+ "or <code>default</code> if it doesn't."
1078+
+ "<p>When building incrementally, any change to the value of the variable named by "
1079+
+ "<code>name</code> will cause this repository to be re-fetched.",
1080+
parameters = {
1081+
@Param(
1082+
name = "name",
1083+
doc = "name of desired environment variable",
1084+
allowedTypes = {@ParamType(type = String.class)}),
1085+
@Param(
1086+
name = "default",
1087+
doc = "Default value to return if `name` is not found",
1088+
allowedTypes = {@ParamType(type = String.class), @ParamType(type = NoneType.class)},
1089+
defaultValue = "None")
1090+
},
1091+
allowReturnNones = true)
1092+
@Nullable
1093+
public String getEnvironmentValue(String name, Object defaultValue)
1094+
throws InterruptedException, NeedsSkyframeRestartException {
1095+
// Must look up via AEF, rather than solely copy from `this.envVariables`, in order to
1096+
// establish a SkyKey dependency relationship.
1097+
if (env.getValue(ActionEnvironmentFunction.key(name)) == null) {
1098+
throw new NeedsSkyframeRestartException();
1099+
}
1100+
1101+
// However, to account for --repo_env we take the value from `this.envVariables`.
1102+
// See https://github.com/bazelbuild/bazel/pull/20787#discussion_r1445571248 .
1103+
String envVarValue = envVariables.get(name);
1104+
accumulatedEnvKeys.add(name);
1105+
return envVarValue != null ? envVarValue : nullIfNone(defaultValue, String.class);
1106+
}
1107+
10571108
@StarlarkMethod(
10581109
name = "path",
10591110
doc =

‎src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkOS.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,16 @@ public boolean isImmutable() {
4242
return true; // immutable and Starlark-hashable
4343
}
4444

45-
@StarlarkMethod(name = "environ", structField = true, doc = "The list of environment variables.")
45+
@StarlarkMethod(
46+
name = "environ",
47+
structField = true,
48+
doc =
49+
"The dictionary of environment variables."
50+
+ "<p><b>NOTE</b>: Retrieving an environment variable from this dictionary does not "
51+
+ "establish a dependency from a repository rule or module extension to the "
52+
+ "environment variable. To establish a dependency when looking up an "
53+
+ "environment variable, use either <code>repository_ctx.getenv</code> or "
54+
+ "<code>module_ctx.getenv</code> instead.")
4655
public ImmutableMap<String, String> getEnvironmentVariables() {
4756
return environ;
4857
}

‎src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@
5555
import com.google.devtools.build.skyframe.SkyKey;
5656
import java.io.IOException;
5757
import java.util.Map;
58+
import java.util.Objects;
59+
import java.util.Set;
5860
import java.util.concurrent.ExecutionException;
5961
import java.util.concurrent.ExecutorService;
6062
import javax.annotation.Nullable;
@@ -323,6 +325,11 @@ private RepositoryDirectoryValue.Builder fetchInternal(
323325
markerData.put("FILE:" + entry.getKey(), entry.getValue());
324326
}
325327

328+
// Ditto for environment variables accessed via `getenv`.
329+
for (String envKey : starlarkRepositoryContext.getAccumulatedEnvKeys()) {
330+
markerData.put("ENV:" + envKey, clientEnvironment.get(envKey));
331+
}
332+
326333
env.getListener().post(resolved);
327334
} catch (NeedsSkyframeRestartException e) {
328335
// A dependency is missing, cleanup and returns null
@@ -373,6 +380,47 @@ private static ImmutableSet<String> getEnviron(Rule rule) {
373380
return ImmutableSet.copyOf((Iterable<String>) rule.getAttr("$environ"));
374381
}
375382

383+
/**
384+
* Verify marker data previously saved by {@link #declareEnvironmentDependencies(Map, Environment,
385+
* Set)} and/or {@link #fetchInternal(Rule, Path, BlazeDirectories, Environment, Map, SkyKey)} (on
386+
* behalf of {@link StarlarkBaseExternalContext#getEnvironmentValue(String, Object)}).
387+
*/
388+
@Override
389+
protected boolean verifyEnvironMarkerData(
390+
Map<String, String> markerData, Environment env, Set<String> keys)
391+
throws InterruptedException {
392+
/*
393+
* We can ignore `keys` and instead only verify what's recorded in the marker file, because
394+
* any change to `keys` between builds would be caused by a change to a .bzl file, and that's
395+
* covered by RepositoryDelegatorFunction.DigestWriter#areRepositoryAndMarkerFileConsistent.
396+
*/
397+
398+
ImmutableSet<String> markerKeys =
399+
markerData.keySet().stream()
400+
.filter(s -> s.startsWith("ENV:"))
401+
.collect(ImmutableSet.toImmutableSet());
402+
403+
ImmutableMap<String, String> environ =
404+
getEnvVarValues(
405+
env,
406+
markerKeys.stream()
407+
.map(s -> s.substring(4)) // ENV:FOO -> FOO
408+
.collect(ImmutableSet.toImmutableSet()));
409+
if (environ == null) {
410+
return false;
411+
}
412+
413+
for (String key : markerKeys) {
414+
String markerValue = markerData.get(key);
415+
String envKey = key.substring(4); // ENV:FOO -> FOO
416+
if (!Objects.equals(markerValue, environ.get(envKey))) {
417+
return false;
418+
}
419+
}
420+
421+
return true;
422+
}
423+
376424
@Override
377425
protected boolean isLocal(Rule rule) {
378426
return (Boolean) rule.getAttr("$local");

‎src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/RepositoryModuleApi.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,10 @@ public interface RepositoryModuleApi {
8484
},
8585
defaultValue = "[]",
8686
doc =
87-
"Provides a list of environment variable that this repository rule depends on. If "
88-
+ "an environment variable in that list change, the repository will be "
87+
"<b>Deprecated</b>. This parameter has been deprecated. Migrate to "
88+
+ "<code>repository_ctx.getenv</code> instead.<br/>"
89+
+ "Provides a list of environment variable that this repository rule depends "
90+
+ "on. If an environment variable in that list change, the repository will be "
8991
+ "refetched.",
9092
named = true,
9193
positional = false),

‎src/test/shell/bazel/external_integration_test.sh‎

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1943,7 +1943,7 @@ EOF
19431943

19441944
function test_cache_hit_reported() {
19451945
# Verify that information about a cache hit is reported
1946-
# if an error happend in that repository. This information
1946+
# if an error happened in that repository. This information
19471947
# is useful as users sometimes change the URL but do not
19481948
# update the hash.
19491949
WRKDIR=$(mktemp -d "${TEST_TMPDIR}/testXXXXXX")
@@ -3000,4 +3000,71 @@ EOF
30003000
test -h "$execroot/external/ext" || fail "Expected symlink to external repo."
30013001
}
30023002

3003+
function test_environ_incrementally() {
3004+
# Set up workspace with a repository rule to examine env vars. Assert that undeclared
3005+
# env vars don't trigger reevaluations.
3006+
cat > repo.bzl <<EOF
3007+
def _impl(rctx):
3008+
rctx.symlink(rctx.attr.build_file, 'BUILD')
3009+
print('UNDECLARED_KEY=%s' % rctx.os.environ.get('UNDECLARED_KEY'))
3010+
print('PREDECLARED_KEY=%s' % rctx.os.environ.get('PREDECLARED_KEY'))
3011+
print('LAZYEVAL_KEY=%s' % rctx.getenv('LAZYEVAL_KEY'))
3012+
3013+
dummy_repository = repository_rule(
3014+
implementation = _impl,
3015+
attrs = {'build_file': attr.label()},
3016+
environ = ['PREDECLARED_KEY'], # sic
3017+
)
3018+
EOF
3019+
cat > BUILD.dummy <<EOF
3020+
filegroup(name='dummy', srcs=['BUILD'])
3021+
EOF
3022+
touch BUILD
3023+
cat > WORKSPACE <<EOF
3024+
load('//:repo.bzl', 'dummy_repository')
3025+
dummy_repository(name = 'foo', build_file = '@@//:BUILD.dummy')
3026+
EOF
3027+
3028+
# Baseline: DEBUG: UNDECLARED_KEY is logged to stderr.
3029+
UNDECLARED_KEY=val1 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed'
3030+
expect_log "UNDECLARED_KEY=val1"
3031+
3032+
# UNDECLARED_KEY is, well, undeclared. This will be a no-op.
3033+
UNDECLARED_KEY=val2 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed'
3034+
expect_not_log "UNDECLARED_KEY"
3035+
3036+
#---
3037+
3038+
# Predeclared key.
3039+
PREDECLARED_KEY=wal1 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed'
3040+
expect_log "PREDECLARED_KEY=wal1"
3041+
3042+
# Predeclared key, no-op build.
3043+
PREDECLARED_KEY=wal1 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed'
3044+
expect_not_log "PREDECLARED_KEY"
3045+
3046+
# Predeclared key, new value -> refetch.
3047+
PREDECLARED_KEY=wal2 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed'
3048+
expect_log "PREDECLARED_KEY=wal2"
3049+
3050+
#---
3051+
3052+
# Side-effect key.
3053+
LAZYEVAL_KEY=xal1 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed'
3054+
expect_log "PREDECLARED_KEY=None"
3055+
expect_log "LAZYEVAL_KEY=xal1"
3056+
3057+
# Side-effect key, no-op build.
3058+
LAZYEVAL_KEY=xal1 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed'
3059+
expect_not_log "LAZYEVAL_KEY"
3060+
3061+
# Side-effect key, new value -> refetch.
3062+
LAZYEVAL_KEY=xal2 bazel query @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed'
3063+
expect_log "LAZYEVAL_KEY=xal2"
3064+
3065+
# Ditto, but with --repo_env overriding environment.
3066+
LAZYEVAL_KEY=xal2 bazel query --repo_env=LAZYEVAL_KEY=xal3 @foo//:BUILD 2>$TEST_log || fail 'Expected no-op build to succeed'
3067+
expect_log "LAZYEVAL_KEY=xal3"
3068+
}
3069+
30033070
run_suite "external tests"

0 commit comments

Comments
 (0)