Use \n line breaks in build-info.properties on all OSes

Ensures that the `build-info.properties` in deploy JARs has the same content across host operating systems.

Along the way fixes a few other tests to get `//src/test/java/com/google/devtools/build/lib/rules/java/...` to pass on Windows.

Work towards #4292

Closes #18073.

PiperOrigin-RevId: 524864425
Change-Id: Iac5076788b92bd35d22e58d851447cee5a66fcc5

.bazelci/postsubmit.yml

@@ -345,7 +345,6 @@ tasks:
       - "-//src/test/java/com/google/devtools/build/lib/query2/cquery/..."
       - "-//src/test/java/com/google/devtools/build/lib/query2/engine/..."
       - "-//src/test/java/com/google/devtools/build/lib/versioning/..."
-      - "-//src/test/java/com/google/devtools/build/lib/rules/java/..."
       - "-//src/test/java/com/google/devtools/build/lib/worker/..."
       - "-//src/test/java/com/google/devtools/build/lib/remote:remote"
       - "-//src/test/shell/bazel/remote/..."

.bazelci/presubmit.yml

@@ -357,7 +357,6 @@ tasks:
       - "-//src/test/java/com/google/devtools/build/lib/query2/cquery/..."
       - "-//src/test/java/com/google/devtools/build/lib/query2/engine/..."
       - "-//src/test/java/com/google/devtools/build/lib/versioning/..."
-      - "-//src/test/java/com/google/devtools/build/lib/rules/java/..."
       - "-//src/test/java/com/google/devtools/build/lib/worker/..."
       - "-//src/test/java/com/google/devtools/build/lib/remote:remote"
       - "-//src/test/shell/bazel/remote/..."

src/main/java/com/google/devtools/build/lib/rules/java/WriteBuildInfoPropertiesAction.java

@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.analysis.actions.DeterministicWriter;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.util.Fingerprint;
+import java.io.BufferedWriter;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.io.OutputStreamWriter;
@@ -108,6 +109,23 @@ public void close() throws IOException {
     }
   }
 
+  /**
+   * A {@link BufferedWriter} that always writes {@code \n} in {@link BufferedWriter#newLine()},
+   * regardless of the value of the {@code line.separator} system property. This can be used to get
+   * consistent output from {@link Properties#store(Writer, String)} across host OSes.
+   */
+  private static class NewLineNormalizingBufferedWriter extends BufferedWriter {
+    public NewLineNormalizingBufferedWriter(Writer out) {
+      super(out);
+    }
+
+    @Override
+    public void newLine() throws IOException {
+      // super.newLine() uses System#lineSeparator(), which differs across platforms.
+      write('\n');
+    }
+  }
+
   /**
    * Creates an action that writes a Java property files with build information.
    *
@@ -175,11 +193,16 @@ public void writeOutputFile(OutputStream out) throws IOException {
         addValues(keys, values, context.getStableKeys());
         Properties properties = new DeterministicProperties();
         keyTranslations.translate(keys, properties);
-        properties.store(new StripFirstLineWriter(out), null);
+        storeNormalized(properties, out);
       }
     };
   }
 
+  @VisibleForTesting
+  static void storeNormalized(Properties properties, OutputStream out) throws IOException {
+    properties.store(new NewLineNormalizingBufferedWriter(new StripFirstLineWriter(out)), null);
+  }
+
   private void addValues(
       Map<String, String> result, Map<String, String> values, Map<String, Key> keys) {
     boolean redacted = values.isEmpty();

src/test/java/com/google/devtools/build/lib/packages/BUILD

@@ -183,5 +183,6 @@ java_library(
         "//third_party:guava",
         "//third_party:jsr305",
         "//third_party:junit4",
+        "@bazel_tools//tools/java/runfiles",
     ],
 )

src/test/java/com/google/devtools/build/lib/packages/util/MockToolsConfig.java

@@ -18,7 +18,9 @@
 import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.util.FileSystems;
+import com.google.devtools.build.runfiles.Runfiles;
 import java.io.IOException;
 import javax.annotation.Nullable;
 
@@ -157,8 +159,13 @@ public void copyTool(String relativePath) throws IOException {
   }
 
   public void copyTool(String relativePath, String dest) throws IOException {
-    Path runfiles = FileSystems.getNativeFileSystem().getPath(BlazeTestUtils.runfilesDir());
-    Path source = runfiles.getRelative(TestConstants.WORKSPACE_NAME).getRelative(relativePath);
+    // Tests are assumed to be run from the main repository only.
+    Runfiles runfiles = Runfiles.preload().withSourceRepository("");
+    PathFragment rlocationPath =
+        PathFragment.create(TestConstants.WORKSPACE_NAME).getRelative(relativePath);
+    Path source =
+        FileSystems.getNativeFileSystem()
+            .getPath(runfiles.rlocation(rlocationPath.getPathString()));
     create(dest, FileSystemUtils.readLinesAsLatin1(source).toArray(String[]::new));
   }
 

src/test/java/com/google/devtools/build/lib/rules/java/BUILD

@@ -159,6 +159,7 @@ java_test(
         ":java_toolchain_test_util",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
+        "//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
         "//src/main/java/com/google/devtools/build/lib/analysis:actions/template_expansion_action",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
@@ -170,6 +171,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/rules/cpp",
         "//src/main/java/com/google/devtools/build/lib/rules/java:java-compilation",
         "//src/main/java/com/google/devtools/build/lib/util:filetype",
+        "//src/main/java/com/google/devtools/build/lib/util:os",
         "//src/main/java/net/starlark/java/eval",
         "//src/test/java/com/google/devtools/build/lib/actions/util",
         "//src/test/java/com/google/devtools/build/lib/analysis/util",

src/test/java/com/google/devtools/build/lib/rules/java/JavaStarlarkApiTest.java

@@ -13,17 +13,20 @@
 // limitations under the License.
 package com.google.devtools.build.lib.rules.java;
 
+import static com.google.common.collect.MoreCollectors.onlyElement;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
 import static com.google.common.truth.Truth8.assertThat;
 import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.prettyArtifactNames;
 import static com.google.devtools.build.lib.rules.java.JavaCompileActionTestHelper.getProcessorNames;
 import static com.google.devtools.build.lib.rules.java.JavaCompileActionTestHelper.getProcessorPath;
+import static java.util.stream.Collectors.joining;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.FileProvider;
@@ -43,10 +46,14 @@
 import com.google.devtools.build.lib.rules.java.JavaRuleOutputJarsProvider.JavaOutput;
 import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.util.FileType;
+import com.google.devtools.build.lib.util.OS;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Objects;
+import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Sequence;
 import net.starlark.java.eval.StarlarkList;
 import org.junit.Before;
@@ -1976,10 +1983,8 @@ public void javaBinary_propagatesDirectNativeLibrariesInJavaInfo() throws Except
     setBuildLanguageOptions("--experimental_google_legacy_api");
     ConfiguredTarget testTarget = getConfiguredTarget("//foo:binary");
 
-    TemplateExpansionAction action =
-        (TemplateExpansionAction) getGeneratingAction(getExecutable(testTarget));
     // Check that the directory name is on the java.library.path
-    assertThat(action.getFileContents())
+    assertThat(getJvmFlags(getExecutable(testTarget)))
         .containsMatch("-Djava.library.path=\\$\\{JAVA_RUNFILES\\}/.*/foo");
   }
 
@@ -2006,10 +2011,8 @@ public void javaTest_propagatesDirectNativeLibrariesInJavaInfo() throws Exceptio
         "java_test(name = 'test', test_class='test', srcs = ['Test.java'], deps = [':r'])");
 
     ConfiguredTarget testTarget = getConfiguredTarget("//foo:test");
-    TemplateExpansionAction action =
-        (TemplateExpansionAction) getGeneratingAction(getExecutable(testTarget));
     // Check that the directory name is on the java.library.path
-    assertThat(action.getFileContents())
+    assertThat(getJvmFlags(getExecutable(testTarget)))
         .containsMatch("-Djava.library.path=\\$\\{JAVA_RUNFILES\\}/.*/foo");
   }
 
@@ -3619,4 +3622,20 @@ public void testEnforceExplicitJavaTestDepsIsPrivateApi() throws Exception {
 
     assertContainsEvent("Rule in 'foo' cannot use private API");
   }
+
+  private String getJvmFlags(Artifact executable)
+      throws CommandLineExpansionException, InterruptedException, EvalException {
+    if (OS.getCurrent() == OS.WINDOWS) {
+      return getGeneratingSpawnActionArgs(executable).stream()
+          .filter(a -> a.startsWith("jvm_flags="))
+          .flatMap(a -> Arrays.stream(a.substring("jvm_flags=".length()).split("\t")))
+          .collect(joining(" "));
+    } else {
+      return ((TemplateExpansionAction) getGeneratingAction(executable))
+          .getSubstitutions().stream()
+              .filter(s -> Objects.equals(s.getKey(), "%jvm_flags%"))
+              .collect(onlyElement())
+              .getValue();
+    }
+  }
 }

src/test/java/com/google/devtools/build/lib/rules/java/WriteBuildInfoPropertiesActionTest.java

@@ -17,6 +17,7 @@
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.base.Joiner;
+import com.google.devtools.build.lib.rules.java.WriteBuildInfoPropertiesAction.StripFirstLineWriter;
 import com.google.devtools.build.lib.testutil.FoundationTestCase;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
@@ -34,13 +35,12 @@ public class WriteBuildInfoPropertiesActionTest extends FoundationTestCase {
 
   private void assertStripFirstLine(String expected, String... testCases) throws IOException {
     ByteArrayOutputStream out = new ByteArrayOutputStream();
-    try (WriteBuildInfoPropertiesAction.StripFirstLineWriter writer =
-        new WriteBuildInfoPropertiesAction.StripFirstLineWriter(out)) {
+    try (StripFirstLineWriter writer = new StripFirstLineWriter(out)) {
       for (String testCase : testCases) {
         writer.write(testCase);
       }
     }
-    assertThat(new String(out.toByteArray(), UTF_8)).isEqualTo(expected);
+    assertThat(out.toString(UTF_8)).isEqualTo(expected);
   }
 
   @Test
@@ -65,10 +65,7 @@ public void deterministicProperties() throws IOException {
     Properties underTest = new WriteBuildInfoPropertiesAction.DeterministicProperties();
     underTest.put("second", "keyb");
     underTest.put("first", "keya");
-    try (WriteBuildInfoPropertiesAction.StripFirstLineWriter writer =
-        new WriteBuildInfoPropertiesAction.StripFirstLineWriter(bytes)) {
-      underTest.store(writer, null);
-    }
-    assertThat(new String(bytes.toByteArray(), UTF_8)).isEqualTo("first=keya\nsecond=keyb\n");
+    WriteBuildInfoPropertiesAction.storeNormalized(underTest, bytes);
+    assertThat(bytes.toString(UTF_8)).isEqualTo("first=keya\nsecond=keyb\n");
   }
 }