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

os: TestMkdirAll failing Plan 9 #23918

Closed
0intro opened this issue Feb 19, 2018 · 13 comments
Closed

os: TestMkdirAll failing Plan 9 #23918

0intro opened this issue Feb 19, 2018 · 13 comments
Assignees
Milestone

Comments

@0intro
Copy link
Member

@0intro 0intro commented Feb 19, 2018

Since CL 86295, TestMkdirAll is failing on Plan 9.

--- FAIL: TestMkdirAll (0.01s)
	path_test.go:67: MkdirAll "/tmp/_TestMkdirAll_/dir/./dir2/file/subdir" returned wrong error path: "/tmp/_TestMkdirAll_/dir/dir2/file/subdir" not "/tmp/_TestMkdirAll_/dir/dir2/file"
FAIL
FAIL	os	1.734s

See https://build.golang.org/log/b073356bfd34ac9b38f472e8ba905e166cd383c6

@0intro 0intro added the OS-Plan9 label Feb 19, 2018
@0intro 0intro added this to the Go1.11 milestone Feb 19, 2018
@0intro 0intro self-assigned this Feb 19, 2018
@0intro

This comment has been minimized.

Copy link
Member Author

@0intro 0intro commented Feb 19, 2018

Before the change:

% go test -v -run  '^TestMkdirAll$'
=== RUN   TestMkdirAll
/tmp is a directory
Mkdir /tmp/_TestMkdirAll_
Mkdir /tmp/_TestMkdirAll_/dir
Mkdir /tmp/_TestMkdirAll_/dir/.
Mkdir /tmp/_TestMkdirAll_/dir/./dir2
/tmp/_TestMkdirAll_/dir/./dir2 is a directory
/tmp/_TestMkdirAll_/dir/./dir2/file is a file
/tmp/_TestMkdirAll_/dir/./dir2/file is a file
--- PASS: TestMkdirAll (0.01s)
PASS
ok  	os	0.052s

After the change:

% go test -v -run  '^TestMkdirAll$'
=== RUN   TestMkdirAll
/tmp/ is a directory
Mkdir /tmp/_TestMkdirAll_/
Mkdir /tmp/_TestMkdirAll_/dir/
Mkdir /tmp/_TestMkdirAll_/dir/./
Mkdir /tmp/_TestMkdirAll_/dir/./dir2
/tmp/_TestMkdirAll_/dir/./dir2 is a directory
/tmp/_TestMkdirAll_/dir/./dir2/file is a file
/tmp/_TestMkdirAll_/dir/./dir2/ is a directory
Mkdir /tmp/_TestMkdirAll_/dir/./dir2/file/
Mkdir /tmp/_TestMkdirAll_/dir/./dir2/file/subdir
--- FAIL: TestMkdirAll (0.02s)
	path_test.go:67: MkdirAll "/tmp/_TestMkdirAll_/dir/./dir2/file/subdir" returned wrong error path: "/tmp/_TestMkdirAll_/dir/dir2/file/subdir" not "/tmp/_TestMkdirAll_/dir/dir2/file"
FAIL
exit status: 'os.test 374140: 1'
FAIL	os	0.057s

This issues emphases a specific behavior on Plan 9. It can be reproduced this way:

% mkdir -p /tmp/_TestMkdirAll_/dir/./dir2
% > /tmp/_TestMkdirAll_/dir/./dir2/file
% mkdir /tmp/_TestMkdirAll_/dir/./dir2/file/
% mkdir /tmp/_TestMkdirAll_/dir/./dir2/file
mkdir: /tmp/_TestMkdirAll_/dir/./dir2/file already exists

Mkdir("file") will return an error, while Mkdir("file/") won't.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Feb 19, 2018

@0intro I filled #23902 yesterday about this. I have no idea how to fix this on plan9. Do you want me to revert CL 86295 ? Or should we adjust CL 86295 to do nothing on plan9?

Alex

@0intro

This comment has been minimized.

Copy link
Member Author

@0intro 0intro commented Feb 19, 2018

Sorry, it seems I've missed your issue.

I'm not sure how to fix this issue properly on Plan 9 yet. Should we change the behavior of the fstat syscall in the Plan 9 kernel? Should we remove trailing slashes in the implementation of os.Stat or dirstat on Plan 9?

I think the safest change for now would be to add an exception for Plan 9 in the implementation of os.MkdirAll, so we get the same behavior as before. I'll propose a CL.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Feb 19, 2018

Sorry, it seems I've missed your issue.

No worries. Sorry for breaking plan9 build.

I think the safest change for now would be to add an exception for Plan 9 in the implementation of os.MkdirAll, so we get the same behavior as before.

I was going to suggest the same,

I'll propose a CL.

Thank you. Feel free to put me as a reviewer. I will push +2 button if I am not in bed.

Alex

@millerresearch

This comment has been minimized.

Copy link

@millerresearch millerresearch commented Feb 19, 2018

MkdirAll is now being called with a trailing slash on the path. In the case where the path exists and is a file, the "fast path" code will now be bypassed -- on all platforms, not just Plan 9 -- because the Stat in line 22 returns error "not a directory", and therefore the dir.IsDir call will never be executed.

The Plan 9 misbehaviour is actually independent of the trailing slash -- the underlying system call

create(path, OREAD, DMDIR | mode)

doesn't return an error if path exists and is a file, whether or not it ends in a slash.

In the altered MkdirAll, it might be a good idea to check the error from the first Stat with IsNotexist, to reinstate the fast path in the case of a file.

But I think we should workaround the underlying Plan 9 problem by having syscall.Mkdir check for pre-existing path, and not relying on the system call to do it right.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 19, 2018

Change https://golang.org/cl/95055 mentions this issue: os: fix MkdirAll on Plan 9

@0intro

This comment has been minimized.

Copy link
Member Author

@0intro 0intro commented Feb 19, 2018

@millerresearch Yes, you're right. Could you submit a CL?

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Feb 19, 2018

We could also just revert CL 86295 and I will try to find different fix for CL CL 86295.
What do you think?

Alex

@millerresearch

This comment has been minimized.

Copy link

@millerresearch millerresearch commented Feb 19, 2018

@0intro, I think the change should be runtime.GOOS != "windows" instead of runtime.GOOS == "plan9", because the stat behaviour is common to other platforms: when the the path ends with a slash but the named file is not a directory, stat returns a "not a directory" error instead of just reporting the file exists.

Independent of this correction, I think I have a workaround for the silly Plan 9 behaviour of syscall.Mkdir, so I'll submit a CL for that.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 19, 2018

Change https://golang.org/cl/94777 mentions this issue: syscall: ensure Mkdir(path) on Plan 9 fails if path exists

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 19, 2018

Change https://golang.org/cl/95095 mentions this issue: os: fix fast path in MkdirAll

@0intro

This comment has been minimized.

Copy link
Member Author

@0intro 0intro commented Feb 19, 2018

I've replaced CL 95055 by CL 95095 with a more general subject and description.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 19, 2018

Change https://golang.org/cl/95255 mentions this issue: path: use OS-specific function in MkdirAll, don't always keep trailing slash

@gopherbot gopherbot closed this in a156fc0 Feb 20, 2018
gopherbot pushed a commit that referenced this issue Feb 23, 2018
…g slash

CL 86295 changed MkdirAll to always pass a trailing path separator to
support extended-length paths on Windows.

However, when Stat is called on an existing file followed by trailing
slash, it will return a "not a directory" error, skipping the fast
path at the beginning of MkdirAll.

This change fixes MkdirAll to only pass the trailing path separator
where required on Windows, by using an OS-specific function fixRootDirectory.

Updates #23918

Change-Id: I23f84a20e65ccce556efa743d026d352b4812c34
Reviewed-on: https://go-review.googlesource.com/95255
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David du Colombier <0intro@gmail.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
@golang golang locked and limited conversation to collaborators Feb 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.