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

CleanDirectory does not clean readonly files. #1317

Closed
frozenskys opened this issue Oct 25, 2016 · 16 comments · Fixed by #4077
Closed

CleanDirectory does not clean readonly files. #1317

frozenskys opened this issue Oct 25, 2016 · 16 comments · Fixed by #4077
Assignees
Milestone

Comments

@frozenskys
Copy link
Contributor

What You Are Seeing?

Trying to run CleanDirectory() on a folder that has had some readonly files copied into it using CopyFiles() The issue is that CleanDirectory() fails with access denied on the read only files.

What is Expected?

CleanDirectory() deletes the files even if they are readonly.

What version of Cake are you using?

0.16.2

Are you running on a 32 or 64 bit system?

x64

What environment are you running on? Windows? Linux? Mac?

Windows

Are you running on a CI Server? If so, which one?

N\A

How Did You Get This To Happen? (Steps to Reproduce)

Add some readonly xsd files to ./Documents/Schemas/ and run the following cake file twice...

Task("Default")
    .Does(() =>
    {
            var schemaFolder = Argument("schemaFolder", "./artifacts/schemas");
            EnsureDirectoryExists(schemaFolder);
            CleanDirectory(schemaFolder);
            var files = GetFiles("./Documents/Schemas/*.xsd");
            CopyFiles(files, schemaFolder);
        });

RunTarget("Default");

Output Log

First Run

Preparing to run build script...
Running build script...
Analyzing build script...
Processing build script...
Compiling build script...

========================================
Default
========================================
Executing task: Default
Creating directory C:/Source/Main/artifacts/schemas
Cleaning directory C:/Source/Main/artifacts/schemas
Copying file main.xsd to C:/Source/Main/artifacts/schemas/main.xsd
Copying file sub.xsd to C:/Source/Main/artifacts/schemas/sub.xsd
Finished executing task: Default

Task                          Duration            
--------------------------------------------------
Default                       00:00:00.1268127    
--------------------------------------------------
Total:                        00:00:00.1268127   

Second Run

Preparing to run build script...
Running build script...
Analyzing build script...
Processing build script...
Compiling build script...

========================================
Default
========================================
Executing task: Default
Cleaning directory C:/Source/Main/artifacts/schemas

An Error occured when executing task 'default'.
Error: Access to the path 'C:/Source/Main/artifacts/schemas/main.xsd' is denied.
@nrjohnstone
Copy link

Not sure if it should be modified to do this... Surely it would be better if you want this behavior to iterate over the files/dirs first and remove the readonly attribute?

@devlead
Copy link
Member

devlead commented Oct 26, 2016

This is the defaullt behavior in .NET when deleteing files. One option could be to add an overload that has an force option so it's explicit intent to change this default behavior.

Meanwhile resetting attributes can be done thru

FilePath filePath = MakeAbsolute(File("./readonly.txt"));
System.IO.File.SetAttributes(filePath.FullPath, FileAttributes.Normal);

or

FilePath filePath = MakeAbsolute(File("./readonly.txt"));
var fileInfo = new System.IO.FileInfo(filePath.FullPath);
//thru IsReadOnly property
fileInfo.IsReadOnly = false;
// alternatively thru Attributes property
fileInfo.Attributes = FileAttributes.Normal;

@frozenskys
Copy link
Contributor Author

@nrjohnstone I agree, however CleanDirectory to me implies forcing a delete - maybe a small change to the docs to describe the behavior would help?

@devlead I'm not sure I'm keen on the overload idea as you'd probably end up overloading all the current methods, but I agree it's a bad idea to change the default behavior

Maybe a new alias something like

SetFileAttribute(FilePath, ​FileAttributes)​
SetFileAttribute(string, ​FileAttributes)​
SetFileAttribute(IEnumerable<string>, ​FileAttributes)​
SetFileAttribute(IEnumerable<FilePath>, ​FileAttributes)​

would be more flexible ?

@WrathZA
Copy link

WrathZA commented Nov 23, 2016

+1

I'm trying to make my builds idempotent and when cloning a repo from git, often the .git directory contains read only files which causes DeleteDirectory or CleanDirectory to fail.

What about adding a Force boolean argument (defaulted to false) to Clean/Delete Directory? I'd be happy to contribute.

@frozenskys
Copy link
Contributor Author

@devlead @WrathZA @nrjohnstone I have some time in the next week or so to work on this if we can decide the approach and still feel it's worth doing...

@mcvavalanche
Copy link

this would be indeed most useful to clean up a git folder. I would expect it as default behavior.

@dave600b
Copy link

Hi, this is the same for files in TFS. I'm trying to copy around some files to include in my build artifact but my clean is failing due to readonly files stopping a delete / clean.

@nbarbettini
Copy link

I ran into this in the context of cleaning up a git folder as well.

Instead of adding overloads to CleanDirectory, does it make sense to add a method like SetFileAttributes(globbingPattern, FileAttributes) to a package like Cake.FileHelpers or similar?

@benwinding
Copy link

One of the main reasons we're using a build script is to pull down git repositories.

This feature would be useful, I'm currently using the System.IO features to remove the git cloned folder.

@bjorkstromm
Copy link
Member

Related to #1564

Now that #1574 is merged, IFile will support changing file attributes in v0.22. This means we could have a overload which has a force option (just like we did with DeleteDirectory in #1574)

@definesfineout
Copy link

I'd like to take a crack at this if nobody else has started it.

@augustoproiete
Copy link
Member

augustoproiete commented Oct 1, 2021

@FrankRay78
Copy link
Contributor

How are you getting on with this @definesfineout ? I'm happy to take over if you won't be able to complete it.

@FrankRay78
Copy link
Contributor

I have now opened PR #4077 that fully addresses this issue - please review if you get a chance.

I will work to address any matters up until this change is successfully merged.

I note this issue was opened Oct 25, 2016 which is a long time between drinks, but equally I hope it was worth the wait 😊

@augustoproiete
Copy link
Member

Awesome. Thanks @FrankRay78 !

@augustoproiete augustoproiete linked a pull request Dec 4, 2022 that will close this issue
@devlead devlead removed this from the Next Minor Candidate milestone Dec 14, 2022
@devlead devlead added this to the v3.1.0 milestone Dec 14, 2022
@cake-build-bot
Copy link

🎉 This issue has been resolved in version v3.1.0 🎉

The release is available on:

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.