Repository: bazelbuild/bazel
Issue: Singlejar: Add support for long paths on Windows
Labels: type: bug
P2
area-Windows
team-OSS


### Description of the problem / feature request:

Invoking "singlejar.exe" sometimes fails on Windows when then argument file (the ".params" file) is located in a path longer than 260 chracters.

### Feature requests: what underlying problem are you trying to solve with this feature?

Add support for long path to singlejar.exe. There is a TODO comment in the source file that long path support should be added:


```
      // TODO(laszlocsomor): use the fopen and related file handling API
      // implementations from ProtoBuf, in order to support long paths:
      // https://github.com/google/protobuf/blob/
      //   47b7d2c7cadf74ceec90fc5042232819cd0dd557/
      //   src/google/protobuf/stubs/io_win32.cc
      // Best would be to extract that library to a common location and use
      // here, in ProtoBuf, and in Bazel itself.
```


Issue comments:
Here is an example of output with the failure"

```
Running bazel from D:\src\studio\studio-master-dev\tools\base\bazel\bazel.cmd
INFO: Invocation ID: 05193316-ecc5-4ade-aadf-8bfb69e76e45
INFO: Analysed target //tools/adt/idea/android:artifacts (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
ERROR: D:/src/studio/studio-master-dev/tools/adt/idea/uitest-framework/testSrc/com/android/tools/idea/tests/gui/framework/heapassertions/bleak/agents/BUILD:26:1: Building deploy jar tools/adt/idea/uitest-framework/testSrc/com/android/tools/idea/tests/gui/framework/heapassertions/bleak/agents/ObjectSizeInstrumentationAgent_deploy.jar failed (Exit 1): singlejar.exe failed: error executing command
  cd D:/src/longpath-longpath/longpath-longpath/.bazel/3d5seqf5/execroot/__main__
external/bazel_tools/tools/jdk/singlejar/singlejar.exe @bazel-out/x64_windows-fastbuild/bin/tools/adt/idea/uitest-framework/testSrc/com/android/tools/idea/tests/gui/framework/heapassertions/bleak/agents/ObjectSizeInstrumentationAgent_deploy.jar-0.params
Execution platform: @bazel_tools//platforms:host_platform
ERROR: bazel-out/x64_windows-fastbuild/bin/tools/adt/idea/uitest-framework/testSrc/com/android/tools/idea/tests/gui/framework/heapassertions/bleak/agents/ObjectSizeInstrumentationAgent_deploy.jar-0.params
Target //tools/adt/idea/android:artifacts failed to build
INFO: Elapsed time: 4.170s, Critical Path: 0.35s
INFO: 0 processes.
FAILED: Build did NOT complete successfully
```

Also, regarding the comment in the source code, I don't think it is required to add a dependency on protobuf to support long paths on Windows. A simple(r) fix would be to call "_wfopen" instead of "fopen" with an absolute path prefixed with "\\?\".
Oh, I missed this TODO comment when I ported singlejar on Windows. PR coming.

Pull request review comments:
@laszlocsomor If I changed the filename from 260 `'A'` to 250 `'A'`, the test will pass. Do you know if long path support of Win32 API has the restriction that each path component cannot exceed certain length even with `\\?`' prefix? Don't see anything from https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file
Yes, individual path components may not exceed ~260 characters. (Can't remember the exact limit and can't find where I read it)
> Because you cannot use the "\\?\" prefix with a relative path, relative paths are always limited to a total of MAX_PATH characters.

Seems like this implies that each path component of an UNC-prefixed absolute path cannot exceed `MAX_PATH` (255). At least this is the maximum value I can set until the test starts to fail.
Yes, that's my understanding.
I suggest testing long path behavior by creating directories like "aaaaaaaa.aaa\bbbbbbbb.bbb\cccccccc.ccc\..."

These cannot be shortened to 8dot3 style so you can reliably compose a long path.
> This type of path is composed of components separated by backslashes, each up to the value returned in the lpMaximumComponentLength parameter of the GetVolumeInformation function (this value is commonly 255 characters).

Found on https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file#fully-qualified-vs-relative-paths
> I suggest testing long path behavior by creating directories like "aaaaaaaa.aaa\bbbbbbbb.bbb\cccccccc.ccc..."

Slightly more troublesome since I need to recursively create the directories with `_wmkdir`, but also done now.
For the record I meant something else, but this will do too.

(I meant creating a path whose directory components are all 8.3 style paths already. Bazel's JNI library shortens paths to 8.3 style when creating processes, because processes can only be created with short paths, and I thought `_wfopen` might, in theory, do the same, but I realize this is too paranoid to test for.)

Patch that resolved the issue:
diff --git a/src/tools/singlejar/BUILD b/src/tools/singlejar/BUILD
index 2cf4acbd8e3ee4..e969cee73b8bd4 100644
--- a/src/tools/singlejar/BUILD
+++ b/src/tools/singlejar/BUILD
@@ -453,7 +453,10 @@ cc_library(
 cc_library(
     name = "token_stream",
     hdrs = ["token_stream.h"],
-    deps = [":diag"],
+    deps = [
+        ":diag",
+        "//src/main/cpp/util",
+    ],
 )
 
 filegroup(
diff --git a/src/tools/singlejar/token_stream.h b/src/tools/singlejar/token_stream.h
index 3bad0a883dd540..d94c2576a40c0d 100644
--- a/src/tools/singlejar/token_stream.h
+++ b/src/tools/singlejar/token_stream.h
@@ -23,6 +23,7 @@
 #include <utility>
 #include <vector>
 
+#include "src/main/cpp/util/path_platform.h"
 #include "src/tools/singlejar/diag.h"
 
 /*
@@ -57,14 +58,19 @@ class ArgTokenStream {
   class FileTokenStream {
    public:
     FileTokenStream(const char *filename) {
-      // TODO(laszlocsomor): use the fopen and related file handling API
-      // implementations from ProtoBuf, in order to support long paths:
-      // https://github.com/google/protobuf/blob/
-      //   47b7d2c7cadf74ceec90fc5042232819cd0dd557/
-      //   src/google/protobuf/stubs/io_win32.cc
-      // Best would be to extract that library to a common location and use
-      // here, in ProtoBuf, and in Bazel itself.
-      if (!(fp_ = fopen(filename, "r"))) {
+#ifdef _WIN32
+      std::wstring wpath;
+      std::string error;
+      if (!blaze_util::AsAbsoluteWindowsPath(filename, &wpath, &error)) {
+        diag_err(1, "%s:%d: AsAbsoluteWindowsPath failed: %s", __FILE__, __LINE__,
+                 error.c_str());
+      }
+      fp_ = _wfopen(wpath.c_str(), L"r");
+#else
+      fp_ = fopen(filename, "r");
+#endif
+
+      if (!fp_) {
         diag_err(1, "%s", filename);
       }
       filename_ = filename;
diff --git a/src/tools/singlejar/token_stream_test.cc b/src/tools/singlejar/token_stream_test.cc
index 3cd28077cd6136..2a900ab855425c 100644
--- a/src/tools/singlejar/token_stream_test.cc
+++ b/src/tools/singlejar/token_stream_test.cc
@@ -15,7 +15,9 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string>
+#include <wchar.h>
 
+#include "src/main/cpp/util/path_platform.h"
 #include "src/tools/singlejar/test_util.h"
 #include "src/tools/singlejar/token_stream.h"
 #include "googletest/include/gtest/gtest.h"
@@ -79,6 +81,54 @@ TEST(TokenStreamTest, CommandFile) {
   EXPECT_TRUE(token_stream.AtEnd());
 }
 
+#ifdef _WIN32
+// '-foo @commandfile -bar' command line.
+TEST(TokenStreamTest, CommandFileLongPath) {
+  const char *tempdir = getenv("TEST_TMPDIR");
+  ASSERT_NE(nullptr, tempdir);
+
+  std::wstring wpath;
+  std::string error;
+  std::string command_file_path = singlejar_test_util::OutputFilePath(std::string(251, 'a'));
+  command_file_path += ".aaa\\";
+  ASSERT_TRUE(blaze_util::AsAbsoluteWindowsPath(command_file_path, &wpath, &error)) << error;
+  ASSERT_EQ(0, _wmkdir(wpath.c_str()));
+
+  command_file_path += std::string(251, 'b');
+  command_file_path += ".bbb\\";
+  ASSERT_TRUE(blaze_util::AsAbsoluteWindowsPath(command_file_path, &wpath, &error)) << error;
+  ASSERT_EQ(0, _wmkdir(wpath.c_str()));
+
+  command_file_path += std::string(251, 'c');
+  command_file_path += ".ccc";
+
+  ASSERT_TRUE(blaze_util::AsAbsoluteWindowsPath(command_file_path, &wpath, &error)) << error;
+
+  FILE *fp = _wfopen(wpath.c_str(), L"w");
+  ASSERT_NE(nullptr, fp);
+  for (size_t i = 0; i < ARRAY_SIZE(lines); ++i) {
+    fprintf(fp, "%s\n", lines[i]);
+  }
+  fclose(fp);
+
+  std::string command_file_arg = std::string("@") + command_file_path;
+  const char *args[] = {"-before_file", "", "-after_file"};
+  args[1] = command_file_arg.c_str();
+  ArgTokenStream token_stream(ARRAY_SIZE(args), args);
+  bool flag = false;
+  ASSERT_TRUE(token_stream.MatchAndSet("-before_file", &flag));
+  EXPECT_TRUE(flag);
+  for (size_t i = 0; i < ARRAY_SIZE(expected_tokens); ++i) {
+    flag = false;
+    ASSERT_TRUE(token_stream.MatchAndSet(expected_tokens[i], &flag));
+    EXPECT_TRUE(flag);
+  }
+  ASSERT_TRUE(token_stream.MatchAndSet("-after_file", &flag));
+  EXPECT_TRUE(flag);
+  EXPECT_TRUE(token_stream.AtEnd());
+}
+#endif
+
 // '--arg1 optval1 --arg2' command line.
 TEST(TokenStreamTest, OptargOne) {
   const char *args[] = {"--arg1", "optval1", "--arg2", "--arg3", "optval3"};


Test patch: