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.

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

Copy files instead of hard-linking on Windows #6188

Conversation

N3xed
Copy link
Contributor

@N3xed N3xed commented Aug 9, 2022

Description

Fixes an issue on Windows where when the source and build directory are on different drives hard-linking to files or directory fails as it doesn't work across filesystem boundaries.
Note also that symlinking is also not possible because it requires administrator privileges on Windows.

The solution copies the files using the built-in cmake file(COPY) command.
The solution copies the files using the built-in cmake configure_file(src dest COPYONLY) command.
As this command only operates on files, if a directory is specified the files will be globbed recursively
and through symlinks.

Status

READY

Additional comments

Fixes #5751

Todos

  • Changelog updated

@N3xed N3xed force-pushed the fix/windows-different-drives-build-error branch 2 times, most recently from 21419a6 to 2238942 Compare August 9, 2022 21:18
@gilles-peskine-arm gilles-peskine-arm added bug needs-review Every commit must be reviewed by at least two team members, component-platform Portability layer and build scripts needs-reviewer This PR needs someone to pick it up for review priority-low Low priority - this may not receive review soon needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Aug 10, 2022
@igrr
Copy link

igrr commented Aug 10, 2022

@N3xed Would you consider using configure_file(src dest COPY_ONLY) instead of file(COPY)?

I think file(COPY) has two downsides:

  1. It will update the destination file modification time if CMake configure stage runs again, which will result in the source file being rebuilt, even if the content is the same.
  2. It will not detect changes to the original file and won't copy it again (unless CMake configure stage runs).

configure_file(src dest COPY_ONLY) would address both issues.

Fixes an issue on Windows where when source and build directory are on different drives hard-linking
to files or directory fails as it doesn't work across filesystem boundaries. Note that symlinking is also
not possible because it requires administrator privileges on Windows.

The solution copies the files using the built-in cmake `configure_file(src dest COPYONLY)` command.
As this command only operates on files, if a directory is specified the files will be globbed recursively
and through symlinks.

Signed-off-by: Dominik Gschwind <[email protected]>
@N3xed N3xed force-pushed the fix/windows-different-drives-build-error branch from 2238942 to c6d1636 Compare August 10, 2022 14:27
@N3xed
Copy link
Contributor Author

N3xed commented Aug 10, 2022

@N3xed Would you consider using configure_file(src dest COPY_ONLY) instead of file(COPY)?

I think this would be better, indeed. But as a result, when a directory is specified, the files are now globbed because configure_file doesn't work with directories.

Additionally, instead of copying every time on Windows, we could check if the ${target} and ${link} are on the same drive and then hard-link instead. I've opted not to do this since this would complicate it even more and the current implementation already works. I could implement that as well, though, if requested.

@igrr
Copy link

igrr commented Aug 10, 2022

I've opted not to do this since this would complicate it even more and the current implementation already works. I could implement that as well, though, if requested.

Thant makes sense to me, I think copying is simpler and more reliable than detecting if files are on the same drive.

@igrr
Copy link

igrr commented Aug 11, 2022

Looks like this version passed CI, so at least it doesn't break anything it seems.

From the perspective of the esp-idf project where the issue was originally reported, this change looks good to me. Hope it can be merged upstream.

@daverodgman daverodgman added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels Aug 15, 2022
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

Copy link
Contributor

@wernerlewis wernerlewis left a comment

Choose a reason for hiding this comment

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

LGTM

@wernerlewis wernerlewis added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Sep 12, 2022
@mpg
Copy link
Contributor

mpg commented Sep 22, 2022

Just commenting so that the pr-merge job runs again tonight.

@mpg mpg added the needs-ci Needs to pass CI tests label Sep 22, 2022
@gilles-peskine-arm gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Sep 23, 2022
@gilles-peskine-arm gilles-peskine-arm merged commit 8c2d236 into Mbed-TLS:development Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports bug component-platform Portability layer and build scripts priority-low Low priority - this may not receive review soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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