-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Create proper relative symbolic links #8251
base: main
Are you sure you want to change the base?
Create proper relative symbolic links #8251
Conversation
8458d54
to
22be40e
Compare
The azure pipeline doesn't give a proper line number for the failing MacOS test and I don't see any artefacts I can look at. |
|
5da1379
to
3c4f76c
Compare
errorMessage = symbolicLinkCreated ? null : "The link() library call failed with the following error code: " + Marshal.GetLastWin32Error(); | ||
} | ||
|
||
return symbolicLinkCreated; | ||
} | ||
|
||
internal static string GetRelativePath(string relativeTo, string path) |
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.
Did you write this method yourself? It looks correct to me, as far as I can tell, but it's substantially longer than an answer using URIs:
https://stackoverflow.com/questions/275689/how-to-get-relative-path-from-absolute-path
Might that work? My primary concern is that it'd be pretty easy to get this slightly wrong in an important way and have it fail. That is, I think it's right, but I'm not confident.
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.
As the comment in the method says, it is based upon .NET 7 Path.cs
The unit test cases also come from .NET7.
I updated the code to using the Uri.MakeRelativeUri
method as used in the article, but many of the unit test cases failed. Even after adding a trailing slash, still many tests failed. Seeing all the comments on the article, I'm not the only one. Uri also have a far more restricted character set than files.
I therefore think the original code is appropriate.
Why does msbuild need a build target for net472 and netstandard20? Do you update msbuild for older SDKs?
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
@ladipro this PR will need |
3c4f76c
to
cc2d757
Compare
Done. |
cc2d757
to
353b02c
Compare
353b02c
to
4e64fbf
Compare
Thank you for taking care of it! We may want to move the code from the constructor to a helper method so it's not duplicated in |
And apologies for missing Jan's mention. |
Fixes #3730
Context
Symbolic link in the
bin
folder toobj/Assembly.dll
were created just like that.Changes Made
Converted relative paths to be relative to the target directory resulting in:
../obj/Assembly.dll
Modified
NativeMethods.MakeSymbolicLink
Testing
Updated one unit test to test both absolute and relative paths. This failed before my changes, succeeds now.
As
Path.GetRelativePath
is not supported onnetstandard2.0
andnet472
, I added a local version with appropriate unit tests.Notes