-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Conversation
Codefactor issues are on untouched code and incorrectly identifying Hungarian notation |
|
Looking |
The spelling error in static analysis is unrelated to this PR. The CodeFactor issues are also unrelated to the changes in this PR. |
test/powershell/Modules/Microsoft.PowerShell.Management/New-Item.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Management/New-Item.Tests.ps1
Outdated
Show resolved
Hide resolved
…em.Tests.ps1 Co-Authored-By: SteveL-MSFT <slee@microsoft.com>
@anmenaga I believe this is ready to merge |
I have only one open question above. |
src/System.Management.Automation/engine/SessionStateContainer.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
There was a problem hiding this 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>
Thanks for fixing this.
I just tried the new code on my Mac, and it was able to create a symlink with a relative path just fine.
Similarly, I suggest enabling the tests for Unix-like platforms too. |
@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. |
To verify that 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
}
} |
@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) |
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
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.[feature]
to your commit messages if the change is significant or affects feature tests