search

mosra / corrade

C++11 multiplatform utility library
https://magnum.graphics/corrade/
Other
482 stars 105 forks source link

Utility: make Path::move() use MoveFileEx() on Windows. #143

Closed williamjcm closed 2 years ago

williamjcm commented 2 years ago

This should prevent failures when the destination already exists. Also added a test to check the behaviour.

As mentioned on Gitter, the MOVEFILE_COPY_ALLOWED flag is only used when to is on a different volume than from, at least according to the Microsoft docs.

williamjcm commented 2 years ago

In the case of Circle, dunno what happened. I never connected it to my account, and I made sure to fetch the latest master before doing any changes and pushing.

mosra commented 2 years ago

Not sure if that helps, but there's https://app.circleci.com/pipelines/github/williamjcm/corrade, so your account seems to be connected somehow.

williamjcm commented 2 years ago

I checked my emails, and looks like it, considering I did log in to it at some point.

Also, in my dashboard, my fork isn't configured in any way, but it's weird that it would also prevent building PRs. 🤔

Eh, I'll try setting it up. It's not like it'll use all of my free minutes anyway.

mosra commented 2 years ago

Encountered a similar case with mosra/magnum-plugins#121 lately, but there it was at least failing with an error message. Here it's completely silent.

For OSS repos (if you enable the setting) there's plenty of free minutes, even on the 100s of build jobs I have I only use half of the monthly allowance :D

williamjcm commented 2 years ago

Orb codecov/codecov@1.1.1 not loaded. To use this orb, an organization admin must opt-in to using third party orbs in Organization Security settings.

I get that even though the setting is enabled. Welp... :/

For OSS repos (if you enable the setting) there's plenty of free minutes, even on the 100s of build jobs I have I only use half of the monthly allowance :D

Apparently, that was auto-enabled when I added the project.

mosra commented 2 years ago

Eh, fuck it then. Pushing the commit onto next and testing there.

mosra commented 2 years ago

I'm getting this:

79:   FAIL [043] moveDestinationNoPermission() at ..\src\Corrade\Utility\Test\PathTest.cpp:1185
79:         String out.str() isn't prefixed with formatString("Utility::Path::move(): can't move {} to {}: error 13 (", from, to), actual is
79:         Utility::Path::move(): can't move C:/projects/corrade/src/Corrade/Utility/Test/PathTestFiles/dir/dummy to C:/Program Files/WindowsApps/writtenFile: error 2 (No such file or directory)
79: 
79:         but expected prefix
79:         Utility::Path::move(): can't move C:/projects/corrade/src/Corrade/Utility/Test/PathTestFiles/dir/dummy to C:/Program Files/WindowsApps/writtenFile: error 13 (

"No such file or directory" sounds weird to me, so just wanted to confirm that you have the same behavior locally, and that it's not specific to the CI images (such as not having the C:/Program Files/WindowsApps directory). Actually that's wrong, and the cause is that it's printing the errno error (stale one, from the previous test case) and not GetLastError(). Fixing that.

mosra commented 2 years ago

Merged as 1584eeaadf7fbe812e354da61fb387abf113e920, with some extra changes to print GetLastError() instead of errno, and disabling the whole implementation on UWP, similarly as with almost all other Utility::Path APIs. Not sure what one is supposed to use there, the "Unix" _wrename() API was available but the Win32 API not.

Nevertheless, thanks for bringing it 90% of the way! :+1: