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

MSVC / wrapper script bug: cannot compile files with long path #2406

Closed
laszlocsomor opened this issue Jan 24, 2017 · 36 comments
Closed

MSVC / wrapper script bug: cannot compile files with long path #2406

laszlocsomor opened this issue Jan 24, 2017 · 36 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) platform: windows type: bug
Milestone

Comments

@laszlocsomor
Copy link
Contributor

laszlocsomor commented Jan 24, 2017

Description of the problem / feature request / question:

If the default or user-specified --output_user_root is too long, then Bazel cannot compile //third_party/protobuf/3.0.0:protoc_lib which has a long path:

c1xx: fatal error C1081: 'C:\Users\laszlocsomor\AppData\Local\Temp\_bazel_laszlocsomor\nYhaG828\execroot\bazel\bazel-out\vc_14_0_x64-fastbuild\bin\third_party\protobuf\3.0.0\_objs\protoc_lib\third_party\protobuf\3.0.0\src\google\protobuf\compiler\objectivec\objectivec_primitive_field.pdb': file name too long

If possible, provide a minimal example to reproduce the problem:

git checkout 2fabee2
bazel build src:bazel

Then take that binary and run this build:

C:/Users/LASZLO~1/AppData/Local/Temp/_bazel_laszlocsomor/Oh5NykPf/execroot/bazel2/bazel-out/local-fastbuild/bin/src/bazel build --cpu=x64_windows_msvc //third_party/protobuf/3.0.0:protoc_lib

Its output is:

(...)
INFO: Found 1 target...
ERROR: C:/work/bazel/third_party/protobuf/3.0.0/BUILD:209:1: C++ compilation of rule '//third_party/protobuf/3.0.0:protoc_lib' failed: msvc_cl.bat failed: error executing command
  cd C:/Users/laszlocsomor/AppData/Local/Temp/_bazel_laszlocsomor/nYhaG828/execroot/bazel
  SET PATH=external/local_config_cc/wrapper/bin
  external/local_config_cc/wrapper/bin/msvc_cl.bat -m64 /D__inline__=__inline /DOS_WINDOWS=OS_WINDOWS /DCOMPILER_MSVC /DNOGDI /DNOMINMAX /DPRAGMA_SUPPORTED /D_WIN32_WINNT=0x0600 /D_CRT_SECURE_NO_DEPRECATE /D_CRT_SECURE_NO_WARNINGS /D_SILENCE_STDEXT_HASH_DEPRECATION_WARNINGS /D_USE_MATH_DEFINES /nologo /bigobj /Zm500 /J /Gy /GF /W3 /EHsc /wd4351 /wd4291 /wd4250 /wd4996 /DNDEBUG /Od -Xcompilation-mode=fastbuild /I. /Ibazel-out/vc_14_0_x64-fastbuild/genfiles /Iexternal/bazel_tools /Ibazel-out/vc_14_0_x64-fastbuild/genfiles/external/bazel_tools /Ithird_party/protobuf/3.0.0/src /Ibazel-out/vc_14_0_x64-fastbuild/genfiles/third_party/protobuf/3.0.0/src /Iexternal/bazel_tools/tools/cpp/gcc3 /DEPENDENCY_FILE bazel-out/vc_14_0_x64-fastbuild/bin/third_party/protobuf/3.0.0/_objs/protoc_lib/third_party/protobuf/3.0.0/src/google/protobuf/compiler/objectivec/objectivec_primitive_field.d /c third_party/protobuf/3.0.0/src/google/protobuf/compiler/objectivec/objectivec_primitive_field.cc /Fobazel-out/vc_14_0_x64-fastbuild/bin/third_party/protobuf/3.0.0/_objs/protoc_lib/third_party/protobuf/3.0.0/src/google/protobuf/compiler/objectivec/objectivec_primitive_field.o -DHAVE_PTHREAD -Wall -Wwrite-strings -Woverloaded-virtual -Wno-sign-compare -Wno-error=unused-function -Wno-error=unused-variable: com.google.devtools.build.lib.shell.BadExitStatusException: Process exited with status 1.
Warning: path "C:\Users\laszlocsomor\AppData\Local\Temp\_bazel_laszlocsomor\nYhaG828\execroot\bazel\bazel-out\vc_14_0_x64-fastbuild\bin\third_party\protobuf\3.0.0\_objs\protoc_lib\third_party\protobuf\3.0.0\src\google\protobuf\compiler\objectivec\objectivec_primitive_field" is > than 260 characters (258); programs may crash with long arguments
Warning: Unmatched arguments: -Wwrite-strings -Woverloaded-virtual -Wno-error=unused-function -Wno-error=unused-variable
c1xx: fatal error C1081: 'C:\Users\laszlocsomor\AppData\Local\Temp\_bazel_laszlocsomor\nYhaG828\execroot\bazel\bazel-out\vc_14_0_x64-fastbuild\bin\third_party\protobuf\3.0.0\_objs\protoc_lib\third_party\protobuf\3.0.0\src\google\protobuf\compiler\objectivec\objectivec_primitive_field.pdb': file name too long
Traceback (most recent call last):
  File "C:\Users\laszlocsomor\AppData\Local\Temp\_bazel_laszlocsomor\nYhaG828\execroot\bazel\external\local_config_cc\wrapper\bin\pydir\msvc_cl.py", line 125, in <module>
    sys.exit(main(sys.argv[1:]))  # need to skip the first argument
  File "C:\Users\laszlocsomor\AppData\Local\Temp\_bazel_laszlocsomor\nYhaG828\execroot\bazel\external\local_config_cc\wrapper\bin\pydir\msvc_cl.py", line 121, in main
    return MsvcCompiler().Run(argv[1:])
  File "C:\Users\laszlocsomor\AppData\Local\Temp\_bazel_laszlocsomor\nYhaG828\execroot\bazel\external\local_config_cc\wrapper\bin\pydir\msvc_cl.py", line 113, in Run
    return self.RunBinary(compiler, parser.options, parser.target_arch, parser)
  File "C:\Users\laszlocsomor\AppData\Local\Temp\_bazel_laszlocsomor\nYhaG828\execroot\bazel\external\local_config_cc\wrapper\bin\pydir\msvc_tools.py", line 537, in RunBinary
    with open(parser.deps_file, 'w') as deps_file:
IOError: [Errno 2] No such file or directory: 'bazel-out/vc_14_0_x64-fastbuild/bin/third_party/protobuf/3.0.0/_objs/protoc_lib/third_party/protobuf/3.0.0/src/google/protobuf/compiler/objectivec/objectivec_primitive_field.d'
ERROR: C:/work/bazel/third_party/protobuf/3.0.0/BUILD:209:1: output 'third_party/protobuf/3.0.0/_objs/protoc_lib/third_party/protobuf/3.0.0/src/google/protobuf/compiler/objectivec/objectivec_message_field.o' was not created.
Target //third_party/protobuf/3.0.0:protoc_lib failed to build
INFO: Elapsed time: 3.634s, Critical Path: 2.87s

Environment info

  • Operating System: Windows 10

  • If bazel info release returns "development version" or "(@non-git)", please tell us what source tree you compiled Bazel from; git commit hash is appreciated (git rev-parse HEAD):

git rev-parse HEAD
2fabee2d329a0fa4bd69cf2c1fc82b74d17ed744
@laszlocsomor laszlocsomor self-assigned this Jan 24, 2017
@laszlocsomor laszlocsomor added P2 We'll consider working on this in future. (Assignee optional) type: bug labels Jan 24, 2017
@laszlocsomor
Copy link
Contributor Author

Workaround is to use --output_user_root=/c/tmp.

@abergmeier-dsfishlabs
Copy link
Contributor

Workaround is to use --output_user_root=/c/tmp.

For you maybe, but even that does not help on our codebase.

@laszlocsomor
Copy link
Contributor Author

Two questions then:

  • If converted to a 8dot3 style short path, would your paths fit into 259 chars?
    BTW another trick you can use to further shorten the paths is:
    @rem In cmd.exe:
    @subst d: c:\path\to\scratch\dir
    
  • What's the value of this key in your system(s)?
    HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\FileSystem\NtfsDisable8dot3NameCreation
    

@laszlocsomor
Copy link
Contributor Author

  • If converted to a 8dot3 style short path, would your paths fit into 259 chars?
    BTW another trick you can use to further shorten the paths is:
    @rem In cmd.exe:
    @subst d: c:\path\to\scratch\dir
    

...and pass --output_user_root=/d

@abergmeier-dsfishlabs
Copy link
Contributor

abergmeier-dsfishlabs commented Jan 31, 2017

...and pass --output_user_root=/d

Does not work for me:

Error: blaze_util::MakeCanonical('/b\dXgGrvqy') failed:

@laszlocsomor
Copy link
Contributor Author

You may need to restart MSYS.
Make sure the new virtual drive is in /etc/mtab.

@abergmeier-dsfishlabs
Copy link
Contributor

You may need to restart MSYS.

Done

Make sure the new virtual drive is in /etc/mtab.

Is in.
What are the minimum rights that the bazel server needs?

@laszlocsomor
Copy link
Contributor Author

Shouldn't need admin rights, or do you mean something else?
Are you still getting an error? What's the output?

@laszlocsomor
Copy link
Contributor Author

And what's the command line you're running?

@abergmeier-dsfishlabs
Copy link
Contributor

Now manually created

  • B:\dXgGrvqy
  • B:\install

and given Everyone full access to B:.
but it still seems as if Bazel is not able to write anything:

bazel --output_user_root /b build foo

Leads to:

Extracting Bazel installation...
Error: install base directory '/b\install\ef18c8a0c4758f558e9141b77d899cfd.tmp.12356' could not be renamed into place: (error: 6): The handle is invalid.

@laszlocsomor
Copy link
Contributor Author

Ah, yes, that is a known issue I'm fixing today or tomorrow.

@laszlocsomor
Copy link
Contributor Author

For now what I can recommend is try disabling your antivirus for the first Bazel run.

@laszlocsomor
Copy link
Contributor Author

I suspect the reason is, as we extract the embedded binaries into the temp install directory, the AV starts scanning them, acquiring and holding file handles to them.
When we're done extracting we attempt to rename the directory, but because the AV is holding open files in there, we fail to.

@abergmeier-dsfishlabs
Copy link
Contributor

abergmeier-dsfishlabs commented Jan 31, 2017

For now what I can recommend is try disabling your antivirus for the first Bazel run.

Disabled as much as I could - still same error message. Might be that some part of antivirus is still running, though.

@laszlocsomor
Copy link
Contributor Author

Hm, can you try this:

git checkout 46dacc9f9407065bc3db0938dfda97724911cee2

Then apply this patch:

diff --git a/src/main/cpp/blaze.cc b/src/main/cpp/blaze.cc
index e26b41dab..86901b1f8 100644
--- a/src/main/cpp/blaze.cc
+++ b/src/main/cpp/blaze.cc
@@ -919,12 +919,20 @@ static void ExtractData(const string &self_path) {
     // Now rename the completed installation to its final name. If this
     // fails due to an ENOTEMPTY then we assume another good
     // installation snuck in before us.
-    if (rename(tmp_install.c_str(), globals->options->install_base.c_str()) ==
+    int attempts = 1;
+    while (rename(tmp_install.c_str(), globals->options->install_base.c_str()) ==
             -1 &&
         errno != ENOTEMPTY) {
-      pdie(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR,
-           "install base directory '%s' could not be renamed into place",
-           tmp_install.c_str());
+      if (attempts == 120) {
+        pdie(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR,
+             "install base directory '%s' could not be renamed into place",
+             tmp_install.c_str());
+      } else {
+        printf("Couldn't rename temp install directory after %d attempt(s)\r",
+               attempts);
+        std::this_thread::sleep_for(std::chrono::seconds(1));
+      }
+      ++attempts;
     }
   } else {
     if (!blaze_util::IsDirectory(globals->options->install_base)) {

then build bazel, and see whether this solves the renaming issue?
I cannot repro it no matter how I try.

@abergmeier-dsfishlabs
Copy link
Contributor

Hm, can you try this:

Perhaps lets rather do that at FOSDEM then?

@laszlocsomor
Copy link
Contributor Author

Sure.

@abergmeier-dsfishlabs
Copy link
Contributor

abergmeier-dsfishlabs commented Feb 3, 2017

Using junction seems to work with 0.4.4. Seems like 855fbe9 was not the best version to pick.

@laszlocsomor laszlocsomor added this to the 0.5 milestone Feb 14, 2017
@laszlocsomor laszlocsomor added P1 I'll work on this now. (Assignee required) and removed P2 We'll consider working on this in future. (Assignee optional) labels Feb 14, 2017
@laszlocsomor
Copy link
Contributor Author

I hit this bug again today, again the --output_user_root workaround helped. We should fix this for the 0.5 milestone.

@dslomov
Copy link
Contributor

dslomov commented Feb 28, 2017

@laszlocsomor on track fro 0.5?

@laszlocsomor
Copy link
Contributor Author

Yes!

@laszlocsomor
Copy link
Contributor Author

I have a change that fixes about 5 long path related bugs. Now I can build bazel with a long output_user_root. Yay!

bazel-io pushed a commit that referenced this issue Mar 6, 2017
The hard limit for SetCurrentDirectory{A,W} is
MAX_PATH-1, even with UNC prefix, therefore a
process' cwd may also not be longer than that.

See #2107
See #2406
See #2181

--
PiperOrigin-RevId: 149290147
MOS_MIGRATED_REVID=149290147
@laszlocsomor
Copy link
Contributor Author

Ok, I was testing more with longer paths and found a couple things:

  1. MSVC cl.exe cannot deal with long paths at all. I can write a hello world program that takes a long path argument and can CreateFileW at that path (after prepending the path with "\\?\"), therefore this is not a limitation of cmd.exe but of cl.exe

  2. We have no way of shortening paths in the msvc python wrapper. I couldn't build even a tiny example C extension library for python 2.7 (kept getting linker errors), so implementing the shortening logic in C and exposing it to Python seems infeasible. We'd have to pass long paths from the Java side. But then again, see (1).

  3. Another, more generic problem is that most of, if not all, the Actions use execroot-relative paths, meaning even if the execroot is shortened, the relative paths of inputs and outputs may push the combined length over the limit. Given that only absolute paths can be shortened, and we of course don't know the execroot during analysis (when the Actions are created) because it's determined by the executor (and with remote execution this wouldn't even be known at all), we cannot shorten these paths ahead of time; the action itself would have to shorten it and of course we cannot expect every single action to reimplement that logic. So we need to think harder about a generic solution, potentially involving per-action junctions under something like "c:/bazel1234/action5678/{inputs,outputs}/" and passing these per-action execroot- (or junction-) -relative paths instead of passing paths relative to the generic execroot.

@abergmeier-dsfishlabs
Copy link
Contributor

abergmeier-dsfishlabs commented Mar 9, 2017

Another, more generic problem is that most of, if not all, the Actions use execroot-relative paths, meaning even if the execroot is shortened, the relative paths of inputs and outputs may push the combined length over the limit. Given that only absolute paths can be shortened, and we of course don't know the execroot during analysis (when the Actions are created) because it's determined by the executor (and with remote execution this wouldn't even be known at all), we cannot shorten these paths ahead of time; the action itself would have to shorten it and of course we cannot expect every single action to reimplement that logic. So we need to think harder about a generic solution, potentially involving per-action junctions under something like "c:/bazel1234/action5678/{inputs,outputs}/" and passing these per-action execroot- (or junction-) -relative paths instead of passing paths relative to the generic execroot.

With regards to "general" handling, I would rather have a special commandline flag where you can tell Bazel to pass shortened label paths to a certain Mnemonic.

Everything else is up to the action implementation. Actually most of our actions e.g. are working fine with prefixed paths.

@laszlocsomor
Copy link
Contributor Author

With regards to "general" handling, I would rather have a special commandline flag where you can tell Bazel to pass shortened label paths a certain Mnemonic.

What do you mean? Can you show an example?

@abergmeier-dsfishlabs
Copy link
Contributor

What do you mean? Can you show an example?

I was thinking about in bazel.rc:

# Force paths exposed to MyAction to be "shortened"
build --path_variant=MyAction=shortened-paths

in myaction.bzl:

ctx.action(
    outputs = [output],
    inputs = [f.path for f in ctx.files.srcs],
)

where inputs gives you a \\?\ path by default and a "shortened" with the above rc entry.
Everything else should IMO be handled by the rule/action impl.

IF we want a "workaround" (for third party binaries that cannot handle long paths (WTF)), I would be rigorous and force users to provide a drive where Bazel can create a junction. Something like c:/bazel1234/action5678/{inputs,outputs}/ sounds a bit too half-assed 😏

@laszlocsomor
Copy link
Contributor Author

laszlocsomor commented Mar 9, 2017

It's actually desirable that actions don't get an absolute path for inputs [1] because this way the execroot can be anything, enabling actions to be reproducible and deterministic, regardless of the machine or the execroot path they run under. Every action has a working directory (AFAIK this is always the execroot), and all their inputs are available as relative paths underneath it.

Requiring an output drive [2] is a promising idea, but doesn't solve the problem of having packages that are several directories deep and have longish names, because then your paths are execroot + longish package name + file name, easy to reach the 260 char limit. And then there are actions like C++ compilation where you could have a cc_library "//foo:lib" with two sources "//bar:a.cc" and "//baz:a.cc", so
it's not enough to put lib's outputs under bazel-genfiles/foo/lib.outs/ because the two a.cc files would produce two a.o files (conflict); you also need to embed the source files' package paths, e.g. bazel-genfiles/foo/lib.outs/bar/a.o, meaning now we have two package paths in an output file's path, so our chances of depleting the 260 chars is twice as high.

And finally there's the problem that it's not only third-party software not supporting long paths, it's Microsoft's own C++ compiler too. This means we cannot rely on the action to handle long paths, we have to run the action under a short root with short input/output paths. So I think using junctions is the direction we need to go in.

[1] therefore also not a shortened path, nor a \\?\-prefixed one
[2] the user or Bazel could create a virtual one with subst for example

@petemounce
Copy link
Contributor

Fixed by #2181 ...?

@laszlocsomor
Copy link
Contributor Author

No, unfortunately not. cl.exe cannot handle long paths, period. #2181 fixed Bazel's own usage of the Windows API. cl.exe is run by the wrapper script (//tools/cpp/wrapper/bin/msvc_cl.bat) which we also author but it's not built directly into Bazel, it's just a tool that Bazel uses.

@dslomov
Copy link
Contributor

dslomov commented Apr 4, 2017

Lowering the priority of this, as this is cl.exe's issue.

@dslomov dslomov added P2 We'll consider working on this in future. (Assignee optional) and removed P1 I'll work on this now. (Assignee required) labels Apr 4, 2017
@dslomov dslomov modified the milestones: 0.6, 0.5 Apr 4, 2017
@abergmeier
Copy link
Contributor

abergmeier commented Jun 10, 2017

Ok, I was testing more with longer paths and found a couple things:

  1. MSVC cl.exe cannot deal with long paths at all. I can write a hello world program that takes a long path argument and can CreateFileW at that path (after prepending the path with "\?"), therefore this is not a limitation of cmd.exe but of cl.exe

@laszlocsomor Do you want to answer https://twitter.com/apardoe/status/873212726968856578?

@laszlocsomor
Copy link
Contributor Author

@abergmeier : Here's a repro: https://gist.github.com/laszlocsomor/720f9c739a0c825896a3b33473255f33
Prefix any of the paths passed to cl.exe with "\\?\" and see that cl.exe can no longer use them.

@abergmeier-dsfishlabs
Copy link
Contributor

@abergmeier : Here's a repro: https://gist.github.com/laszlocsomor/720f9c739a0c825896a3b33473255f33

Thx. Passed that along.

@laszlocsomor
Copy link
Contributor Author

This will definitely not be done until 0.6, and in all honesty I doubt it'll become a priority at all before 1.0... so I'm marking this as 1.0 for now.

@laszlocsomor laszlocsomor modified the milestones: 1.0, 0.6 Jul 24, 2017
@laszlocsomor
Copy link
Contributor Author

related: #4149

@laszlocsomor
Copy link
Contributor Author

Closing this in favour of #4149. That bug also deals with C++ compilation of files with long paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) platform: windows type: bug
Projects
None yet
Development

No branches or pull requests

5 participants