From 046475a2afe9dc4163327f2f4146a5736c8ff3b0 Mon Sep 17 00:00:00 2001 From: Arrielle Opotowsky Date: Thu, 18 Apr 2024 18:54:28 -0500 Subject: [PATCH 1/7] Replace shutil.copyfile and shutil.copymode with a copy or cp command that gets run via os.system instead --- armi/utils/__init__.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/armi/utils/__init__.py b/armi/utils/__init__.py index 2c8cfbb1d..144170198 100644 --- a/armi/utils/__init__.py +++ b/armi/utils/__init__.py @@ -810,8 +810,14 @@ def safeCopy(src: str, dst: str) -> None: if os.path.isdir(dst): dst = os.path.join(dst, os.path.basename(src)) srcSize = os.path.getsize(src) - shutil.copyfile(src, dst) - shutil.copymode(src, dst) + # Historical note: this used to run shutil.copyfile then shutil.copymode, + # but the latter clashed with permissions on a linux system. Both copy and + # cp -a accomplish the same thing. + if os.name == "nt": + cmd = f'copy "{src}" "{dst}"' + else: + cmd = f'cp -a "{src}" "{dst}"' + os.system(cmd) while True: dstSize = os.path.getsize(dst) if srcSize == dstSize: From 68b7e0df1109e352626812f4ef0cb37978cdf89e Mon Sep 17 00:00:00 2001 From: Arrielle Opotowsky Date: Thu, 18 Apr 2024 19:02:40 -0500 Subject: [PATCH 2/7] release notes --- doc/release/0.3.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/release/0.3.rst b/doc/release/0.3.rst index bb58d291d..7df7c300e 100644 --- a/doc/release/0.3.rst +++ b/doc/release/0.3.rst @@ -37,6 +37,7 @@ API Changes Bug Fixes --------- #. Fixed four bugs with "corners up" hex grids. (`PR#1649 `_) +#. Fixed ``safeCopy`` to work on both Windows and Linux with strict permissions (`PR#1691 `_) #. TBD Quality Work From 1ae449af17891591008880d7b2f7d9e5d2ed3415 Mon Sep 17 00:00:00 2001 From: Arrielle Opotowsky Date: Mon, 22 Apr 2024 14:35:02 -0500 Subject: [PATCH 3/7] Implement reviewer feedback x3 + fix os.path 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. --- armi/utils/__init__.py | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/armi/utils/__init__.py b/armi/utils/__init__.py index 144170198..bfedbd073 100644 --- a/armi/utils/__init__.py +++ b/armi/utils/__init__.py @@ -806,23 +806,39 @@ def merge(self, *otherDictionaries) -> None: def safeCopy(src: str, dst: str) -> None: """This copy overwrites ``shutil.copy`` and checks that copy operation is truly completed before continuing.""" + # Convert files to OS-independence + src = os.path.abspath(src) + dst = os.path.abspath(dst) waitTime = 0.01 # 10 ms + maxWaitTime = 1800 # 30 min if os.path.isdir(dst): dst = os.path.join(dst, os.path.basename(src)) srcSize = os.path.getsize(src) - # Historical note: this used to run shutil.copyfile then shutil.copymode, - # but the latter clashed with permissions on a linux system. Both copy and - # cp -a accomplish the same thing. - if os.name == "nt": + # Both copy and cp -a accomplish the same thing (copy a file and its metadata) + if "win" in sys.platform: cmd = f'copy "{src}" "{dst}"' - else: + elif "linux" in sys.platform: cmd = f'cp -a "{src}" "{dst}"' + else: + runLog.warning( + f"Cannot perform ``safeCopy`` on files because ARMI only supports " + + "Linux and Windows." + ) os.system(cmd) + totalWaitTime = 0 while True: dstSize = os.path.getsize(dst) if srcSize == dstSize: break time.sleep(waitTime) + totalWaitTime += waitTime + if totalWaitTime > maxWaitTime: + runLog.warning( + f"File copy from {dst} to {src} has failed due to exceeding " + + f"``maxWaitTime`` of {maxWaitTime/60} min." + ) + break + runLog.extra("Copied {} -> {}".format(src, dst)) From f1a536e5d3438d0ae7c7695627053c176a7ce489 Mon Sep 17 00:00:00 2001 From: Arrielle Opotowsky Date: Mon, 22 Apr 2024 15:16:07 -0500 Subject: [PATCH 4/7] Run ruff and add unit test --- armi/utils/__init__.py | 2 +- armi/utils/tests/test_utils.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/armi/utils/__init__.py b/armi/utils/__init__.py index bfedbd073..3f2f6bf53 100644 --- a/armi/utils/__init__.py +++ b/armi/utils/__init__.py @@ -821,7 +821,7 @@ def safeCopy(src: str, dst: str) -> None: cmd = f'cp -a "{src}" "{dst}"' else: runLog.warning( - f"Cannot perform ``safeCopy`` on files because ARMI only supports " + "Cannot perform ``safeCopy`` on files because ARMI only supports " + "Linux and Windows." ) os.system(cmd) diff --git a/armi/utils/tests/test_utils.py b/armi/utils/tests/test_utils.py index c2131c308..7c763cc6d 100644 --- a/armi/utils/tests/test_utils.py +++ b/armi/utils/tests/test_utils.py @@ -14,6 +14,7 @@ """Testing some utility functions.""" from collections import defaultdict +import os import unittest import numpy as np @@ -21,6 +22,7 @@ from armi import utils from armi.reactor.tests.test_reactors import loadTestReactor from armi.settings.caseSettings import Settings +from armi.tests import mockRunLogs from armi.utils import ( directoryChangers, getPowerFractions, @@ -37,6 +39,7 @@ getCumulativeNodeNum, hasBurnup, codeTiming, + safeCopy, ) @@ -166,6 +169,33 @@ def testFunc(): self.assertEqual(getattr(testFunc, "__doc__"), "Test function docstring.") self.assertEqual(getattr(testFunc, "__name__"), "testFunc") + def test_safeCopy(self): + with directoryChangers.TemporaryDirectoryChanger(): + os.mkdir("dir1") + os.mkdir("dir2") + file1 = "dir1/file1.txt" + with open(file1, "w") as f: + f.write("Hello") + file2 = "dir1\\file2.txt" + with open(file2, "w") as f: + f.write("Hello2") + + with mockRunLogs.BufferLog() as mock: + # Test Linuxy file path + self.assertEqual("", mock.getStdout()) + safeCopy(file1, "dir2") + self.assertIn("Copied", mock.getStdout()) + self.assertIn("file1", mock.getStdout()) + self.assertIn("->", mock.getStdout()) + # Clean up for next safeCopy + mock.emptyStdout() + # Test Windowsy file path + self.assertEqual("", mock.getStdout()) + safeCopy(file2, "dir2") + self.assertIn("Copied", mock.getStdout()) + self.assertIn("file2", mock.getStdout()) + self.assertIn("->", mock.getStdout()) + class CyclesSettingsTests(unittest.TestCase): """ From 760ce742660a0568883e0f799d402afea9b4ecab Mon Sep 17 00:00:00 2001 From: Arrielle Opotowsky Date: Mon, 22 Apr 2024 15:46:52 -0500 Subject: [PATCH 5/7] Remove the -a flag from cp. --- armi/utils/__init__.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/armi/utils/__init__.py b/armi/utils/__init__.py index 3f2f6bf53..1676a3098 100644 --- a/armi/utils/__init__.py +++ b/armi/utils/__init__.py @@ -809,22 +809,21 @@ def safeCopy(src: str, dst: str) -> None: # Convert files to OS-independence src = os.path.abspath(src) dst = os.path.abspath(dst) - waitTime = 0.01 # 10 ms - maxWaitTime = 1800 # 30 min if os.path.isdir(dst): dst = os.path.join(dst, os.path.basename(src)) srcSize = os.path.getsize(src) - # Both copy and cp -a accomplish the same thing (copy a file and its metadata) if "win" in sys.platform: cmd = f'copy "{src}" "{dst}"' elif "linux" in sys.platform: - cmd = f'cp -a "{src}" "{dst}"' + cmd = f'cp "{src}" "{dst}"' else: runLog.warning( "Cannot perform ``safeCopy`` on files because ARMI only supports " + "Linux and Windows." ) os.system(cmd) + waitTime = 0.01 # 10 ms + maxWaitTime = 1800 # 30 min totalWaitTime = 0 while True: dstSize = os.path.getsize(dst) From d5dfc6133296b9c941fea5694dbdcc421e7d7c5a Mon Sep 17 00:00:00 2001 From: Arrielle Opotowsky Date: Mon, 22 Apr 2024 16:17:47 -0500 Subject: [PATCH 6/7] convert from code-ish to language-ish :-) Co-authored-by: John Stilley <1831479+john-science@users.noreply.github.com> --- armi/utils/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/armi/utils/__init__.py b/armi/utils/__init__.py index 1676a3098..1bd5c69ed 100644 --- a/armi/utils/__init__.py +++ b/armi/utils/__init__.py @@ -834,7 +834,7 @@ def safeCopy(src: str, dst: str) -> None: if totalWaitTime > maxWaitTime: runLog.warning( f"File copy from {dst} to {src} has failed due to exceeding " - + f"``maxWaitTime`` of {maxWaitTime/60} min." + + f"a maximum wait time of {maxWaitTime/60} minutes." ) break From 74dc6271620c5204db94db33cb489f57f4c4c932 Mon Sep 17 00:00:00 2001 From: Arrielle Opotowsky Date: Tue, 23 Apr 2024 12:22:14 -0500 Subject: [PATCH 7/7] Change runLog warning to OS Error Co-authored-by: John Stilley <1831479+john-science@users.noreply.github.com> --- armi/utils/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/armi/utils/__init__.py b/armi/utils/__init__.py index 1bd5c69ed..be6dbc234 100644 --- a/armi/utils/__init__.py +++ b/armi/utils/__init__.py @@ -817,7 +817,7 @@ def safeCopy(src: str, dst: str) -> None: elif "linux" in sys.platform: cmd = f'cp "{src}" "{dst}"' else: - runLog.warning( + raise OSError( "Cannot perform ``safeCopy`` on files because ARMI only supports " + "Linux and Windows." )