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] Use std::filesystem when Visual Studio is greater than 2015 #12774

Merged
merged 15 commits into from
Aug 18, 2020

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Aug 6, 2020

  • When the Visual Studio version is 2015, use std::experimental::filesystem
  • When the Visual Studio version is greater than 2015, use std::filesystem

Fixes #9132.

@JackBoosY JackBoosY added category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) info:internal This PR or Issue was filed by the vcpkg team. labels Aug 6, 2020
@etheory
Copy link

etheory commented Aug 6, 2020

In the process of testing this now, thank you.
It's not building for me.
I get tens of pages of warnings and errors, sorry.
vcpkgpaths.cpp(245,39): error C4996: 'std::filesystem::u8path'

warning STL4021: The std::filesystem::u8path() overloads are deprecated in C++20. T
he constructors of std::filesystem::path provide equivalent functionality via construction from u8string, u8string_view, or iterators with value_type char8_t.You can define _SILENCE_CXX20_U8PATH_DEPREC
ATION_WARNING or _SILENCE_ALL_CXX20_DEPRECATION_WARNINGS to acknowledge that you have received this warning. (compiling source file ..\src\vcpkg\vcpkgpaths.cpp) [Z:\Luke\Programming\Graphics\vcpkg2\too
lsrc\vcpkglib\vcpkglib.vcxproj]

and many others.

I'm using the latest:
Visual Studio 2019 Developer Command Prompt v16.6.5

@strega-nil
Copy link
Contributor

Yeah, we need to turn off that warning, because that deprecation is not a thing we can deal with rn since we need to work back to VS2015

@BillyONeal
Copy link
Member

I think this is just another reason to get off std::filesystem.

@etheory
Copy link

etheory commented Aug 7, 2020

OK, just did a git pull, and it did re-build this time without errors.
Trying now to see if it works when trying to do a package install.
Will report back shortly....

@etheory
Copy link

etheory commented Aug 7, 2020

So far so good.
The install is currently not giving me any rename or copy errors.
Will keep you posted if I can get all the way to the end of a full install.
Might take a while, but I will let you know.

@etheory
Copy link

etheory commented Aug 7, 2020

Nope, the dreaded issue is back!
(Note I've manually edited the below file-paths just to remove references to my local drive, but safely assume on my end that is a full path)

PS **\Programming\Graphics\vcpkg2> .\vcpkg.exe install openimageio --triplet x64-windows
Computing installation plan...
A suitable version of cmake was not found (required v3.17.2). Downloading portable cmake v3.17.2...
Downloading cmake...
  https://github.com/Kitware/CMake/releases/download/v3.17.2/cmake-3.17.2-win32-x86.zip -> **\Programming\Graphics\vcpkg2\downloads\cmake-3.17.2-win32-x86.zip
Extracting cmake...
A suitable version of 7zip was not found (required v18.1.0). Downloading portable 7zip v18.1.0...
Downloading 7zip...
  https://www.nuget.org/api/v2/package/7-Zip.CommandLine/18.1.0 -> **\Programming\Graphics\vcpkg2\downloads\7-zip.commandline.18.1.0.nupkg
Extracting 7zip...
A suitable version of nuget was not found (required v5.5.1). Downloading portable nuget v5.5.1...
Downloading nuget...
  https://dist.nuget.org/win-x86-commandline/v5.5.1/nuget.exe -> **\Programming\Graphics\vcpkg2\downloads\22ea847d-nuget.exe
Failed to do post-extract rename-in-place.
fs.rename(**\Programming\Graphics\vcpkg2\downloads\tools\cmake-3.17.2-windows.partial.9060, **\Programming\Graphics\vcpkg2\downloads\tools\cmake-3.17.2-windows, Access is denied.)

FWIW I'm running this through a Windows Powershell using the very very latest Microsoft Visual Studio Comunity 2019 - 16.7.0.
Also didn't work via "x64 Native Tools Command Prompt for VS 2019".

@BillyONeal
Copy link
Member

Can you try running it under procmon and see which process is holding the file locked?

@JackBoosY
Copy link
Contributor Author

And can you try to use powershell with admin permission?

@etheory
Copy link

etheory commented Aug 7, 2020

Admin permissions is not something I want to have to revert to sorry @JackBoosY and I can't anyway, since for some reason Windows hides my network drive in Admin mode (I never figured out why).

Could the issue be that I'm working from a network drive?
A friend of mine has suggested that file locking on network drives doesn't work on Windows properly.
Could this be the reason?

I'll try again from C: and let you know.

I wasn't aware that Windows performed so poorly when accessing network drives.

@etheory
Copy link

etheory commented Aug 7, 2020

@JackBoosY

OK. Somewhat incredibly anticlimactically, this patch isn't required.

The issue is that vcpkg doesn't work with network drives.

I tried again with a fresh vcpkg install on C: and everything worked perfectly.

That's possibly the reason other users are having issues also....

It would be great if it DID work on network drives, but, it doesn't.

@JackBoosY
Copy link
Contributor Author

JackBoosY commented Aug 7, 2020

From network driver... that's we didn't expected. I think that also affect WSL.
@BillyONeal What do you think about?

@BillyONeal
Copy link
Member

Is it both VS2015 and being on a network drive needed to trigger the problem? We intend to remove all std::experimental paths when we start shipping binaries, always building vcpkg.exe with VS2019.

@etheory
Copy link

etheory commented Aug 7, 2020

@BillyONeal no, only the location of vcpkg.
I have all VS2019 (I don't use VC2015) installations on C: and was trying to work from a network drive with my code on it.
I installed vcpkg itself, and it's packages to the network location.
That's what fails.

I'm not sure what would happen if both VS2019 AND vcpkg were on the network location, since that's not my use-case.

Moving all my stuff to C: is working perfectly now.

@Neumann-A
Copy link
Contributor

I have all VS2019 (I don't use VC2015) installations on C: and was trying to work from a network drive with my code on it.
I installed vcpkg itself, and it's packages to the network location.

Have you tried subst'ing the network drive away?

e.g. subst N: <networkpath> but I highly discourage that usage since the linker/compile speed will suffer dramatically due to a lot of small files and network speed etc.

@etheory
Copy link

etheory commented Aug 7, 2020

Have you tried subst'ing the network drive away?

e.g. subst N: <networkpath> but I highly discourage that usage since the linker/compile speed will suffer dramatically due to a lot of small files and network speed etc.

I already had used subst.
The drive letter was Z: and it mapped to a synology DiskStation, which is normally accessed as \\DiskStation.

And it didn't work with vcpkg.

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
@BillyONeal BillyONeal self-assigned this Aug 7, 2020
#if defined(_WIN32)
return vcpkg::Strings::to_utf8(p.native());
#else
return p.string();
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be p.native() since we're making an assumption that it'll be UTF-8.

#if defined(_WIN32)
return vcpkg::Strings::to_utf8(p.generic_wstring());
#else
return p.string();
Copy link
Member

Choose a reason for hiding this comment

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

That can't be right, it at least needs to be generic_string

@strega-nil strega-nil self-requested a review August 7, 2020 19:51
@etheory
Copy link

etheory commented Aug 9, 2020

If you need someone to test this once the PR has stabilized, please let me know!

@strega-nil
Copy link
Contributor

@etheory it should be stable now; the one concern I want to fix is getting CMake to support compiling with VS 2015 (so we can continue to test VS2015 even without the VS build system), which will be fixed in a different PR.

@BillyONeal
Copy link
Member

I see, so it's not just being a network share, it's also a "samba" server. It's possible that they don't implement our attempts to canonicalize the vcpkg root path. :/

@etheory
Copy link

etheory commented Aug 15, 2020

I'm testing this now....
Unfortunately it doesn't work.
It produces exactly the same issue as before from a network location:

PS z:\Luke\Programming\Graphics\vcpkg> .\vcpkg.exe install openimageio --triplet x64-windows
Computing installation plan...
A suitable version of cmake was not found (required v3.17.2). Downloading portable cmake v3.17.2...
Downloading cmake...
  https://github.com/Kitware/CMake/releases/download/v3.17.2/cmake-3.17.2-win32-x86.zip -> Z:\Luke\Programming\Graphics\vcpkg\downloads\cmake-3.17.2-win32-x86.zip
Extracting cmake...
A suitable version of 7zip was not found (required v18.1.0). Downloading portable 7zip v18.1.0...
Downloading 7zip...
  https://www.nuget.org/api/v2/package/7-Zip.CommandLine/18.1.0 -> Z:\Luke\Programming\Graphics\vcpkg\downloads\7-zip.commandline.18.1.0.nupkg
Extracting 7zip...
A suitable version of nuget was not found (required v5.5.1). Downloading portable nuget v5.5.1...
Downloading nuget...
  https://dist.nuget.org/win-x86-commandline/v5.5.1/nuget.exe -> Z:\Luke\Programming\Graphics\vcpkg\downloads\22ea847d-nuget.exe
Failed to do post-extract rename-in-place.
fs.rename(Z:\Luke\Programming\Graphics\vcpkg\downloads\tools\cmake-3.17.2-windows.partial.11688, Z:\Luke\Programming\Graphics\vcpkg\downloads\tools\cmake-3.17.2-windows, Access is denied.)

@etheory
Copy link

etheory commented Aug 18, 2020

Should I test this again? As far as I'm aware this PR has been approved but still doesn't fix the issue I'm encountering?

@strega-nil
Copy link
Contributor

@etheory unfortunately, there's nothing much we can do if it doesn't fix your issue; fs::rename is a straight MoveFileEx -- if MoveFileEx doesn't work, nothing works.

@BillyONeal BillyONeal merged commit 0bb8780 into microsoft:master Aug 18, 2020
@BillyONeal
Copy link
Member

Thanks for your fix Jack!

@JackBoosY JackBoosY deleted the dev/jack/use_std_filesystem branch August 19, 2020 02:07
@etheory
Copy link

etheory commented Aug 20, 2020

@etheory unfortunately, there's nothing much we can do if it doesn't fix your issue; fs::rename is a straight MoveFileEx -- if MoveFileEx doesn't work, nothing works.

If that's the case, it might be pertinent to add to the vcpkg documentation that it doesn't work when run on network drives.

@BillyONeal
Copy link
Member

It does work on network drives, just not those hosted on whatever software this Synology uses.

remz1337 pushed a commit to remz1337/vcpkg that referenced this pull request Aug 23, 2020
…icrosoft#12774)

Co-authored-by: Nicole Mazzuca <mazzucan@outlook.com>
Co-authored-by: Billy Robert O'Neal III <bion@microsoft.com>
strega-nil added a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021
…icrosoft#12774)

Co-authored-by: Nicole Mazzuca <mazzucan@outlook.com>
Co-authored-by: Billy Robert O'Neal III <bion@microsoft.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:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to rename files using fs.rename
7 participants