8000 Enable creating relative symlinks with `New-Item` by SteveL-MSFT · Pull Request #8783 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

Enable creating relative symlinks with New-Item #8783

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

Merged
merged 6 commits into from
Feb 7, 2019

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
8000

@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
0