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

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

Closed

Conversation

williamjcm
Copy link
Contributor

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.

This should prevent failures when the destination already exists. Also
added a test to check the behaviour.
@mosra mosra added this to the 2022.0a milestone Jun 11, 2022
Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

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

Not sure what's up with CircleCI, why is it not building this PR. Is it possible you have some stale configuration for it set up? Either adding your fork (and marking it as "OSS" or whatever the setting is to get it built with free credits) or completely removing any CircleCI integration from this repo / your GH account should probably do the trick.

Or maybe it just needs some time to wake up, no idea... but #142 went fine, so this doesn't seem to be an outage.

Worst case I'll run the CI on the next branch myself, if you don't manage.

src/Corrade/Utility/Path.cpp Outdated Show resolved Hide resolved
@williamjcm
Copy link
Contributor Author

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
Copy link
Owner

mosra commented Jun 11, 2022

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
Copy link
Contributor Author

williamjcm commented Jun 11, 2022

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
Copy link
Owner

mosra commented Jun 11, 2022

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
Copy link
Contributor Author

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
Copy link
Owner

mosra commented Jun 11, 2022

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

@mosra
Copy link
Owner

mosra commented Jun 11, 2022

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
Copy link
Owner

mosra commented Jun 11, 2022

Merged as 1584eea, 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! 👍

@mosra mosra closed this Jun 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants