Skip to content
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.

Sign up for GitHub

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

Fix out-of-tree testing symlinks on Windows #1699

Merged
merged 2 commits into from
Jun 11, 2018
Merged

Fix out-of-tree testing symlinks on Windows #1699

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d75ee64
Fix out-of-tree testing symlinks on Windows
dgreen-arm Jun 7, 2018
2a1edac
Change symlink to hardlink to avoid permission issues
dgreen-arm Jun 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,11 @@ function(link_to_source base_name)
if (CMAKE_HOST_UNIX)
set(command ln -s ${target} ${link})
else()
set(command cmd.exe /c mklink /j ${link} ${target})
if (IS_DIRECTORY ${target})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation of mklink tells me that /j “Creates a Directory Junction”, but I have no idea why you'd use one as opposed to the default mode. It does sound like it wouldn't work for non-directories. Why is /j used for directories?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not clued up on the intricacies of exactly how junctions works, but Windows differentiates between symlinks for files and directories. I'll do a deeper dive into the documentation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no expret, but I believe junctions can only be to folders, but symbolic links can be to either folders or files. On some systems, you may be able to create a junction where you couldn't create any symbolic links due to permissions issues (fewer permissions required for junction making), but on our test system we seem to have permission enough to make symlinks.

Since we can make symlinks, was using the /d option to mklink considered? It creates a directory symbolic link.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not just for our test systems. #688 changed from /d to /j for permission reasons.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may introduce a regression, then. Since it goes back to using symbolic links for files, the permissions elevation may be required again.

Since the sh files aren't needed by Windows anyway, could we avoid making links for them and get away with only making junctions?

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm Jun 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the .sh files are needed by Windows. They're how the Windows tests are invoked. We don't currently run SSL tests in an out-of-tree CMake build on Windows on our CI, but this should work for the sake of users who build on Windows and use CMake. We tend to use CMake in-tree on our CI, but that's not the way most people use CMake.

If we can't make symbolic links work on Windows, then we should copy the files. This has the downside that the build directory will contain stale copies if the original file has been edited, so the build scripts should be modified to make the a build product of the original that gets rebuilt (copied) when the original is updated, preferably only on Windows.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did suggest when we reviewed the original PR #1470, that it should also be able to copy files, and raised it as issue #1496.

I still think that work needs to be done, but I think we can defer it for now, and just fix the use of mklink as I suggest elsewhere.

set(command cmd.exe /c mklink /j ${link} ${target})
else()
set(command cmd.exe /c mklink /h ${link} ${target})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be fine with this as either symlink or hardlink, although symlink gives a better experience in certain use cases IF you have the permissions to use them.

endif()
endif()

execute_process(COMMAND ${command}
Expand Down