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

[JENKINS-57855] read-only flag of folders on windows #4059

Merged
merged 2 commits into from
Jun 22, 2019

Conversation

mawinter69
Copy link
Contributor

@mawinter69 mawinter69 commented Jun 6, 2019

See JENKINS-57855.

Proposed changelog entries

  • Fix: reliably remove read-only flag for windows folders

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

On windows the current way of removing the read only flag is not working
for folders.
Using the Files api this can be solved.
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Looks good overall. Maybe SecurityExceptions need to be handled in the code

if (dos != null)
{
dos.setReadOnly(false);
}
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then we would need to wrap almost all File stuff in this class.

Copy link
Member

Choose a reason for hiding this comment

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

fair point

core/src/main/java/jenkins/util/io/PathRemover.java Outdated Show resolved Hide resolved
* If on Windows a folder has a read only attribute set, the file.setWritable(true) doesn't work (JENKINS-57855)
*/
DosFileAttributeView dos = Files.getFileAttributeView(path, DosFileAttributeView.class, LinkOption.NOFOLLOW_LINKS);
if (dos != null)
Copy link
Member

Choose a reason for hiding this comment

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

Is it supposed to fall through and call path.toFile().setWritable(true); in such case? Looks so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would assume that it is not necessary to call the setWritable(true) for windows.
If this fails then the setWritable would also fail I guess.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

I think it is fine and ready to go. Thanks @mawinter69 !
Just to avoid future regressions, an Windows-only unit test could be created (Assume class allows to create optional tests in JUnit)

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jun 22, 2019
@oleg-nenashev
Copy link
Member

I am going to merge it towards the next weekly if no negative feedback

@oleg-nenashev oleg-nenashev merged commit f23b93a into jenkinsci:master Jun 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants