Improve windows symlink handling #715
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request modernizes the symbolic link handling implementation by removing the dependency on NCode.ReparsePoints and implementing native Windows symlink functionality. The changes consolidate symlink operations into platform-specific implementations while maintaining compatibility across multiple .NET versions.
Key changes:
- Replaces NCode.ReparsePoints dependency with native Windows P/Invoke implementation
- Consolidates file and directory symlink creation into a unified method
- Adds comprehensive test coverage for various symlink scenarios and edge cases
Reviewed Changes
Copilot reviewed 4 out of 16 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/Velopack.Tests/SymbolicLinkTests.cs | Adds extensive test coverage for symlink operations, error handling, and cross-platform compatibility |
| src/lib-csharp/Util/SymbolicLink.cs | Refactors main symlink logic to use unified creation method and removes NCode dependency |
| src/lib-csharp/Util/SymbolicLink.Windows.cs | Implements native Windows symlink operations using P/Invoke with reparse point handling |
| src/lib-csharp/Util/SymbolicLink.Unix.cs | Extracts Unix-specific symlink operations with EINTR signal handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| } | ||
|
|
||
| [DllImport("kernel32.dll", CharSet = CharSet.Unicode)] | ||
| static extern bool CreateHardLink(string lpFileName, string lpExistingFileName, IntPtr lpSecurityAttributes); |
There was a problem hiding this comment.
[nitpick] The P/Invoke declaration for CreateHardLink should be moved to a separate platform-specific file or at least grouped with other Windows-specific code, rather than being embedded in the test class.
| static extern bool CreateHardLink(string lpFileName, string lpExistingFileName, IntPtr lpSecurityAttributes); | |
| // Windows-specific interop methods | |
| internal static class WindowsInterop | |
| { | |
| [DllImport("kernel32.dll", CharSet = CharSet.Unicode)] | |
| public static extern bool CreateHardLink(string lpFileName, string lpExistingFileName, IntPtr lpSecurityAttributes); | |
| } |
| { | ||
| if (!PInvokeWindowsCreateSymlink(linkPath, target, mode)) { | ||
| var errorCode = Marshal.GetLastWin32Error(); | ||
| throw new InvalidOperationException($"Error creating symlink: {errorCode}", new Win32Exception()); |
There was a problem hiding this comment.
The error message includes the error code but doesn't include the actual Win32Exception message. Consider using new Win32Exception(errorCode) to get the descriptive error message, or include both the code and description in the message.
| throw new InvalidOperationException($"Error creating symlink: {errorCode}", new Win32Exception()); | |
| var win32Ex = new Win32Exception(errorCode); | |
| throw new InvalidOperationException($"Error creating symlink: {errorCode} - {win32Ex.Message}", win32Ex); |
No description provided.