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.

Sign up for GitHub

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

Jump to bottom

Use libuv for tempdir function #31434

Merged
merged 2 commits into from
Jun 26, 2019
Merged

Use libuv for tempdir function #31434

merged 2 commits into from
Jun 26, 2019

Conversation

musm
Copy link
Contributor

@musm musm commented Mar 21, 2019 •
edited
Loading

Use libuv's implementation for tempdir for both linux and windows. This fixes the inconsistency between the windows and linux version, where the windows version does not currently include a trailing slash when calling tempdir compared to the linux implementation.

@musm
Copy link
Contributor Author

musm commented May 9, 2019

Is this something we care about?

@StefanKarpinski StefanKarpinski added this to the 1.3 milestone May 9, 2019
@StefanKarpinski
Copy link
Sponsor Member

Yes, but too late for 1.2.

@musm musm closed this Jun 4, 2019
@musm musm reopened this Jun 4, 2019
stevengj
stevengj reviewed Jun 5, 2019
View reviewed changes
base/file.jl Outdated Show resolved Hide resolved
@musm
Copy link
Contributor Author

musm commented Jun 5, 2019

came up here https://discourse.julialang.org/t/tempdir-differences-in-windows-and-linux/24930

musm
musm commented Jun 5, 2019
View reviewed changes
base/file.jl Outdated Show resolved Hide resolved
vtjnash
vtjnash reviewed Jun 6, 2019
View reviewed changes
base/file.jl Outdated Show resolved Hide resolved
vtjnash
vtjnash reviewed Jun 6, 2019
View reviewed changes
test/file.jl Outdated Show resolved Hide resolved
stevengj
stevengj reviewed Jun 6, 2019
View reviewed changes
base/file.jl Outdated Show resolved Hide resolved
musm
musm commented Jun 10, 2019
View reviewed changes
base/file.jl Outdated Show resolved Hide resolved
stevengj
stevengj reviewed Jun 10, 2019
View reviewed changes
base/file.jl Outdated Show resolved Hide resolved
stevengj
stevengj reviewed Jun 10, 2019
View reviewed changes
base/file.jl Outdated Show resolved Hide resolved
@musm
Copy link
Contributor Author

musm commented Jun 10, 2019

@stevengj I addressed your feedback, let me know if these changes now have your approval. Thanks for the review.

I also think GetTempPathW could probably just be implemented in Julia, without too much difficulty. Probably best for another PR, however.

The GetTempPath function checks for the existence of environment variables in the following order and uses the first path found:

The path specified by the TMP environment variable.
The path specified by the TEMP environment variable.
The path specified by the USERPROFILE environment variable.
The Windows directory.
Note that the function does not verify that the path exists, nor does it test to see if the current process has any kind of access rights to the path. The GetTempPath function returns the properly formatted string that specifies the fully qualified path based on the environment variable search order as previously specified. The application should verify the existence of the path and adequate access rights to the path prior to any use for file I/O operations.
Symbolic link behavior—If the path points to a symbolic link, the temp path name maintains any symbolic links.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jun 10, 2019

If we want to go that route, we could switch to just using uv_os_tmpdir everywhere (it already handle this case).

@musm
Copy link
Contributor Author

musm commented Jun 10, 2019

Any reason not to go the libuv route? Seems simpler, then custom casing Windows.

@musm
Copy link
Contributor Author

musm commented Jun 11, 2019 •
edited
Loading

I was testing uv_os_tmpdir

it seems to work fine EXCEPT when ENV["TMP"]= "" , in which case garbage is spit out. GetTempPathW would spit out the correct value. I think there might be a bug in uv_os_tmpdir ?
It seems like in this case the sz from LIBUV spits out is incorrect?

NVM: GetTempPathW returns the null '\0 in this pathological case. So it should be OK. Although, I'm not sure if anything should be done regarding this special case. When ENV["TMP"]= "" and calling uv_os_tmpdir it sets sz[] = 6 .

stevengj
stevengj reviewed Jun 11, 2019
View reviewed changes
base/file.jl Outdated Show resolved Hide resolved
stevengj
stevengj reviewed Jun 11, 2019
View reviewed changes
base/file.jl Outdated Show resolved Hide resolved
@musm
Copy link
Contributor Author

musm commented Jun 11, 2019 •
edited
Loading

According to 84e68f3 by @Keno it seems like we should be using RefValue instead of Ref here? (there are two instances where Ref is used)

stevengj
stevengj reviewed Jun 11, 2019
View reviewed changes
base/file.jl Outdated Show resolved Hide resolved
@musm
Copy link
Contributor Author

musm commented Jun 11, 2019

Ok I updated the code and this is now ready for a final review. Last commit handles the UV_ENOBUFS case with tests. I think this is now good to merge.

stevengj
stevengj reviewed Jun 12, 2019
View reviewed changes
base/file.jl Outdated Show resolved Hide resolved
stevengj
stevengj reviewed Jun 12, 2019
View reviewed changes
base/file.jl Outdated Show resolved Hide resolved
@stevengj stevengj mentioned this pull request Jun 12, 2019
Merged
@musm musm changed the title Strip trailing slash on windows to match linux Use libuv for tempdir function Jun 12, 2019
@musm musm force-pushed the win branch 2 times, most recently from 8b354e2 to 8af140f Compare June 14, 2019 21:33
musm
musm commented Jun 15, 2019
View reviewed changes
test/file.jl Outdated

var = Sys.iswindows() ? "TMP" : "TMPDIR"
PATH_PREFIX = Sys.iswindows() ? "C:\\" : "/tmp/"
MAX_PATH = (Sys.iswindows() ? 251 : 1024) - length(PATH_PREFIX) - 1 # null-termination character
Copy link
Contributor Author

@musm musm Jun 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

For reference I'm pretty sure there's a bug in the libuv function that hits an edge case that shouldn't happen, which is why the test here is for 251 chars and does not test longer paths.

The edge case occurs on windows when the TMP variable has 260 chars

hitting this branch I believe https://github.com/libuv/libuv/blob/c4e9657d59723a6273ce68bda935680e1b3596c5/src/win/util.c#L1186

@musm
Copy link
Contributor Author

musm commented Jun 19, 2019

Before merging let me push an update to account for the latest changes on master

@musm
Copy link
Contributor Author

musm commented Jun 21, 2019

good to merge?

@musm
Use libuv API for tempdir function
5914920 
Fixes the Windows tempdir function returning a path with the trailing slash.
@musm
Copy link
Contributor Author

musm commented Jun 24, 2019

Test failures are unrelated, due to downloads

@musm
Copy link
Contributor Author

musm commented Jun 25, 2019

@vtjnash can you sign off if this looks good to you?

FYI the @test_broken is fixed by my PR to libuv

vtjnash
vtjnash approved these changes Jun 25, 2019
View reviewed changes
base/file.jl
@@ -481,9 +497,6 @@ function tempname()
return s
end
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Change to use tempdir() for d?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will make these changes in a separate PR.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

sgtm. feel free to merge.

base/file.jl
Obtain the path of a temporary directory (possibly shared with other processes).
"""
tempdir()

"""
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

In the tempname doc string below, it might be clearer to add, "This can lead to security holes." and recommend opening the file with O_EXCL, if this is a concern (in addition to the suggestion to use mktemp).

Aside: the windows implementation of mktemp appears to be wrong because it fails to heed this warning.

@musm
Copy link
Contributor Author

musm commented Jun 26, 2019

sgtm. feel free to merge.

Someone with merge powers want to take care of this 😉

@ararslan ararslan merged commit 2b08860 into JuliaLang:master Jun 26, 2019
@musm musm deleted the win branch June 26, 2019 16:15
@fredrikekre fredrikekre mentioned this pull request Aug 5, 2019
Closed
@fredrikekre fredrikekre mentioned this pull request Sep 25, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash approved these changes

@stevengj stevengj stevengj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.
5 participants
@musm @StefanKarpinski @vtjnash @stevengj @ararslan