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.

Sign up for GitHub

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

Jump to bottom

Fix safeCopy to work on Linux #1691

Merged
merged 9 commits into from
Apr 25, 2024
Merged

Fix safeCopy to work on Linux #1691

merged 9 commits into from
Apr 25, 2024

Conversation

opotowsky
Copy link
Member

@opotowsky opotowsky commented Apr 19, 2024 •
edited
Loading

What is the change?

Replace shutil.copyfile and shutil.copymode with a copy (windows) or cp (linux) command. This accomplishes the same thing as before (file copy with metadata).

We are NOT taking away the fact that safeCopy overrides shutil.copy in this PR, because there are significant downstream impacts and we need to check in with other ARMI users first. That is being tracked in the ticket #1690.

Why is the change being made?

Depending on how permissions are set up, this doesn't work on all linux systems.

Checklist

  • The release notes have been updated if necessary.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in pyproject.toml.

@opotowsky opotowsky added the bug Something is wrong: Highest Priority label Apr 19, 2024
@opotowsky opotowsky requested a review from john-science April 19, 2024 00:04
@opotowsky
Copy link
Member Author

More internal testing/processes are needed before this can be merged, but it's ready for a review, at least

@john-science john-science linked an issue Apr 19, 2024 that may be closed by this pull request
Remove silent overwriting of shutil.copy in ARMI utils #1690
Open
@john-science
Copy link
Member

Yeah, let's remove the possible infinite loop of "while True" here and set a time for... I don't know, 30 or 60 minutes.

@opotowsky
Implement reviewer feedback x3 + fix os.path
1ae449a 
1. remove comment about historical use
2. implement a maxWaitTime of 30 min
3. explicitly test for windows v linux and throw warning otherwise

And, src/dst needed to be treated with os.path.abspath to make
os-indepedent.
@opotowsky opotowsky requested a review from john-science April 22, 2024 19:44
opotowsky and others added 2 commits April 22, 2024 15:46
@opotowsky
Remove the -a flag from cp.
760ce74
@opotowsky @john-science
convert from code-ish to language-ish :-)
d5dfc61 
Co-authored-by: John Stilley <1831479+john-science@users.noreply.github.com>
opotowsky and others added 3 commits April 23, 2024 12:22
@opotowsky @john-science
Change runLog warning to OS Error
74dc627 
Co-authored-by: John Stilley <1831479+john-science@users.noreply.github.com>
@opotowsky
Merge branch 'main' of https://github.com/terrapower/armi into safeco…
713a741 
…py-update
@opotowsky
Merge branch 'safecopy-update' of https://github.com/terrapower/armi …
1b7c123 
…into safecopy-update
@opotowsky opotowsky merged commit b3403a7 into main Apr 25, 2024
21 checks passed
@opotowsky opotowsky deleted the safecopy-update branch April 25, 2024 17:35
@opotowsky opotowsky mentioned this pull request May 16, 2024
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakehader jakehader jakehader left review comments

@john-science john-science john-science approved these changes

Assignees
No one assigned
Labels
bug Something is wrong: Highest Priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove silent overwriting of shutil.copy in ARMI utils
3 participants
@opotowsky @john-science @jakehader