Skip to content

don't use FileExistsError in mkdir function (doesn't exist in Python 2.7)#4328

Merged
boegel merged 4 commits intoeasybuilders:developfrom
Flamefire:makedirs
Aug 16, 2023
Merged

don't use FileExistsError in mkdir function (doesn't exist in Python 2.7)#4328
boegel merged 4 commits intoeasybuilders:developfrom
Flamefire:makedirs

Conversation

@Flamefire
Copy link
Copy Markdown
Contributor

@Flamefire Flamefire commented Aug 15, 2023

Follow-up to #4300 which introduced a Python3 class

Fix by introducing a Python2 implementation and using that.

@Flamefire
Add makedirs(..., exits_ok) for Python2
886d7bf
@Flamefire
Don't rely on errno
22def64 
Same as the Python3 implementation which has a comment:
> Cannot rely on checking for EEXIST, since the operating system could give priority to other errors like EACCES or EROFS
@Flamefire
Remove use of Python3 FileExistsError
2048143 
Use the new py2vpy3.makedirs compat layer.
boegel
boegel previously approved these changes Aug 15, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel boegel added the bug fix label Aug 15, 2023
@boegel boegel added this to the next release (4.8.1?) milestone Aug 15, 2023
@boegel
Copy link
Copy Markdown
Member

boegel commented Aug 15, 2023 •
edited
Loading

@Flamefire Hmm, new test fails:

Traceback (most recent call last):
  File "/tmp/runner/e377429f58b1fec22562d603da05e3480ca0cfff/lib/python3.6/site-packages/test/framework/filetools.py", line 3399, in test_compat_makedirs
    self.assertErrorRegex(Exception, os.path.basename(name), py2vs3.makedirs, name)
  File "/tmp/runner/e377429f58b1fec22562d603da05e3480ca0cfff/lib/python3.6/site-packages/easybuild/base/testing.py", line 178, in assertErrorRegex
    self.assertTrue(regex.search(msg), "Pattern '%s' is found in '%s'" % (regex.pattern, msg))
AssertionError: None is not true : Pattern 'folder' is found in '17'

edit: fixed in 40f9fe7

@boegel boegel changed the title Remove use of Python3 FileExistsError Remove use of Python3 FileExistsError in mkdir function Aug 15, 2023
@boegel
use liberal pattern to check whether makedirs wrapper raises an excep…
40f9fe7 
…tion if directory already exists, so the check passes with both Python 2 and 3
@easybuilders easybuilders deleted a comment from boegelbot Aug 15, 2023
@boegel
Copy link
Copy Markdown
Member

boegel commented Aug 15, 2023

We overlooked this because we stopped running the tests on top of Python 2 in #4267, ugh...

ERROR: test_mkdir (__main__.FileToolsTest)
Test mkdir function.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/arcanine/scratch/gent/vo/000/gvo00002/vsc40023/easybuild/easybuild-framework/test/framework/filetools.py", line 631, in test_mkdir
    check_mkdir(barfoodir, error="Failed.*No such file or directory")
  File "/arcanine/scratch/gent/vo/000/gvo00002/vsc40023/easybuild/easybuild-framework/test/framework/filetools.py", line 623, in check_mkdir
    self.assertErrorRegex(EasyBuildError, error, ft.mkdir, path, **kwargs)
  File "easybuild/base/testing.py", line 170, in assertErrorRegex
    call(*args, **kwargs)
  File "easybuild/tools/filetools.py", line 1924, in mkdir
    except FileExistsError as err:
NameError: global name 'FileExistsError' is not defined

@Flamefire
Copy link
Copy Markdown
Contributor Author

Flamefire commented Aug 16, 2023

@Flamefire Hmm, new test fails:

Traceback (most recent call last):
  File "/tmp/runner/e377429f58b1fec22562d603da05e3480ca0cfff/lib/python3.6/site-packages/test/framework/filetools.py", line 3399, in test_compat_makedirs
    self.assertErrorRegex(Exception, os.path.basename(name), py2vs3.makedirs, name)
  File "/tmp/runner/e377429f58b1fec22562d603da05e3480ca0cfff/lib/python3.6/site-packages/easybuild/base/testing.py", line 178, in assertErrorRegex
    self.assertTrue(regex.search(msg), "Pattern '%s' is found in '%s'" % (regex.pattern, msg))
AssertionError: None is not true : Pattern 'folder' is found in '17'

edit: fixed in 40f9fe7

Your fix works, thanks! For reference: "17" is EEXISTS, strange that the name isn't always included.

@boegel boegel mentioned this pull request Aug 16, 2023
Merged
@boegel boegel changed the title Remove use of Python3 FileExistsError in mkdir function don't use FileExistsError in mkdir function (doesn't exist in Python 2.7) Aug 16, 2023
@boegel
Copy link
Copy Markdown
Member

boegel commented Aug 16, 2023

I've opened #4333 to bring back running the test suite with Python 2.7...

@boegel boegel merged commit 46654a6 into easybuilders:develop Aug 16, 2023
@Flamefire Flamefire deleted the makedirs branch August 22, 2023 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@boegel boegel boegel approved these changes

Assignees

No one assigned

Labels

bug fix

Projects

None yet

Milestone

4.8.1

Development

Successfully merging this pull request may close these issues.

2 participants

@Flamefire @boegel