Remove read-only mark on dist-newstyle when doing cabal clean on Windows

[jasagredo]

Just one commit, the rest are from the base branch #10255. This still doesn't ensure that the directory can be deleted on the first go, but at least a second cabal clean will for sure clean it.

Closes #10182 #6933

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
• The documentation has been updated, if necessary.
• Manual QA notes have been included.
• Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

Depends-On: #10225

QA

Very simple, just doing cabal clean if there is a source-repository-package

❯ cat cabal.project
packages: .

source-repository-package
  type: git
  location: https://github.com/haskell/bytestring

❯ cabal build
Cloning into 'C:\Users\Javier\code\aa\dist-newstyle\src\bytestring-ea6cd2d51c6ca30b'...
...

❯ cabal clean
C:\Users\Javier\code\aa\dist-newstyle\src\bytestring-ea6cd2d51c6ca30b\.git\objects\pack\pack-8793205445310ec514631949079f7ebb57de3771.rev: removeDirectoryRecursive:removeContentsRecursive:removePathRecursive:removeContentsRecursive:removePathRecursive:removeContentsRecursive:removePathRecursive:removeContentsRecursive:removePathRecursive:removeContentsRecursive:removePathRecursive:removeContentsRecursive:removePathRecursive:DeleteFile "\\\\?\\C:\\Users\\Javier\\code\\aa\\dist-newstyle\\src\\bytestring-ea6cd2d51c6ca30b\\.git\\objects\\pack\\pack-8793205445310ec514631949079f7ebb57de3771.rev": permission denied (Access is denied.)

The above will work with this patch.

[Mikolaj]

Thank you.

[andreabedini]

👍 I would have never thought deleting files on Windows would be so hard 😂

[geekosaur]

Are we waiting on something or should the "merge me" label be added now?

[jasagredo]

I wanted to add a test before merging.

[jasagredo]

Added test, we can merge

[jasagredo]

Retrying once doesn't seem to solve the issue. Should I:
a) remove the test, we know this thing is flaky but the fix just makes it sometimes less flaky but not always
b) retry until it succeeds
?

[ulysses4ever]

This is a driveby opinion. Removing the test would be a pity, so not (a). If there's a slight increase in CI spurious failures with the test enabled (I assume, there is, but just sanity checking), it shouldn't be enabled, so not (b). This leaves me at (c): mark the test as skipped and refer to an issue best positioned to explain the problem.

[jasagredo]

When I say the test I mean the new test that I have introduced, which just gets a source-repository-package and then cleans. It is mainly interesting in Windows, so skipping it is kind of not very interesting.

I wonder if another combinator could be created that specifies that a test is flaky, and accepts it whether it passes or not. If it passes many times hopefully someone will eventually notice that it started passing again and maybe re-enabling it can be done at that point.

Or that it specifies the output also for failing and something that says "sometimes it fails, if it fails with this output the it is ok because it is flake, if it fails with another output then there is a different error going on, if it passes then it's fine"

[jasagredo]

Depends on #10225

[geekosaur]

FWIW you can put that in the top of the pull request: https://docs.mergify.com/workflow/actions/merge/#pull-request-dependencies

[ulysses4ever]

FWIW you can put that in the top of the pull request: docs.mergify.com/workflow/actions/merge/#pull-request-dependencies

wow, that rocks!

[geekosaur]

Is this still waiting on something?

[tomsmeding]

For some reason, perhaps because this package has a custom Setup.hs, cabal build seems to want to reinitialise submodules of source-repository-packages repos very often. Normally this is annoying but not really a problem, but on Windows, due to the issue that this PR addresses, this means that the second cabal build will always fail for this package. If you have built once, you cannot build again -- you have to delete dist-newstyle first, then you can build from scratch, thank you.

Please merge this PR and release it as soon as possible. :)

[geekosaur]

@jasagredo, GitHub is complaining about conflicts and Mergify is throwing errors trying to correct them?

[ulysses4ever]

Maybe someone other than the author should manually rebase, if the author is currently busy.

[geekosaur]

I considered doing it myself, but since it's Windows I worry that I'd run into things I wouldn't know how to resolve.

[jasagredo]

I will rebase and merge this PR.

@tomsmeding keep in mind what I mentioned in the description: This is not a silver bullet. Perhaps what was here in the first version was better, namely removeDirectoryForcibly, but that still might fail due to the files being in use 🙄

[jasagredo]

I implemented a slightly stronger version (remove read-only mark + force delete the directory). Also when dealing with submodules now. @tomsmeding may you try with the cabal from this branch to see if it works better now, or point me to the package that you are having issues with so that I can play with it myself?

[tomsmeding]

@jasagredo No guarantees I'll have time (sorry, have to find a windows machine again), but to increase the chances: is there a cabal windows binary that I can easily download (CI artifacts?) that includes this PR?

[geekosaur]

https://github.com/haskell/cabal/releases/download/cabal-head/cabal-head-Windows-x86_64.tar.gz

[tomsmeding]

This works beautifully for me! Thanks for fixing (at least for my usecase); looking forward to the release.