From 104f1ddd4f0feb6d0e9ad435dba877adf7b2a11f Mon Sep 17 00:00:00 2001 From: Ulrik Falklof Date: Tue, 7 Sep 2021 08:38:13 +0200 Subject: [PATCH] Review updates --- .../starlark/FunctionTransitionUtil.java | 60 ++++++++++--------- .../StarlarkAttrTransitionProviderTest.java | 1 - 2 files changed, 31 insertions(+), 30 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java index 9c2b6e0817b057..8c96c9c02efdd0 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java @@ -275,15 +275,15 @@ private static BuildOptions applyTransition( Object optionValue = entry.getValue(); if (!optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) { + Label optionLabel = Label.parseAbsoluteUnchecked(optionName); Object oldValue = - buildOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName)); + buildOptions.getStarlarkOptions().get(optionLabel); if ((oldValue == null && optionValue != null) || (oldValue != null && optionValue == null) || (oldValue != null && !oldValue.equals(optionValue))) { // TODO(bazel-team): Figure out if we need to create a whole new build options every // time. Can we just keep track of the running changes and actually build a new build // options after this loop? - Label optionLabel = Label.parseAbsoluteUnchecked(optionName); buildOptions = BuildOptions.builder() .merge(buildOptions) @@ -383,10 +383,10 @@ private static BuildOptions applyTransition( } /** - * Compute the output directory name fragment corresponding to the new BuildOptions based on (1) - * the names and values of all native options previously transitioned anywhere in the build by - * starlark options, (2) names and values of entries in the starlark options map previously - * transitioned anywhere in the build and not only set on command line. + * Compute the output directory name fragment corresponding to the new BuildOptions based on + * the names and values of all options (both native and Starlark) previously transitioned + * anywhere in the build by Starlark transitions. Options only set on command line are + * not affecting the computation. * * @param changedOptions the names of all options changed by this transition in label form e.g. * "//command_line_option:cpu" for native options and "//myapp:foo" for starlark options. @@ -407,41 +407,31 @@ private static void updateOutputDirectoryNameFragment( } CoreOptions buildConfigOptions = toOptions.get(CoreOptions.class); - Set updatedAffectedByStarlarkTransition = - new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition); - // Add newly changed options to overall list of changed options - for (String option : changedOptions) { - updatedAffectedByStarlarkTransition.add(option); - } - buildConfigOptions.affectedByStarlarkTransition = - ImmutableList.sortedCopyOf(updatedAffectedByStarlarkTransition); - TreeMap toHash = new TreeMap<>(); - // Hash all affected native options. + updateAffectedByStarlarkTransition(buildConfigOptions, changedOptions); + + TreeMap toHash = new TreeMap<>(); for (String optionName : buildConfigOptions.affectedByStarlarkTransition) { if (optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) { String nativeOptionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length()); Object value; try { value = - optionInfoMap - .get(nativeOptionName) - .getDefinition() - .getField() - .get(toOptions.get(optionInfoMap.get(nativeOptionName).getOptionClass())); + optionInfoMap + .get(nativeOptionName) + .getDefinition() + .getField() + .get(toOptions.get(optionInfoMap.get(nativeOptionName).getOptionClass())); } catch (IllegalAccessException e) { throw new RuntimeException( "IllegalAccess for option " + nativeOptionName + ": " + e.getMessage()); } toHash.put(optionName, value); - } - } - - // Hash all affected starlark options. - for (Map.Entry opt : toOptions.getStarlarkOptions().entrySet()) { - String optionName = opt.getKey().toString(); - if (updatedAffectedByStarlarkTransition.contains(optionName)) { - toHash.put(optionName, opt.getValue()); + } else { + Object value = toOptions.getStarlarkOptions().get(Label.parseAbsoluteUnchecked(optionName)); + if (value != null) { + toHash.put(optionName, value); + } } } @@ -454,6 +444,18 @@ private static void updateOutputDirectoryNameFragment( transitionDirectoryNameFragment(hashStrs.build()); } + /** + * Extend the global build config affectedByStarlarkTransition, by adding any new option names + * from changedOptions + */ + private static void updateAffectedByStarlarkTransition(CoreOptions buildConfigOptions, Set changedOptions) { + Set mutableCopyToUpdate = new TreeSet<>(buildConfigOptions.affectedByStarlarkTransition); + for (String option : changedOptions) { + mutableCopyToUpdate.add(option); + } + buildConfigOptions.affectedByStarlarkTransition = ImmutableList.sortedCopyOf(mutableCopyToUpdate); + } + public static String transitionDirectoryNameFragment(Iterable opts) { Fingerprint fp = new Fingerprint(); for (String opt : opts) { diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java index c80ff8801e86e9..d0abd1cc8fe38e 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java @@ -1470,7 +1470,6 @@ public void testOutputDirHash_starlarkOption_differentBoolRepresentationsNotEqua .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment( ImmutableList.of("//test:foo=1"))); - assertThat(getCoreOptions(dep).transitionDirectoryNameFragment) .isEqualTo( FunctionTransitionUtil.transitionDirectoryNameFragment(