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

testscript: ensure that temp dir isn't a symlink #79

Merged
merged 1 commit into from
Sep 4, 2019
Merged

testscript: ensure that temp dir isn't a symlink #79

merged 1 commit into from
Sep 4, 2019

Conversation

leitzler
Copy link
Contributor

@leitzler leitzler commented Sep 2, 2019

I ran into issues in govim with the testscript matcher since it uses real paths in the logs. When running macOS ioutil.TempDir() returns a sym-linked directory by default.

This PR ensures that the path used is a real directory by evaluating the root test temp dir that acts as base for $WORK.

See full description in govim/govim#308

@myitcv
Copy link
Collaborator

myitcv commented Sep 2, 2019

This LGTM but we'll wait for @rogpeppe to opine. We can't test this adequately because the macOS Travis environment is not recent enough.

@mvdan
Copy link
Collaborator

mvdan commented Sep 2, 2019

When running macOS ioutil.TempDir() returns a sym-linked directory by default.

If that's the case, add a test to prove what this is fixing? The project is already tested on Mac via Travis.

@myitcv
Copy link
Collaborator

myitcv commented Sep 2, 2019

If that's the case, add a test to prove what this is fixing? The project is already tested on Mac via Travis.

I think (but @leitzler will be able to confirm) this is only surfaced on Mojave.

@leitzler
Copy link
Contributor Author

leitzler commented Sep 2, 2019

I've only tested on Mojave (10.14.6) but I don't think the current tests would catch it anyway since it's a bit of an edge case.

go-internal as well as the original go source always handles the symlink and therefor won't be affected, but as govim is acting from within vim as context where it resolves the actual path it becomes a problem.

This would end up with different paths on macOS:

package main

import (
	"fmt"
	"io/ioutil"
	"os"
)

func main() {
	tmpd, err := ioutil.TempDir("", "foo")
	if err != nil {
		panic(err)
	}
	fmt.Println(tmpd)

	os.Chdir(tmpd)
	cwd, err := os.Getwd()
	if err != nil {
		panic(err)
	}
	fmt.Println(cwd)
}

e.g.:

/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/foo872692457
/private/var/folders/xm/ls208xd174v95pgd20rn_qbh0000gn/T/foo872692457

I'll see if I can create a simple testscript that details it further.

@leitzler
Copy link
Contributor Author

leitzler commented Sep 2, 2019

Here is an example repo: https://github.com/leitzler/testscript-symlink

It's a bit hard to create a fair example since govim has a custom testdriver with custom commands to compare logs, but I hope this makes more sense.

Copy link
Owner

@rogpeppe rogpeppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one suggestion, thanks!

testscript/testscript.go Show resolved Hide resolved
@myitcv
Copy link
Collaborator

myitcv commented Sep 2, 2019

Comment looks good to me @leitzler - I think we should include the test, no?

https://github.com/leitzler/testscript-symlink/blob/master/scripts/foo.txt

@leitzler
Copy link
Contributor Author

leitzler commented Sep 2, 2019

Good idea, I added it.

@myitcv
Copy link
Collaborator

myitcv commented Sep 3, 2019

Thanks @leitzler

Please can you squash the commits down and provide a descriptive commit message?

As you can see from https://github.com/rogpeppe/go-internal/commits/master we do not use merge commits, instead relying on the commit message in the PR. When there are multiple commits in a PR (like now) GitHub simply does a concatenation of the messages... which leaves us with something less useful :)

In cases where ioutil.TempDir() returns a symbolic link (default
behaviour in macOS) the workdir substitution could mess up
output matching for programs that uses the real dir.

This PR introduces sym-link evaluation of the test temp dir to
ensure consistency.
@myitcv myitcv merged commit 9181448 into rogpeppe:master Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants