Skip to content

Fix safe_copy(..., preserve_meta=False)#4938

Open
samsrabin wants to merge 11 commits intoESMCI:masterfrom
samsrabin:safe_copy-202602
Open

Fix safe_copy(..., preserve_meta=False)#4938
samsrabin wants to merge 11 commits intoESMCI:masterfrom
samsrabin:safe_copy-202602

Conversation

@samsrabin
Copy link
Copy Markdown
Contributor

@samsrabin samsrabin commented Feb 16, 2026

Description

This changes safe_copy(..., preserve_meta=False) to use shutil.copyfile instead of shutil.copy when safe_copy is called on a file directly. This fixes the bug where preserve_meta=False incorrectly preserved permissions.

The first set of test runs will fail because I'm pushing without the bugfix. Once those tests are complete, I'll push the fix and they should pass.

I added the tests in a new file, but they could be moved to test_unit_utils.py instead.

Checklist

  • My code follows the style guidelines of this project (black formatting)
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • I have added tests that exercise my feature/fix and existing tests continue to pass
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding additions and changes to the documentation

The failing test is test_safe_copy_preserve_meta_false: According to the docstring, we do not expect permissions to be preserved when using safe_copy() on a file, but they are.
@samsrabin samsrabin self-assigned this Feb 16, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 28.35%. Comparing base (fc2de6d) to head (4524f13).
⚠️ Report is 67 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4938      +/-   ##
==========================================
- Coverage   31.37%   28.35%   -3.03%     
==========================================
  Files         264      262       -2     
  Lines       38899    38362     -537     
  Branches     8260     8116     -144     
==========================================
- Hits        12204    10876    -1328     
- Misses      25457    26245     +788     
- Partials     1238     1241       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@jedwards4b jedwards4b left a comment

Choose a reason for hiding this comment

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

Thank you!

@samsrabin
Copy link
Copy Markdown
Contributor Author

Hmm... not sure what that failing CESM system test is about.

@samsrabin
Copy link
Copy Markdown
Contributor Author

samsrabin commented Feb 16, 2026

Very strange; it works on Derecho. Resubmitting.

@samsrabin
Copy link
Copy Markdown
Contributor Author

That did it! All tests have now passed.

@samsrabin
Copy link
Copy Markdown
Contributor Author

In my testing on Izumi, all CTSM baseline files now have at least rw-r--r--. Unfortunately they're not all group-writable, but I can file that as a separate issue.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes safe_copy(..., preserve_meta=False) so that direct file copies do not accidentally preserve permissions (allowing SharedArea’s umask behavior to take effect as intended).

Changes:

  • Switch direct file copy behavior for preserve_meta=False from shutil.copy to shutil.copyfile (avoid copying mode bits).
  • Add a new unit test module covering safe_copy behavior across preserve_meta settings and within/outside SharedArea.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
CIME/utils.py Updates safe_copy to avoid preserving permissions when preserve_meta=False for new direct-file copies.
CIME/tests/test_unit_safe_copy.py Adds unit tests intended to catch permission/umask regressions and validate SharedArea interactions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@samsrabin
Copy link
Copy Markdown
Contributor Author

@rljacob Could you request a Claude re-review?

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…se behavior.

* Ensure that test descriptions match behavior.
* Ensure that calling umask is different from both src and tgt files.
@samsrabin
Copy link
Copy Markdown
Contributor Author

@jedwards4b @rljacob I think this is ready to go!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

safe_copy(..., preserve_meta=False) does preserve perms

3 participants