Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Enable creating relative symlinks with New-Item#8783

Merged
anmenaga merged 6 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
SteveL-MSFT:relative-symlinkSteveL-MSFT/PowerShell:relative-symlinkCopy head branch name to clipboard
Feb 7, 2019
Merged

Enable creating relative symlinks with New-Item#8783
anmenaga merged 6 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
SteveL-MSFT:relative-symlinkSteveL-MSFT/PowerShell:relative-symlinkCopy head branch name to clipboard

Conversation

@SteveL-MSFT

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

Copy link
Copy Markdown
Member

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
Copy Markdown
Member Author

Codefactor issues are on untouched code and incorrectly identifying Hungarian notation

@anmenaga

Copy link
Copy Markdown

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

@SteveL-MSFT

Copy link
Copy Markdown
Member Author

Looking

@SteveL-MSFT

Copy link
Copy Markdown
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.

Comment thread src/System.Management.Automation/engine/SessionStateContainer.cs
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/New-Item.Tests.ps1 Outdated
Comment thread test/powershell/Modules/Microsoft.PowerShell.Management/New-Item.Tests.ps1 Outdated
…em.Tests.ps1

Co-Authored-By: SteveL-MSFT <slee@microsoft.com>
@SteveL-MSFT

Copy link
Copy Markdown
Member Author

@anmenaga I believe this is ready to merge

@anmenaga

anmenaga commented Feb 6, 2019

Copy link
Copy Markdown

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

@iSazonov

iSazonov commented Feb 7, 2019

Copy link
Copy Markdown
Collaborator

I have only one open question above.

Comment thread src/System.Management.Automation/engine/SessionStateContainer.cs Outdated

@iSazonov iSazonov left a comment

Copy link
Copy Markdown
Collaborator

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.

@vexx32 vexx32 left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

SteveL-MSFT commented Feb 8, 2019

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
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
Copy Markdown
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.