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

dgreen-arm · 2018-06-07T12:01:06Z

Some of the symlinks in PR #1470 were being accidentally created as directory symlinks on Windows, this adds a check to create the symlink of the correct type.

Backports will be needed for 2.7 and 2.1

gilles-peskine-arm

If this makes it works, ok. But please get a review from someone who actually knows Windows.

gilles-peskine-arm · 2018-06-07T12:21:54Z

CMakeLists.txt

@@ -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})


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?

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.

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.

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

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?

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.

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.

simonbutcher · 2018-06-08T09:01:02Z

As defined by MSDN, there are three different types of links as you can read here and here.

To do a very dumb and very quick summary, they are:

Symbolic links - for files or directories and can span volumes, but needs additional privileges (SeCreateSymbolicLinkPrivilege), so really needs to be a local Admin or a specially created account.
Hard links - only for files on the same volume
Junctions - only for directories on the same volume

We originally had symbolic links, which were changed to junctions in #688 to remove the need for Admin privileges, because it they didn't work on some development machines.

This PR is changing the junction to a symbolic link. e.g. from making a junction :

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

to a symbolic link:

set(command cmd.exe /c mklink ${link} ${target})

(Please see the mklink definition).

I think that line needs to be changed to the following to form a hardlink:

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

simonbutcher · 2018-06-08T09:01:48Z

CMakeLists.txt

+            if (IS_DIRECTORY ${target})
+                set(command cmd.exe /c mklink /j ${link} ${target})
+            else()
+                set(command cmd.exe /c mklink ${link} ${target})


This line needs to be a hardlink, not a symbolic link, to avoid requiring local Admin privileges. e.g.:

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

I don't think a hard link is reasonable. A legitimate, realistic use case. is to have a read-only source tree (whether for permission or performance reasons) and to do the build on a different, read-write filesystem.

If a symbolic link is not possible, we should copy.

I think that's the argument for #1496. What we want to do now is to get the build working again. Symbolic links are a regression, and will break the build on machines where it previously built.

What you're suggesting is reasonable, but not a regression, and therefore is an enhancement, which I think will be fixed by #1496.

I therefore see no reason to delay merging this fix assuming it passes the CI, for the sake of an enhancement.

cc: @Patater

Changing to a hard link is a regression from #1470. It passes on the CI by luck because of how we set up the directory structure for the builds, just like symbolic links pass on the CI by luck because our build environment happens to have the required permission.

simonbutcher · 2018-06-08T09:55:03Z

This passes the internal release and Windows CI, and is the change I requested, so I approve.

Patater · 2018-06-08T09:57:40Z

CMakeLists.txt

+            if (IS_DIRECTORY ${target})
+                set(command cmd.exe /c mklink /j ${link} ${target})
+            else()
+                set(command cmd.exe /c mklink /h ${link} ${target})


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.

simonbutcher · 2018-06-08T09:59:36Z

@dgreen-arm - I don't believe this needs backports, as the breaking change was never backported. You marked it as needing backports, so can you please confirm?

dgreen-arm · 2018-06-08T10:04:22Z

I marked it as needing backports as #1470 has backports, at least for 2.1 here #1465. I see it hasn't been merged yet, so no backport is currently needed.

Patater · 2018-06-11T09:21:55Z

In the interest of getting CI working again, we should merge this now and fix 2.1 and 2.7 in the PRs that would break them instead of waiting to break them and fixing them with a backport of this PR.

simonbutcher · 2018-06-11T11:00:30Z

I've pushed the fix for mbedtls-2.1 branch to #1465, so no backport is needed for that, and for mbedtls-2.7 branch, the original PR #1470 hasn't been merged yet, but when it is, this PR can be used as a fix as mbedtls-2.7 hasn't diverged far from development.

Therefore no backports needed!

simonbutcher · 2018-06-11T14:32:44Z

Merged into development.

simonbutcher · 2018-06-12T08:11:07Z

Merged PR into mbedtls-2.7.

Fix out-of-tree testing symlinks on Windows

d75ee64

dgreen-arm added mbed TLS team needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. component-platform Portability layer and build scripts labels Jun 7, 2018

dgreen-arm requested a review from gilles-peskine-arm June 7, 2018 12:01

gilles-peskine-arm approved these changes Jun 7, 2018

View reviewed changes

dgreen-arm requested a review from Patater June 7, 2018 12:44

simonbutcher self-requested a review June 7, 2018 17:11

simonbutcher suggested changes Jun 8, 2018

View reviewed changes

Change symlink to hardlink to avoid permission issues

2a1edac

simonbutcher approved these changes Jun 8, 2018

View reviewed changes

Patater approved these changes Jun 8, 2018

View reviewed changes

simonbutcher added approved for design and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 8, 2018

Patater mentioned this pull request Jun 11, 2018

Backport 2.1: Support out-of-tree testing with cmake #1465

Closed

simonbutcher added approved Design and code approved - may be waiting for CI or backports and removed needs-backports Backports are missing or are pending review and approval. labels Jun 11, 2018

simonbutcher merged commit 2a1edac into Mbed-TLS:development Jun 11, 2018

renzbagaporo mentioned this pull request Apr 28, 2019

Creating links only when ENABLE_TESTING is ON #2608

Closed

tom-cosgrove-arm mentioned this pull request Apr 19, 2022

Symlinking issues on Windows - Compile fails if build and src folder is on different drives. #5751

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dgreen-arm commented Jun 7, 2018

gilles-peskine-arm left a comment

gilles-peskine-arm Jun 7, 2018

dgreen-arm Jun 7, 2018

Patater Jun 7, 2018

gilles-peskine-arm Jun 7, 2018

Patater Jun 8, 2018

gilles-peskine-arm Jun 8, 2018 •

edited

Loading

simonbutcher Jun 8, 2018

simonbutcher commented Jun 8, 2018

simonbutcher Jun 8, 2018

gilles-peskine-arm Jun 8, 2018

simonbutcher Jun 8, 2018

gilles-peskine-arm Jun 8, 2018

simonbutcher commented Jun 8, 2018

Patater Jun 8, 2018

simonbutcher commented Jun 8, 2018

dgreen-arm commented Jun 8, 2018

Patater commented Jun 11, 2018

simonbutcher commented Jun 11, 2018

simonbutcher commented Jun 11, 2018

simonbutcher commented Jun 12, 2018

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

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

Conversation

dgreen-arm commented Jun 7, 2018

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbutcher commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbutcher commented Jun 8, 2018

Choose a reason for hiding this comment

simonbutcher commented Jun 8, 2018

dgreen-arm commented Jun 8, 2018

Patater commented Jun 11, 2018

simonbutcher commented Jun 11, 2018

simonbutcher commented Jun 11, 2018

simonbutcher commented Jun 12, 2018

gilles-peskine-arm Jun 8, 2018 •

edited

Loading