Skip to content

Conversation

SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Jan 29, 2019

PR Summary

The current code always tries to glob the target path which results in an absolute path and limits the usefulness of creating a symlink. Fix is to see if the target path even contains any wildcards, if not, don't use the resolved globbed path and instead use the value as literal. Since there isn't a -LiteralTarget parameter, this does mean that the globber can fail if the target path contains a wildcard character intended to be literal, but that is not in scope of this PR.

Note that relative links are only supported on Windows so skipping test on non-Windows. On non-Windows, the relative path is passed to the symlink() api which ends up creating an absolute link. Using the ln command line tool has the same result.

PR Context

Fix #3500

PR Checklist

@SteveL-MSFT
Copy link
Member Author

Codefactor issues are on untouched code and incorrectly identifying Hungarian notation

@anmenaga
Copy link

FileSystem.Tests.ps1 failed in all 3 CI systems.
This looks suspicious considering that the change is in SessionStateContainer.cs. :)

@SteveL-MSFT
Copy link
Member Author

Looking

@SteveL-MSFT
Copy link
Member Author

The spelling error in static analysis is unrelated to this PR. The CodeFactor issues are also unrelated to the changes in this PR.

…em.Tests.ps1

Co-Authored-By: SteveL-MSFT <slee@microsoft.com>
@SteveL-MSFT
Copy link
Member Author

@anmenaga I believe this is ready to merge

@anmenaga
Copy link

anmenaga commented Feb 6, 2019

I'd like to see sign offs from @iSazonov and @vexx32 .

@iSazonov
Copy link
Collaborator

iSazonov commented Feb 7, 2019

I have only one open question above.

Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

LGTM with one minor comment.

Copy link
Collaborator

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

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

Nothing I can see that hasn't already been mentioned; looks good! 🙂

Co-Authored-By: SteveL-MSFT <slee@microsoft.com>
@anmenaga anmenaga merged commit 26a5867 into PowerShell:master Feb 7, 2019
@anmenaga anmenaga added CL-Engine Indicates that a PR should be marked as an engine change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log and removed CL-Engine Indicates that a PR should be marked as an engine change in the Change Log labels Feb 7, 2019
@mklement0
Copy link
Contributor

@SteveL-MSFT:

Thanks for fixing this.

On non-Windows, the relative path is passed to the symlink() api which ends up creating an absolute link.

I just tried the new code on my Mac, and it was able to create a symlink with a relative path just fine.

Using the ln command line tool has the same result.

Similarly, ln on macOS and Linux creates symlinks with relative target paths, if given a relative path.
In fact, that's the typical use case.

I suggest enabling the tests for Unix-like platforms too.

@SteveL-MSFT
Copy link
Member Author

SteveL-MSFT commented Feb 8, 2019

@mklement0 I tried it on my macBook and it didn't create a relative link. Not sure why it's different for me as the tests failed on my system and was also failing in CI.

@SteveL-MSFT SteveL-MSFT deleted the relative-symlink branch February 8, 2019 06:04
@mklement0
Copy link
Contributor

@SteveL-MSFT:

To verify that ln is capable of creating relative symlinks on both macOS and Linux, run the following test:

Describe "Relative symlink path test" {
  It "ln creates a symlink with a relative path when given one" {
    ln -s . /tmp/$pid
    readlink /tmp/$pid | should -BeExactly '.'
    rm /tmp/$pid
  }
}

@mklement0
Copy link
Contributor

@SteveL-MSFT, inferring the correct symlink type on Windows seems to be broken with relative paths to existing directories - see #9127

On a related note, we must come up with a way to explicitly specify the target type for nonexistent (not-yet-extant) targets - see #9067 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New-Item -ItemType SymbolicLink should support creating symlinks with relative paths
6 participants