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

[vcpkg] implement copy_symlink working for non-elevated processes #12400

Merged
merged 14 commits into from
Jul 15, 2020

Conversation

Maximus5
Copy link
Contributor

Function fs::stdfs::copy_symlink used by install_files_and_write_listfile requires the vcpkg process to be running elevated (Windows). Otherwise it always failed with operation not permitted error.

This PR fixes inability to copy symlink in non-elevated processes.
I tried to instal libressl via command

install --recurse libressl[core]:x64-windows

Description of the problem is here: #11949 (comment)

{
#if defined(_WIN32)
const DWORD flags = SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE;
if (!CreateSymbolicLinkW(newpath.c_str(), oldpath.c_str(), flags))
Copy link
Contributor

@strega-nil strega-nil Jul 13, 2020

Choose a reason for hiding this comment

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

This doesn't work, I don't think -- this has newpath be a symlink for oldpath, which requires that oldpath continues existing; what you really want is newpath to be a symlink to the thing oldpath points to. Maybe you want:

Suggested change
if (!CreateSymbolicLinkW(newpath.c_str(), oldpath.c_str(), flags))
if (!CreateSymbolicLinkW(newpath.c_str(), fs::stdfs::read_symlink(oldpath).c_str(), flags))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that okay, that symlink points to packages folder?
In either case fs::stdfs::read_symlink does not work at all: _Map_mode from filesys.cpp does not check FILE_ATTRIBUTE_REPARSE_POINT and does not return file_type::symlink.
Furthermore _Symlink_get returns empty string. It's not implemented yet in my VS 2019.

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 okay; this is not a copy_symlink at all, it's a create_symlink

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Than read_symlink is not a solution, even if it works. Right?

The proper logic will be:

  • get target of oldpath (it's in packages)
  • map oldpath to new_oldpath (the original file copied from packages to installed folder)
  • create symlink newpath targeted to new_oldpath

Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not okay; this is not a copy_symlink at all, it's a create_symlink

Or what did you mean? What should happen, when vcpkg copies file_v1 + file (symlink pointed to file_v1) from packages to installed folder?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not a question I'm interested in answering -- we want copy_symlink to do what copy_symlink says it does. That means, take a file that is a symlink, and copy that file (i.e., create a new file with the same target as the original symlink). copy_symlink needs to do that, not create a new symlink that links to the old file.

Copy link
Member

Choose a reason for hiding this comment

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

std::experimental::filesystem will never be updated to support symlinks; it will be left in a bug for bug compatible with VS2015 state. That implementation had too many bugs and the filesystem TS vs. what got standardized were too divergent to share implementations. std::filesystem is a complete replacement implementation that does support symlinks. As a result _Map_mode will never be changed to enable such support.

The only symlinks that make sense here are relative symlinks, that when copied, still form the correct path in their new location. The "packages" directory is not generally persistent; for example when packages are restored from a binary cache only the 'installed' directory is restored. As a result I don't believe this change will ever persist a symlink correctly :(

@Maximus5 Maximus5 requested a review from strega-nil July 14, 2020 08:14
@strega-nil
Copy link
Contributor

Why can't we use std::filesystem::read_symlink here?

@Maximus5
Copy link
Contributor Author

Why can't we use std::filesystem::read_symlink here?

std::filesystem::read_symlink is available from C++17 only.
The project properties don't allow this language standard.

and std::filesystem::experimental::read_symlink just doesn't work - it returns empty string.

toolsrc/src/vcpkg/base/files.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/base/files.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/base/files.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/base/files.cpp Outdated Show resolved Hide resolved
Maximus5 and others added 4 commits July 14, 2020 19:20
Co-authored-by: nicole mazzuca <mazzucan@outlook.com>
Co-authored-by: nicole mazzuca <mazzucan@outlook.com>
Co-authored-by: nicole mazzuca <mazzucan@outlook.com>
Copy link
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

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

Just some more minor changes, then I'm good with merging :)

It's still not perfect; given a -> b -> c (a, b symlinks with a pointing to b, b pointing to c), copy_symlink(a, d) will give you d -> c, whereas in a perfect world you'd get d -> b -> c. Unfortunately, Windows does not support this feature without using DeviceIoControl, and so I'm willing to deal with this for now. When we rewrite the files code to remove std::filesystem, that'll be a thing to do -- specifically, using https://docs.microsoft.com/en-us/windows-hardware/drivers/ifs/fsctl-get-reparse-point

toolsrc/src/vcpkg/base/files.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/base/files.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/base/files.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/base/files.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/base/files.cpp Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/base/files.cpp Show resolved Hide resolved
toolsrc/src/vcpkg/base/files.cpp Outdated Show resolved Hide resolved
Maximus5 and others added 7 commits July 14, 2020 19:55
Co-authored-by: nicole mazzuca <mazzucan@outlook.com>
Co-authored-by: nicole mazzuca <mazzucan@outlook.com>
Co-authored-by: nicole mazzuca <mazzucan@outlook.com>
Co-authored-by: nicole mazzuca <mazzucan@outlook.com>
Co-authored-by: nicole mazzuca <mazzucan@outlook.com>
@Maximus5 Maximus5 requested a review from strega-nil July 15, 2020 07:41
@strega-nil strega-nil added category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) info:reviewed Pull Request changes follow basic guidelines labels Jul 15, 2020
@ras0219-msft ras0219-msft merged commit 8254d3b into microsoft:master Jul 15, 2020
@ras0219-msft
Copy link
Contributor

Thanks everyone!

@BillyONeal
Copy link
Member

Unfortunately this implements read_symlink as canonical which breaks relative symlinks -- so it now succeeds but all the symlinks point into the packages tree which is about to be deleted.

@Maximus5
Copy link
Contributor Author

I've asked about symlinks pointed to packages folder.
#12400 (comment)

@BillyONeal
Copy link
Member

@Maximus5 I see; sorry for not noticing before; I just found this PR through a 'git blame'.

strega-nil added a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021
…crosoft#12400)

* [vcpkg] implement copy_symlink working for non-elevated processes

* [vcpkg] read_symlink Windows implementation

* [vcpkg] normalize_path on Windows only

* Update toolsrc/src/vcpkg/base/files.cpp

Co-authored-by: nicole mazzuca <mazzucan@outlook.com>

* Update toolsrc/src/vcpkg/base/files.cpp

Co-authored-by: nicole mazzuca <mazzucan@outlook.com>

* Update toolsrc/src/vcpkg/base/files.cpp

Co-authored-by: nicole mazzuca <mazzucan@outlook.com>

* remove normalization

* Update toolsrc/src/vcpkg/base/files.cpp

Co-authored-by: nicole mazzuca <mazzucan@outlook.com>

* Update toolsrc/src/vcpkg/base/files.cpp

Co-authored-by: nicole mazzuca <mazzucan@outlook.com>

* Update toolsrc/src/vcpkg/base/files.cpp

Co-authored-by: nicole mazzuca <mazzucan@outlook.com>

* Update toolsrc/src/vcpkg/base/files.cpp

Co-authored-by: nicole mazzuca <mazzucan@outlook.com>

* Update toolsrc/src/vcpkg/base/files.cpp

Co-authored-by: nicole mazzuca <mazzucan@outlook.com>

* use unique_ptr

* comments

Co-authored-by: nicole mazzuca <mazzucan@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants