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

path/filepath: no-op path.filepath.EvalSymLinks on Windows #40104

Closed
ericwj opened this issue Jul 7, 2020 · 65 comments
Closed

path/filepath: no-op path.filepath.EvalSymLinks on Windows #40104

ericwj opened this issue Jul 7, 2020 · 65 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone

Comments

@ericwj
Copy link

ericwj commented Jul 7, 2020

What version of Go are Windows users using (go version)?

$ go version
go version * windows/*

Does this issue reproduce with the latest release?

Yes, it is due to architectural differences between Windows and other operating systems.

What did Windows people do?

Nothing.

We include regular end users of Go programs, including system administrators and developers who use software written in Go like Docker or software which is partially written in or makes use of Go components like Visual Studio and Git.

What did you expect to see?

That software written in go continues to work after disaster recovery or changing a sub directory from being physically present to being a reparse point to a large fresh volume, without hunting down configuration issues with go software because it persisted paths after calling EvalSymLinks, assuming that the path would be more stable than the path configured explicitly by the system administrator after calling EvalSymLinks.

Why should EvalSymLinks no-op on Windows?

On Windows, the path that EvalSymLinks returns is not more stable, rather it is in very common situations random due to the GPT volume identifier being random and is hence the path returned by this API is also not under control of the administrator. The administrator might be forced to change reparse points or hard links due to disk failure, disaster recovery, resolving disk full problems, and similar. Hence, the use of EvalSymLinks for the purposes it is designed for is wrong on Windows, even if its use is justified in the same software on other operating systems.

What did you see instead?

Docker services stop being able to start, even though they should have run and should continue to run from C:\Docker, regardless of the fact that this directory resolves now to a Windows path on a different volume with a different GPT unique identifer and hence a different volume path in the form \\?\Volume{guid}.

@networkimprov
Copy link

Thanks for raising this. It's a proposal (I think) so doesn't have to follow the standard issue template. It's also rather long and a bit grumpy.

Could you thin it down to a list of specific problems (with examples) in the current API, and possible solutions?

Note that the Windows port has long depended on contributions from outside the team at Google.

@ericwj
Copy link
Author

ericwj commented Jul 7, 2020

Well sorry for being grumpy, but leaving Windows end users like me with a myriad of problems that they should wholeheartedly own as stewards of go is ... more grumpiness. Notice that I was explicitly mentioning Google and not the well-meaning community, but they are limited to playing whack-a-mole and naturally everybody cares about just their own little issues.

As I said, I have no experience with Go. I just counted them and I only ever wrote 54 lines in 2 files and just 20 lines that actually do something.

So, although I might shorten this post and make it less grumpy, making this specific I feel first of all doesn't truly reflect the fact I find issues in about every function in path_windows.go and file_windows.go and probably other *_windows.go files. I mean from palmface issues like the VolumeName API not returning or intending even to return any kind of volume name but a root path to hacking strings and finding ':' where you might also in fact be looking for '\\?\Volume{GUID}' to stripping '\\?\' from a path after 'GetFinalPathNameByHandle' returns that, breaking long paths and special paths in the process, to suspecting that happens in perhaps every file API in go by looking at the issues in this repository, on Stack Overflow and in the docker repository, etc.

I mean how did they get that name so wrong? Was that API proposal not reviewed or did they like the confusion that would result but which they could hope would go unnoticed? Is go a joke?

@networkimprov
Copy link

What Go app has been giving you trouble, Docker? Have you filed an issue there? If so, can you point it out?

It's the app's responsibility to work correctly on platforms it supports. I patch the Go stdlib on Windows where needed.

@ianlancetaylor
Copy link
Contributor

@ericwj I'm sorry that you are frustrated but this issue doesn't seem to give us a useful way forward.

I doubt this will make you feel any better but pretty much all the Windows path support was written and contributed by Windows experts who do not work at Google.

It seems to me that filepath.EvalSymlinks is well defined, and it is clearly useful on Unix systems. Perhaps programs that run on Windows shouldn't be calling it, but that doesn't mean that we should remove it.

CC @alexbrainman @mattn

@ianlancetaylor ianlancetaylor changed the title Consider obsoleting or no-op'ing EvalSymLinks and review all Go path handling on Windows path/filepath: consider obsoleting or no-op'ing EvalSymLinks and review all Go path handling on Windows Jul 7, 2020
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows labels Jul 7, 2020
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jul 7, 2020
@ericwj
Copy link
Author

ericwj commented Jul 7, 2020

Are you saying Google isn't involved? They are at least reviewing every single change - or they appear to be.

It's the app's responsibility to work correctly on platforms it supports.

If you personally wrote a new language, I would agree. Imho Google has a larger responsibility. But hey, the fact the first thing Microsoft does contributing to chromium is to add features for people with disabilities speaks to the company culture at Google refusing to do anything of the sort for 12 whole years.

@ianlancetaylor I am not asking for the API to be removed if it is 'normal' to use that on *nix, although I think overruling an administrator is about always wrong by principle regardless of the operating system. But go knows whether you are on Windows or cross building for it and if it has infrastructure to give warnings or suggest fixes for this API on Windows then that would be step 1.

I didn't personally file issues with Docker, but the are legion links to be found in their issues and on Stack Overflow with people stumbling on error messages like unable to prepare context: unable to evaluate symlinks in context path and similar.

I did file issues with Microsoft about this Visual Studio behavior, but since these errors do not reside in actual Microsoft code they de-prioritized.

About Git I am honestly just unaware of the components that make up the system. Visual Studio Code works quite allright, but Visual Studio has some go somewhere in its Git extension or I don't know where such that these problems magically appear in recent versions of Visual Studio.

@ianlancetaylor
Copy link
Contributor

Are you saying Google isn't involved? They are at least reviewing every single change - or they appear to be.

Google is a huge company. I'm not sure what it means to say that Google is involved. Certainly Google employees are heavily involved with the development of Go. But it's not accurate to say that Google employees are reviewing every single change. Go is an open source project. Many changes to Go are written, reviewed, and submitted by people who are not Google employees, without anybody employed by Google being involved at all.

@networkimprov
Copy link

Well, Eric, since you learned Go, maybe you could fix some of those Docker issues (perhaps by replacing EvalSymlinks in their codebase), and then file an issue or two here about how Go could support them better :-)

But we need specific bug reports to make progress.

@ericwj
Copy link
Author

ericwj commented Jul 8, 2020

Go is an open source project.

So is .NET. And .NET actually works even with Windows-isms on a long list of *nix variants, because Microsoft as primary steward of the ecosystem is taking its proper responsibility. The downside is they have so much tests that it is hard to find any actual code if you look for a definition of something in /dotnet/runtime say.

Look, let's talk solutions. I am fed up hitting brick walls every time someone chooses go over another language and then shoves it onto my system as part of some software that I use - that's why I'm here. I identified very concrete issues and I can probably write code from plain primitives that uses the Windows API properly to fill in go API surface -- I just can't realistically write it in go. Would I help writing plain C# implementing some of the broken API's in go?

I could perhaps even write tests that will show how to break go, but I'd have to do that mostly in PowerShell and C#.

@ericwj
Copy link
Author

ericwj commented Jul 8, 2020

PowerShell to create .vhdx files and mount/unmount them - system administration stuff, which I don't think go can do today at all.

@networkimprov
Copy link

Scripts in Powershell or programs in C demonstrating Go bugs would be fine. C# programs might not be as widely accessible, and you can't necessarily tell by reading them which winapi calls they make.

But it sounds like part of the solution is for certain Go programs not to call EvalSymlinks on Windows.

@ericwj
Copy link
Author

ericwj commented Jul 8, 2020

you can't necessarily tell by reading them which winapi calls they make

You can if I use only primitives - if I exclusively P/Invoke directly. You just get code that is more or less C++ but cleaner.

I actually found this microsoft/DockerTools#235 and why don't I also link this.

@networkimprov
Copy link

Many Go devs won't be set up to compile C#.

@networkimprov
Copy link

BTW you can use this Go package to invoke most of the winapi directly: https://godoc.org/golang.org/x/sys/windows

@ericwj
Copy link
Author

ericwj commented Jul 8, 2020

you can use this Go package

Yes, I've used it in #40095 it must be. But it will take me too much time I do think. To get that single line of error handling proper in the pull took me...I don't remember, an hour?

Many Go devs won't be set up to compile C#.

My idea is to make a repo for this which serves to show how to use the Windows API to say implement IsAbs, which won't be part of the go repo ever but will be ported. All you need to do is read that to port it. Does that fit your expectation?

I might ask you to write a go program which accepts standard input with a switch statement to run public go API with varying arguments and write error codes and maybe messages back to standard output. Can I ask you for such a thing?

@ericwj
Copy link
Author

ericwj commented Jul 8, 2020

A complex issue over which I have no overview whatsoever is how to incorporate tests in this repo and whether it would be acceptible to download and install PowerShell 7 or higher into a local installation folder upon building or running tests on Windows.

If not then it would be needed to port a significant Windows API surface dealing with virtual disks to that x/sys/windows package just to be able to run tests in go.

@ericwj
Copy link
Author

ericwj commented Jul 8, 2020

Then there is the fact I have no idea what the test capabilities are of this repo prior to proper public releases.

Long Paths is a global registry setting, although it can be enabled per process by changing that global setting temporarily.

VirtDisk.dll API's may not be available on all editions of Windows and requires privileges.

Storage Spaces afaik is on all editions of Windows but is certainly different between various Server and non-Server editions.

And these are just the issues from the top of my head.

@ianlancetaylor
Copy link
Contributor

We run tests on various different builders, seen on https://golang.org/build. We're currently doing four different versions of Windows. In principle we can run on any version of Windows that can run in a container.

@networkimprov
Copy link

write a go program which accepts standard input with a switch statement to run public go API with varying arguments

If that's as easy as it sounds, I can do that. I won't be able to port your C# IsAbs() logic, as I haven't signed the Go CLA. However it sounds like anyone else could do that. @bcmills might be a candidate :-)

port a significant Windows API surface dealing with virtual disks to that x/sys/windows package

Right, the VirtualDisk API isn't there. But it's easy to add calls to x/sys/windows.
https://docs.microsoft.com/en-us/previous-versions/windows/desktop/legacy/dd323699(v=vs.85)

You can adjust the configuration of the container that runs tests as needed, I believe.

@ericwj
Copy link
Author

ericwj commented Jul 8, 2020

it's easy to add calls

Luckily I don't need all the API's on that list and yet more luckily I think I already have those that I need in C#, but the snake in the grass is a long list of structs and unions that go with these API's - not sure how many would strictly speaking be absolutely required. Also Setup API is not there afaik. That would be some of these.

we can run on any version of Windows that can run in a container

That means Windows Server exclusively if I'm not mistaken, maybe that's not a problem. Can't see whether these are nano servers or server core though. That could be relevant if there are API's that nanoserver might not have. At least VirtDisk will definitely work on server at least after configuring the container with Hyper-V, no need for enabled virtualization extensions.

If that's as easy as it sounds,

It will probably be for functions that take and return strings and numbers. For path/filepath most arguments are plain strings, so that seems easy enough. Delegates, yeah, WalkFunc. Well, I guess one or several hardcoded very basic ones will suffice. If the errors are actual Windows error numbers or HRESULTs, those are just numbers but trying to match them by name or if go errors are complex objects and I would need to know, that could be a source of difficulty depending on how well go does on getting names of definitions and the reverse operation (reflection).

@ericwj
Copy link
Author

ericwj commented Jul 8, 2020

In principle we can run on any version of Windows that can run in a container.

Does that include local docker on Windows 10 stable latest with Windows Containers? Does anyone of you have experience with that for running go tests?

@networkimprov
Copy link

Go errors stemming from the OS include the system error code, so I can return that with the Go error string.

Some info on the Windows builder environment (not certain it's current): https://github.com/golang/go/wiki/WindowsBuild

@ericwj
Copy link
Author

ericwj commented Jul 8, 2020

I just hacked a driver together - it hasn't even run yet but it gives the idea and it mirrors path/filepath already. You should have some kind of message about that from GitHub.

@networkimprov
Copy link

Found this. Is it a template for the Go app you need, or would it call the Go app?

https://github.com/ericwj/GoWindows/blob/master/src/GoWindows.CSharp/Program.cs

@ericwj
Copy link
Author

ericwj commented Jul 8, 2020

I just wrote that down to see if I can map the API really. This is yeah then a template for go code that I would call.
Would have to change that as you see fit to e.g. include integral error codes if present and to fix the bugs that I already spotted.

Can you add that in go in a subfolder of src? I will probably start with the simplified way of doing it from my perspective that is to make Program.cs accept a few arguments and then call that after hacking the system configuration with PowerShell to create various kinds of paths to test against.

@alexbrainman
Copy link
Member

@ericwj

I don't understand what this issue is about.

I don't see any bug reports. If you found some bug in Go, please, provide steps to reproduce. Or at least some details.

If you are proposing to discontinue use of filepath.EvalSymlinks, then I disagree with your proposal. filepath.EvalSymlinks has been used in many Go programs. Whatever the function does is useful to these programs. If it does not work for your use case, do not use it. Write whatever function you need.

Sorry.

Alex

@ericwj
Copy link
Author

ericwj commented Jul 10, 2020

@alexbrainman I found a number of issues, some of which are mentioned in this thread. Although it is the world upside down, as an end user that has written now 59 lines of go out of frustration with go and is not intending to add to that, I am spending days to get tests setup on a clean Windows machine that will proof a series of issues.

I am being advised to file new issues for those, tbd how exactly.

Sure EvalSymLinks has been used a lot, because go has it and doesn't point at any issues with it. It might also be perfectly valid to always default to evaluating all symlinks on Linux, although I might disagree with that too in the case where these resolved paths might be persisted to disk, as outlined. But all programs using it on Windows today give people like me headaches, until it at least works properly in all system configurations.

I have no use case other than to setup my systems the way I used to when it had no go programs on it and to continue doing so now that go software is being forced on me.

So my frustration with go is about for example Docker using this API, substituting perfectly good paths for unreadable ones for imho no good reason - the Windows API calls that Docker uses should do the translation - and on top of that until 40095 is part of go, substituting perfectly good paths for errors which pretty much completely break Docker depending on where it is configured to run.

Same for Git or whatever components make up the combination of Visual Studio and Git.

@ericwj
Copy link
Author

ericwj commented Jul 10, 2020

Whatever the function does is useful to these programs.

" If a stable result is needed, path/filepath.EvalSymlinks might help." Wrong. The stable path is the path configured by the administrator. The path that was given to EvalSymLinks. The path returned by EvalSymLinks might change due to disaster recovery, moving all or part of a parent directory wholly to another disk e.g. becausee the disk is filling up, due to any responsibility of the system administrator that software should be unaware of unless explicitly directed.

This just took me another ten minutes to clarify, so I will leave it at one example, but I'm sure I can find more.

Also, I just don't want to see resolved paths in the console or pretty much anywhere. Uses like this just make me sad. They completely forget the path that I gave them. It is inappropriate and wrong. I should be in control of the path that they hit.

They try to be smart and do it wrong. And they are very smart people. Imho that is why this is a go issue on Windows and why I consider EvalSymLinks a trap.

@ericwj ericwj changed the title path/filepath: validate all functions on Windows path/filepath: no-op path.filepath.EvalSymLinks on Windows Jul 10, 2020
@ericwj
Copy link
Author

ericwj commented Jul 10, 2020

Let me make this again about where it began before I started reading go code.

@networkimprov
Copy link

See the "Go 1 compatibility" policy. There is zero chance that a stdlib API will be disabled, even if most uses of it are incorrect. Propose a new API in a new issue, or submit documentation fixes re Windows.

@ericwj
Copy link
Author

ericwj commented Jul 10, 2020

Same probably holds for #40158 but it is the low hanging fruit. Well I suppose this one isn't even trying to be looking like hanging at all.

Be perfect reviewing stuff or I guess go would have to rev a bit faster in major version numbers and obsolete some mistakes. Nobody expects anyone to be that perfect.

@ianlancetaylor
Copy link
Contributor

We are far from perfect, but we try as hard we can to not break existing code. We consider stability of the language and libraries to be extremely important.

@ericwj
Copy link
Author

ericwj commented Jul 10, 2020

That is fine. But if you seriously do the review process should match that seriousness in the first place.

@ericwj
Copy link
Author

ericwj commented Jul 10, 2020

Like I hinted at, the common thing to do is rev a bit faster in major versions and obsolete stuff without any functional change in the next major version, then remove it in the major version after that.

I don't know the roadmap but if go 2 would be fairly soon and I would know that go 3 would be at most a year or maximum two after that, that would give me some peace of mind looking at logfiles with horrible paths etc that it is not endless.

And please hurry with #40095 if at all possible because that is not just wrong that is a showstopper.

@networkimprov
Copy link

There is no announced plan for a Go 2.0. The term "Go 2" refers rather unfortunately to new language features in Go 1.x.

You are wasting time grumbling about Go's flaws & policies. You can document some bugs; that will help a lot.

@ianlancetaylor
Copy link
Contributor

@ericwj I'm sorry you find our approach to be annoying. As @networkimprov says, there is no Go 2, and there is no Go 3. There may be a path/filepath/v2 some day, but we aren't in a hurry about it. Our general long-term transition plans are outlined at https://go.googlesource.com/proposal/+/refs/heads/master/design/28221-go2-transitions.md .

@mattn
Copy link
Member

mattn commented Jul 11, 2020

Please provide a small repro-steps to fix the problem (if this is clearly problem). We have not yet shared the information. Thanks.

@ericwj
Copy link
Author

ericwj commented Jul 11, 2020

Ah that's why I come across grumpy.

Just to clarify, I just talk from basic assumptions, not to criticize whatever policies go has adopted for whatever reasons or with which languages they compare themselves and which ones not.

I am still an end user. I don't care about go 1, how would I care about go 2? How can I criticize what I haven't even tried to learn the first thing about? I just assumed that part of designing a language and a large API surface is to device some means or another to fix issues with it that will inevitably build over time and that that would involve revving at some point.

So sorry for assuming that.

It just makes that review process more important. And my opinion of it travels with the inverse.

@networkimprov
Copy link

@ianlancetaylor @mattn I think Eric is working on a CL that will demonstrate some filepath bugs. He posted a note in golang-dev requesting advice on assembling it, but hasn't received any responses.

@alexbrainman
Copy link
Member

@alexbrainman I found a number of issues, some of which are mentioned in this thread.

Sorry, but I cannot find them. Perhaps this thread is too long. Why don't you create new issue for every bug you found? Please, do not ignore the issue template, but fill in as much details as you can.

Please, be aware that Go issue tracker is only used to track real bugs / problems, and submit change proposals.

Thank you.

Alex

@ericwj
Copy link
Author

ericwj commented Jul 11, 2020

Yes just leave this one to be about no-op ing EvalSymLinks and nothing else.

Even the way to do that without breaking anyone is a separate discussion about alternatives and what they look like.

Perhaps this issue could be about changing documentation, or at least adding a few lines of text to make people aware of why this API could bite them on Windows and about expectations by Windows foaks that they shall not default to using it.

@networkimprov
Copy link

You can just submit a CL with documentation fix clarifying what happens for certain paths.

BTW English folk is spelled like the German volk :-)

@ericwj
Copy link
Author

ericwj commented Jul 11, 2020

@ericwj
Copy link
Author

ericwj commented Jul 12, 2020

I think messed this up. This patch was supposed to be just against ericwj/go for a new pull request. Did it get added to #40095? How do I fix that?

@networkimprov
Copy link

It was added to the CL, yes. Just revert. Then make a new branch, and cherrypick the docs commits to that, and file a new PR.

Also, it should describe EvalSymlinks problems on Windows, not make blanket statements.

@ericwj
Copy link
Author

ericwj commented Jul 12, 2020

That is explained in the commit message, but I'm not sure that is visible. Just revert? How? Commit after getting the old file from history?

@networkimprov
Copy link

git revert <commit> is a common op; it creates new commits. Alternatively make a new commit by hand that removes the docs change.

@networkimprov
Copy link

Best to close this issue, and make a new one for the docs change. Should post the new docs there for discussion before filing PR/CL.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
None yet
Development

No branches or pull requests

6 participants