Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

TestResolveProjectRoot fails on Windows when not run with administrator privileges. #494

Closed
ChrisHines opened this issue May 1, 2017 · 14 comments
Labels

Comments

@ChrisHines
Copy link
Contributor

By default only administrator accounts have privileges to create symbolic links on Windows. The test code in TestResolveProjectRoot calls os.Symlink. Since very few people run anything in an admin shell on Windows, the symlinks will not be created when run in a standard Windows environment.

The test code does not check the error return from os.Symlink, so the result is the test assertions that follow fail.

My understanding is that symlinks in GOPATH are not supported by Go tooling (see golang/go#15507).

What is dep's policy regarding symlinks? On Windows?

@ChrisHines
Copy link
Contributor Author

By default only administrator accounts have privileges to create symbolic links on Windows.

Correction: I meant to say that by default symlinks can only be created when run with administrator privileges. The equivalent on Linux would be requiring ln -s to be run with sudo.

@sdboyer
Copy link
Member

sdboyer commented May 1, 2017

By default only administrator accounts have privileges to create symbolic links on Windows.

...huh. Learn something new every day 😄

/cc @spenczar, who may be interested

The test code does not check the error return from os.Symlink, so the result is the test assertions that follow fail.

Huh...I guess, then, that appveyor must be running in an admin shell?

Either way, seems like disabling that test on windows is wise. And also, checking those err returns.

My understanding is that symlinks in GOPATH are not supported by Go tooling (see golang/go#15507).

After spending more time than is healthy trying to work on and/or guide people through symlinks over the past few months, I'm certainly inclined to agree with Rob that they're just a bad, painful idea.

The only real support we have for them right now is very narrowly targeted at the problem of figuring out the project root. The working spec for it is here: #247 (comment). My impression is that, by only utilizing symlinks at this initial "entry point", we're creating an environment that is ultimately consistent with how the go tool operates.

The other bit of support was introduced in sdboyer/gps#198, specifically as a guard against the toolchain apparently following symlinks into a vendor/ dir. I guess. My unscientific feeling is that the toolchain's support for symlinks is of the "undefined, but often works" variety, as such gymnastic seem to be necessary

What is dep's policy regarding symlinks? On Windows?

Our goal is to mirror the toolchain's behavior as much as possible. I let #247 through because it seemed like a case where we could keep the scope contained, it appeared to be interoperable with observed toolchain behavior, and the obvious benefit to UX.

But none of that is worth going out of alignment with the toolchain. In the grand game of absorbing dep into the toolchain, it's an unforced error to voluntarily move out of alignment on those few topics where the toolchain has already laid down rules. So, if the existing symlink interactions do create such a misalignment, we need to modify or remove them.

@ChrisHines
Copy link
Contributor Author

ChrisHines commented May 1, 2017

The good news with regard to symlinks on Windows is that they are rarely used.

I suggest that we add the error checking on the os.Symlink calls. In the case of an error the test would behave as follows.

  • For non-Windows platforms call t.Fatal.
  • For Windows platforms call t.Skip.

I think we should also wrap each test case with t.Run to make the Fatal or Skip calls fine-grained.

@sdboyer
Copy link
Member

sdboyer commented May 1, 2017

@ChrisHines all of this SGTM 😄

@ChrisHines
Copy link
Contributor Author

OK. I'll work on a PR.

@sdboyer
Copy link
Member

sdboyer commented May 2, 2017

Feels like we should make an FAQ item out of this.

@sdboyer sdboyer added the docs label May 2, 2017
@ChrisHines
Copy link
Contributor Author

What part of this do you think should be in a FAQ?

I'd be amazed if you can find many Windows developers that have ever used symbolic links by choice. I would expect many of them don't even know they exist (on Windows). In my anecdotal experience they have essentially zero mind-share on that platform.

@sdboyer
Copy link
Member

sdboyer commented May 2, 2017

Not the windows bit specifically - rather, the more general question about "What is dep's policy regarding symlinks?"

@ChrisHines
Copy link
Contributor Author

Agreed, that makes sense.

@sdboyer
Copy link
Member

sdboyer commented May 2, 2017

@brianstarke any chance I could coerce you into writing an FAQ entry for the symlink stuff you worked on? :makes bambi eyes:

@brianstarke
Copy link
Contributor

Sorry for the delay, I'd be happy to take a swing at it but I must warn you that my command of the english language isn't very water buffalo.

@sdboyer
Copy link
Member

sdboyer commented May 9, 2017

sounds gross to myself!

@brianstarke
Copy link
Contributor

brianstarke commented May 18, 2017

@sdboyer, #605

@sdboyer
Copy link
Member

sdboyer commented May 24, 2017

Awesome, thanks! That's merged, so closing this 😄

@sdboyer sdboyer closed this as completed May 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants