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

fix #3023 make expandSymlink work on Windows #17289

Closed

ringabout wants to merge 10 commits into nim-lang:devel from ringabout:a140

Member

ringabout commented Mar 7, 2021 •

edited

Loading

followup

expandFileName should use GetFinalPathNameByHandleW too.

ringabout added 2 commits

March 7, 2021 22:45


          fix nim-lang#3023

0c06a19


          default

a6d82cc

ringabout added the TODO: followup needed label

ringabout marked this pull request as draft

March 7, 2021 14:53

konsumlamm reviewed

View reviewed changes

lib/pure/os.nim Outdated Show resolved Hide resolved

Member

timotheecour commented Mar 7, 2021 •

edited

Loading

would fix bug2 from improve windows support for symlinks #16784

expandFileName should use GetFinalPathNameByHandleW too.

see bug8 from #16784:

bug8: expandFilename doesn't seems to resolve symlinks (unlike on posix); GetFinalPathNameByHandleA seems like what should be used (but should be called through expandSymlink)

ie, it should instead call expandSymlink once it's fixed

timotheecour mentioned this pull request

improve windows support for symlinks #16784

Open

9 tasks

timotheecour reviewed

View reviewed changes

lib/pure/os.nim Show resolved Hide resolved

timotheecour reviewed

View reviewed changes

lib/pure/os.nim Outdated Show resolved Hide resolved

timotheecour reviewed

View reviewed changes

lib/pure/os.nim Outdated Show resolved Hide resolved

fix

c806527

ringabout commented

View reviewed changes

lib/pure/os.nim Outdated Show resolved Hide resolved


          Update lib/pure/os.nim

adb0f50

ringabout marked this pull request as ready for review

March 12, 2021 15:34

ringabout commented

View reviewed changes

lib/pure/os.nim Outdated Show resolved Hide resolved


          Update lib/pure/os.nim

ad5564d

ringabout commented

View reviewed changes

lib/pure/os.nim Outdated Show resolved Hide resolved


          Update lib/pure/os.nim

4102af5

juancarlospaco reviewed

View reviewed changes

lib/pure/os.nim Outdated Show resolved Hide resolved

juancarlospaco reviewed

View reviewed changes

lib/pure/os.nim Outdated Show resolved Hide resolved

ringabout marked this pull request as draft

March 14, 2021 05:06


          add testcase

1deba99

ringabout changed the title ~~fix #3023~~ fix #3023 make expandSymlink() work on Windows

ringabout changed the title ~~fix #3023 make expandSymlink() work on Windows~~ fix #3023 make expandSymlink work on Windows

ringabout marked this pull request as ready for review

March 14, 2021 08:35

ringabout marked this pull request as draft

March 14, 2021 08:36

ringabout added 2 commits

March 14, 2021 16:47

try

fd494e2

fix

b7ec44c

ringabout marked this pull request as ready for review

March 14, 2021 15:54

ringabout closed this

ringabout reopened this

timotheecour reviewed

View reviewed changes

tests/stdlib/tos.nim

Comment on lines +272 to +273

		let f = open(symlinkSrc, fmWrite)
		f.close()

Member

timotheecour Mar 20, 2021 •

edited

Loading

I don't understand this; you're creating a file "D20210101T1920_nonexistant" (nonexistant should indicate it doesn't exist)
generated files should go under buildDir (which is gitignored), but symlinkSrc is not

simply use:

const symlinkFile = buildDir/"D20210101T191320_BROKEN_SYMLINK"
const symlinkSrc = "nonexistant"
createSymlink(symlinkSrc, symlinkFile)
doAssert expandSymlink(symlinkFile) == symlinkSrc

timotheecour reviewed

View reviewed changes

lib/pure/os.nim Show resolved Hide resolved

timotheecour reviewed

View reviewed changes

lib/pure/os.nim

Comment on lines +1733 to +1734

		if not symlinkExists(symlinkPath):
		raise newException(OSError, symlinkPath & " is not symlink or does not exist")

Member

timotheecour Mar 20, 2021

Suggested change

      
                if not symlinkExists(symlinkPath):
          
                  raise newException(OSError, symlinkPath & " is not symlink or does not exist")

because it's redundant with the check a few lines below:

if handle == INVALID_HANDLE_VALUE:
  raiseOSError(osLastError(), symlinkPath)

unless i'm missing something


          Update lib/pure/os.nim

e478789

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>

ringabout closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

TODO: followup needed