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

New-Item Type SymbolicLink on Unix-like platforms: converts relative target paths to absolute ones, doesn't normalize path separators on macOS #13064

Closed
mklement0 opened this issue Jun 30, 2020 · 7 comments
Labels
Issue-Bug Issue has been identified as a bug in the product Up-for-Grabs Up-for-grabs issues are not high priorities, and may be opportunities for external contributors WG-Cmdlets-Core cmdlets in the Microsoft.PowerShell.Core module

Comments

@mklement0
Copy link
Contributor

PR #12797 tried to implement support for relative target paths for symbolic links (symlinks).

On Windows, this now works properly as of PowerShell Core 7.1.0-preview.4, including normalizing path separators to \.

On Unix-like platforms, a relative input path is still unexpectedly converted to a full path and stored as such in the symlink.

Curiously that doesn't happen when you use \ as a separator on macOS - which must be normalized to / on Unix-like platforms - but the normalization doesn't happen and the \-based relative path is stored as-is, which results in a broken symlink; on Linux, the unexpected conversion to an absolute path (with appropriate separators) takes place.

Steps to reproduce

# OK on Windows, FAILS on macOS and Linux.
Describe "New-Item symlinks" {
  BeforeAll {
    Push-Location testdrive:

    $targets = "./$PID.tmp", ".\$PID.tmp"
    $lnk = "$PID-L.tmp"

    'foo' > $targets[0]

  }

  It "Can create symlink with relative target <target>" -TestCases @{ target = $targets[0] }, @{ target = $targets[1] } {
    param([string] $target)

    $null = New-Item -Type SymbolicLink  $lnk -Target $target  -Force

    # Make sure the relative target path was recorded as such and uses
    # the platform-appropriate separator.
    (Get-Item $lnk).Target | Should -Be ($target -replace '[\\/]', [IO.Path]::DirectorySeparatorChar)

  }

  AfterAll {
    Pop-Location
  }
}

Expected behavior

The tests should succeed on all platforms.

Actual behavior

On macOS and Linux, both tests fail:

The first test fails, because the relative path was converted to an absolute one.

      Expected: './95916.tmp'
      But was:  '/private/t...'

On macOS, the second test fails because \ wasn't normalized to /:

      Expected: './95916.tmp'
      But was:  '.\95916.tmp'

On Linux, by contrast, the test fails because the relative path is again unexpectedly converted to an absolute one.

Environment data

PowerShell Core 7.1.0-preview.4
@mklement0 mklement0 added the Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a label Jun 30, 2020
@mklement0 mklement0 changed the title New-Item Type SymbolicLink on Unix-like platforms: converts relative target paths to absolute ones, doesn't normalize path separators New-Item Type SymbolicLink on Unix-like platforms: converts relative target paths to absolute ones, doesn't normalize path separators on macOS Jun 30, 2020
@iSazonov
Copy link
Collaborator

@mklement0 I can not debug on Unix and investigate the issue. :-(

If you look tests in #12797 you can see that on MacOs we have to have a workaround for TestDrive - maybe it is related to the issue.
Also the fix is based on SessionState.Internal.CurrentLocation.ProviderPath - perhaps this works different on different platforms.

@iSazonov iSazonov added the WG-Cmdlets-Core cmdlets in the Microsoft.PowerShell.Core module label Jun 30, 2020
@mklement0
Copy link
Contributor Author

mklement0 commented Jun 30, 2020

@iSazonov I see. Not using TestDrive: (instead using a directory that isn't a symlink) on macOS doesn't make a difference - the tests still fail in the same way.

@iSazonov
Copy link
Collaborator

iSazonov commented Jul 1, 2020

I looked on WSL. I think it is how SessionState.Internal.CurrentLocation.ProviderPath works. On Windows it contains a relative path, on Linux - absolute one, on MacOs - ???.

@iSazonov iSazonov added Issue-Bug Issue has been identified as a bug in the product Up-for-Grabs Up-for-grabs issues are not high priorities, and may be opportunities for external contributors and removed Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a labels Jul 1, 2020
@mklement0
Copy link
Contributor Author

@iSazonov

Arguably, you shouldn't try to resolve a relative path at all: you should only check it for syntactic validity (e.g., no illegal characters) and normalize the separators.

Note that this ties in with #9067, which asks that it be possible to create symlinks to nonexistent (not yet existent) targets.

@iSazonov
Copy link
Collaborator

iSazonov commented Jul 1, 2020

There is a fundamental problem - there is no way to verify that path is correct without accessing the OS therefore resolving paths is preferred. I believe #9067 could be additive but explicit to avoid erroneous links due to typos.

@mklement0
Copy link
Contributor Author

@iSazonov

It's fine to resolve for the purpose of validation, but when it comes to creating the symlink, it's sufficient to use the given target path as-is, with only the separators normalized - this should fix the problem on macOS and Linux.

Incidentally, #9067 seemingly did get implemented in 7.0, perhaps accidentally, because it amounts to a breaking change: non-existent target paths are now quietly accepted (try New-Item -Type SymbolicLink tmpLnk -Target NoSuchTarget) - in 6.x and in Windows PowerShell you get an error instead.

@mklement0
Copy link
Contributor Author

mklement0 commented Apr 14, 2021

The macOS path-separator problem no longer occurs.

I've created a new issue that focuses just on the relative-path issue and also has an updated (working in v5) Pester test: #15233

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug Issue has been identified as a bug in the product Up-for-Grabs Up-for-grabs issues are not high priorities, and may be opportunities for external contributors WG-Cmdlets-Core cmdlets in the Microsoft.PowerShell.Core module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants