-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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
syscall: Go Build Fails on Windows With Long Paths #18468
Comments
My guess is that problem is that the magic we added to the Maybe we need to move the /cc @quentinmit |
And /cc @alexbrainman |
@bradfitz - I think this would be handled by the caller of Exec instead of in syscall directly since they are opaque strings and the difference between a file path and any other arbitrary string is determined by the caller. I do agree that this and the os package should share the same path-fixing code. I also agree it should be an internal function for now until more thought is given to expose (maybe |
Why not have |
@harshavardhana If the go std lib handles windows long filepaths, which I think is great, I don't think it would be good to expose it anywhere except (maybe) in syscall, as this is a really platform specific issue where the fix doesn't need to affect API. |
Yeah, no new public API.
|
👍 |
We very consciously did not make syscall transparently handle long paths, because it would be confusing if the resulting syscall was called with different arguments than the caller intended. Probably the right thing to do is to teach |
Actually, I'll send out a fix. |
CL https://golang.org/cl/34725 mentions this issue. |
Not trivial, it turns out. Somebody should tackle it during Go 1.9. https://golang.org/cl/34725 might be a good starting point. It has some debugging notes in it and a test. |
If I was to guess, the UNC meses with the quote logic; guess only. |
Note: #21145, just closed as a dup of this issue, has an handy Powershell script that creates a folder that triggers the problem. |
After reviewing #21145 I think the CL that Brad had started ( https://golang.org/cl/34725 ) is barking up the wrong tree. In that CL it attempts to fixup the process name to use the UNC path equivalent that the rest of the os functions can use. However, as noted in the CL, CreateProcess doesn't accept UNC paths for the process name. Looking at this issue again the process name is short I suspect that the error is actually referring to the argument length (Windows doesn't really differentiate between the two in error message). From https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx
Two ways to fix this would be to instrument the Go compilers to take a new argument "-args-on-stdin=true" flag where the exec would read from stdin for args or pass the argument list file to the executable "@C:\Temp\arglist" where the given file is read off of disk and used as arguments. |
It would be nice to solve the general problem, but perhaps Windows does not provide a mechanism for that. If we can only solve the problem for cmd/go, then of course we should do that. For the record, GCC and friends do this by treating any argument that starts with |
I’m running into this issue and I’d like to contribute a fix. If the maintainers would like me to teach the toolchain about response files, I’m happy to take a swing at it, but I have some questions:
|
@ellismg before deciding how to fix this, can you provide me with steps to reproduce it. Thank you Alex |
Sure. In the case I'm hitting, the problem is just that we construct a command line that is too long (>32K characters) when we try to exec into the compiler. This works:
If you add a bunch of additional files (depending on how deep your source code is, you may need more copies of the dummy file, to construct a command line that is over the windows limit):
Then
Here's my system information:
Locally, I did a hacky thing where I just taught compiler.exe about a Nothing long path specific here, as @kardianos surmised. Just generating a very long command that ends up being over a Windows limit. |
Something like The response files should be parsed exactly as the Microsoft tools parse them. I believe they permit multiple options on a single line, and permit quoting with backslash, single quote, and double quote. |
@arizvisa, thanks for testing and noting the |
…ments into a response file if the total length of all the args is larger than the limitation imposed by windows. Also Modified runOut to use this when executing a sub-command. Closes issue golang#18468
@bradfitz, hey man. I noticed that one of the reviewers of your patch suggested the idea of going the path of response files. I made a branch that goes down this route, and adds '@'-prefixed response files to the same place that your patch added your fix. I essentially re-implemented the At the moment its dev'd against go1.10.1. If the community prefers this solution for some reason, though, I can rebase it against master and create a PR so it can be merged upstream. However, I could use another pair of eyes to sanity check how I'm escaping things when writing to the response file (since it's pretty common to have mistakes here). The branch I'm referring to is at arizvisa/golang#GH-18468 |
…ments into a response file if the total length of all the args is larger than the limitation imposed by windows. Also Modified runOut to use this when executing a sub-command. Closes issue golang#18468
Actually, another update. I didn't realize go1.10.2 came out, so I rebased it for that tag. lmk.. |
This adds support for parsing response files to all tools. This was done by re-implementing the buildargv func from gcc's libiberty/argv.c and then updating Flagparse to look for '@'- prefixed filenames. If one is found, then buildargv is used to expand the command line args from the response file into the tool's arguments. On windows, there's a space limitation for commandline args due to windows internally using UNICODE_STRING which has a 32k limit. Adding support for response files allows compile and link to act on packages that have more than 32k worth of filenames. This includes a capability that is necessary to close golang#18468. src/cmd/internal/objabi/flag.go: added buildargv and updated Flagparse
This modifies golang's tools to check if the total number of arguments will result in a commandline larger than 32k. If we're on windows, this will then replace the commandline args with a response file which should bypass the 32k limitation. This was done by adding a function that copies the arguments into a response file if the total length of the args is larger than the windows limit. Each of the arguments written to the response file are newline delimited, double-quoted, and escaped. Closes issue golang#18468. src/cmd/go/internal/work/exec.go: added buildLongCommand and escapeString
@arizvisa if you want your change looked at, you have to use tools that this this repo uses - https://golang.org/doc/contribute.html on how to contribute. Or just create a PR here on master branch. It was @ianlancetaylor who suggested to use response files on https://golang.org/cl/110395. If you post your change here, maybe it will be prefered to CL 110395. Alex |
@alexbrainman, I don't want to step on bradfitz's toes as it was his original fix, I was just offering some direction as it was based on his original work and his response to my complaining. It works for me, but I also don't really want to maintain it or anything ;-) as I'm not really a developer. Plus, I had realized that I implemented it from a GPL-licensed piece of software which I'm pretty sure is a licensing conflict. |
Sounds good to me. I gave Brad +2 now, so he can submit his CL.
It could well be. Alex |
I've submitted 17fbb83 (https://go-review.googlesource.com/c/go/+/110395). @arizvisa, please double check that it works for you? I at least made it such that when it runs on our build farm, 10% of the compile/link invocations go through this code path, so it will get some exercise even for non-large build arguments. |
@bradfitz, sorry to get back to you so late as I've been ill for a bit. It seems to work without issue. So, thanks! One thing not too important but worth thinking about in the future, the way you split lines in Regardless though. Thanks for getting this fixed! |
I don't think we need to care about "alternate data streams", which I assume are like Mac "resource forks"? |
Ah, the api call suceeds. (Although I'm using touch to avoid writing a program).
Result of the file in the old cmd prompt.
Then a streams which isn't related, plus I couldn't get a newline added as a stream (even using the api)
I'm not really a mac guy so my exp w/ resource forks are pretty spotty. but alternate data streams are typically used for things like metadata as they have no particular data structure. NTFS exposes some interesting default ADS which one can use to get some pretty interesting side effects out of software on windows. But as an example for a legitimate thing, browsers will add an ADS to a downloaded file so that a file can be noted as being "unsafe". But regardless about platform differences/semantics, I'm just happy things are fixed and I just have tendencies to note side effects about things which is what that stripping of The implementation of Again though, thanks for fixing this! (edited to fix some type-o's and the missing backticks around CR) |
Is this supposed to be fixed in 1.10.2? I just upgraded to the latest version and I'm still getting the error. |
You could patch it in and recompile Go if it's urgent. But I suppose we could also backport it to Go 1.10.x if we do another one. @gopherbot, please backport. |
Backport issue(s) opened: #25292 (for 1.10). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
Opening by request in #3358
What version of Go are you using (
go version
)?What operating system and processor architecture are you using (
go env
)?Win64
What did you do?
Tried to compile several files, many with long names, in a folder via go build
What did you expect to see?
Successful compile
What did you see instead?
The text was updated successfully, but these errors were encountered: