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

Move realpath() earlier in cmdlineargs test #34506

Merged
merged 3 commits into from
Jan 27, 2020

Conversation

staticfloat
Copy link
Member

This should hopefully fix issues such as #34418 (comment)

@staticfloat staticfloat force-pushed the sf/fix_win_case_sensitivity_woes branch from 6773f18 to ccd1548 Compare January 24, 2020 21:44
@staticfloat staticfloat force-pushed the sf/fix_win_case_sensitivity_woes branch from ccd1548 to 394eca4 Compare January 24, 2020 22:48
@@ -600,6 +600,7 @@ end
end

mktempdir() do dir
dir = realpath(dir)
Copy link
Member

Choose a reason for hiding this comment

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

If this is needed on Windows, shouldn’t it be part of the mktempdir implementation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think probably yes.

@staticfloat staticfloat force-pushed the sf/fix_win_case_sensitivity_woes branch from e5e30b0 to a0d7eb7 Compare January 25, 2020 03:20
@stevengj
Copy link
Member

stevengj commented Jan 25, 2020

I'm still confused about the real source of the bug here. Why do we need the realpath result?

Currently, mktempdir(), mktemp(), and tempname() all use tempdir() as their default parent directory. The tempdir() function gets its path from a user environment variable (TMP etc.), and hence it may be a path that does not yet exist or a path spelled with the "wrong" case (on Windows or Mac).

I just checked, and none of mktempdir(), mktemp(), or tempname() currently return the realpath result on Windows — all of them preserve the case that was passed (and on my system return the "short" 8.3 path since tempdir() is a short path). Similarly, on MacOS (also case-insensitive, case-preserving), if I set ENV["TMPDIR"]=uppercase(tempdir()) then all of these functions currently return the uppercase path.

I see three options:

  1. Continue to do what we do now — preserve the user environment's specified path as-is in all returned paths. Any caller that needs the realpath (such as these tests, apparently) should call this manually.

  2. Change tempdir() to return the realpath() if the path exists. This seems potentially dangerous, however — if they current path is a symbolic link, but the symbolic link is changed to a different target after the tempdir() call

  3. Keep tempdir() as-is, but change mktemp etcetera to return the realpath, which should be safer once the file is created. (If someone renames the file or rearranges symbolic links after the mktemp call, we already have potential problems when you try to delete the file.)

@stevengj stevengj added filesystem Underlying file system and functions that use it test This change adds or pertains to unit tests labels Jan 25, 2020
@KristofferC KristofferC added the priority This should be addressed urgently label Jan 27, 2020
@KristofferC
Copy link
Member

Bump, all Windows CI currently fails so it would be good to do something here.

@stevengj
Copy link
Member

I think it's fine to merge this for now and debate whether we want to call realpath by default for mktemp etc. later.

@vtjnash vtjnash merged commit a5c422f into master Jan 27, 2020
@vtjnash vtjnash deleted the sf/fix_win_case_sensitivity_woes branch January 27, 2020 19:24
@vtjnash
Copy link
Member

vtjnash commented Jan 27, 2020

These tests are probably demonstrating bad practice and should be using samefile instead of ==, but they're only tests.

@staticfloat
Copy link
Member Author

I think Jameson is correct; I think the reason the realpath() is needed has more to do with the tests trying to ensure that two things are the same file than that mktempdir() is doing something fundamentally wrong. That being said, it does seem reasonable to me that if a standard library function returns you a pathname to a file, it would be the most "canonical" way to refer to that file, which realpath() seems to be to me.

staticfloat referenced this pull request Jan 28, 2020
Don't include one-time costs (JIT compilation) so that warm-up isn't generally required.
And adjust codegen emission to charge call entry costs to the caller.

fixes #11753
fixes #19981
fixes #21871
fixes #34054
close #18595
staticfloat added a commit that referenced this pull request Jan 29, 2020
These should probably be using `samefile`, if they were real code instead of just tests. Though it's unclear why real code would be doing this. Maybe just don't put your paths in hash-tables and you'll normally be fine.

(cherry picked from commit a5c422f)
KristofferC pushed a commit that referenced this pull request Jan 30, 2020
These should probably be using `samefile`, if they were real code instead of just tests. Though it's unclear why real code would be doing this. Maybe just don't put your paths in hash-tables and you'll normally be fine.

(cherry picked from commit a5c422f)
@KristofferC KristofferC mentioned this pull request Jan 30, 2020
26 tasks
@KristofferC KristofferC mentioned this pull request Feb 23, 2020
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
These should probably be using `samefile`, if they were real code instead of just tests. Though it's unclear why real code would be doing this. Maybe just don't put your paths in hash-tables and you'll normally be fine.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filesystem Underlying file system and functions that use it priority This should be addressed urgently test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants