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

Don't try to overwrite an existing file if the contents are identical #4604

Closed
wants to merge 3 commits into from

Conversation

ocaisa
Copy link
Member

@ocaisa ocaisa commented Aug 6, 2024

No description provided.

@ocaisa ocaisa changed the base branch from develop to 5.0.x August 9, 2024 13:08
@ocaisa ocaisa closed this Aug 9, 2024
@ocaisa ocaisa reopened this Aug 9, 2024
@ocaisa ocaisa changed the title Don't overwrite an existing file if the contents are identical Don't try to overwrite an existing file if the contents are identical Aug 10, 2024
@ocaisa ocaisa added the EasyBuild-5.0 EasyBuild 5.0 label Aug 10, 2024
@hattom
Copy link
Contributor

hattom commented Aug 13, 2024

Note that file permissions are not compared as part of filecmp.cmp:

> ls -althr file?
-rwxr-xr-x 1 thay grp-tok-ipp 12 Aug 13 13:15 file1
-rw-r--r-- 1 thay grp-tok-ipp 12 Aug 13 13:15 file2
> cat test_cmp.py 
#!/usr/bin/env python3
import filecmp
print(filecmp.cmp("file1", "file2"))
> python test_cmp.py 
True

Could this cause problems?

@boegel boegel added this to the 5.0 milestone Aug 13, 2024
@boegel
Copy link
Member

boegel commented Aug 13, 2024

Note that file permissions are not compared as part of filecmp.cmp:

> ls -althr file?
-rwxr-xr-x 1 thay grp-tok-ipp 12 Aug 13 13:15 file1
-rw-r--r-- 1 thay grp-tok-ipp 12 Aug 13 13:15 file2
> cat test_cmp.py 
#!/usr/bin/env python3
import filecmp
print(filecmp.cmp("file1", "file2"))
> python test_cmp.py 
True

Could this cause problems?

This is a very good point imho, could result in unintended side effects/breakage?

@@ -2385,6 +2386,9 @@ def copy_file(path, target_path, force_in_dry_run=False):
target_exists = os.path.exists(target_path)
if target_exists and path_exists and os.path.samefile(path, target_path):
_log.debug("Not copying %s to %s since files are identical", path, target_path)
# Don't copy them if they are already considered the same
elif target_exists and path_exists and os.path.isfile(target_path) and filecmp.cmp(path, target_path):
Copy link
Member

Choose a reason for hiding this comment

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

At the very least we should also check whether file permissions are the same as well before skipping the actual copy operation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the permissions may have been changed later on by postinstallcmds or hooks or the normal EB change-permission step and thus permission differences should not be considered if the content is actually the same.

@ocaisa
Copy link
Member Author

ocaisa commented Sep 16, 2024

Closing this in favour of #4642 which I think is the real problem

@ocaisa ocaisa closed this Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants