Skip to content

don't use distutils.dir_util in copy_dir#3310

Merged
lexming merged 4 commits intoeasybuilders:developfrom
boegel:no_distutils_dir_utils
Apr 30, 2020
Merged

don't use distutils.dir_util in copy_dir#3310
lexming merged 4 commits intoeasybuilders:developfrom
boegel:no_distutils_dir_utils

Conversation

@boegel
Copy link
Copy Markdown
Member

@boegel boegel commented Apr 30, 2020

fixes #3306

@boegel boegel added the bug fix label Apr 30, 2020
@boegel boegel added this to the next release (4.2.1?) milestone Apr 30, 2020
@boegel boegel requested a review from lexming April 30, 2020 08:31
@boegel boegel mentioned this pull request Apr 30, 2020
Closed
@Flamefire
Copy link
Copy Markdown
Contributor

Flamefire commented Apr 30, 2020

May I suggest not to switch implementations based on Python version? Why not use a single implementation which behaves as the Python 3.8 version always? Or a defined subset of it. I see no advantage of the usage of Python 3.8 function if a hand-rolled version must still exist. Just makes the test-surface larger

@lexming lexming mentioned this pull request Apr 30, 2020
Merged
@lexming
Copy link
Copy Markdown
Contributor

lexming commented Apr 30, 2020

Tests with this PR can be found in easybuilders/easybuild-easyblocks#2049

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Apr 30, 2020

May I suggest not to switch implementations based on Python version? Why not use a single implementation which behaves as the Python 3.8 version always? Or a defined subset of it. I see no advantage of the usage of Python 3.8 function if a hand-rolled version must still exist. Just makes the test-surface larger

The idea is that we can get rid of our own code whenever we reach only having to support Python 3.8 & newer...

I sort of see your point though.

Thoughts on this @lexming?

@lexming
Copy link
Copy Markdown
Contributor

lexming commented Apr 30, 2020

@boegel If our own implementation for Python < 3.8 works just as well in Python 3.8 and onwards, we could indeed use it indiscriminately.

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Apr 30, 2020

@lexming I reproduced your broken use case in the tests and fixed it in e086444; special handling for Python >= 3.8 removed in 3c80fef

@lexming
Copy link
Copy Markdown
Contributor

lexming commented Apr 30, 2020

New tests in easybuilders/easybuild-easyblocks#2049 (comment)

lexming
lexming previously approved these changes Apr 30, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

LGTM

@easybuilders easybuilders deleted a comment from boegelbot Apr 30, 2020
lexming
lexming approved these changes Apr 30, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@lexming lexming left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

All tests were fine, covering all current use cases (overwrite and merging) and directories with regular files and symlinks.

@lexming
Copy link
Copy Markdown
Contributor

lexming commented Apr 30, 2020

Going in, thanks @boegel !

@lexming lexming merged commit 42aaf63 into easybuilders:develop Apr 30, 2020
@boegel boegel deleted the no_distutils_dir_utils branch April 30, 2020 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lexming lexming lexming approved these changes

Assignees

No one assigned

Labels

bug fix

Projects

None yet

Milestone

4.2.1

Development

Successfully merging this pull request may close these issues.

Replace usage of distutils.dir_util

3 participants

@boegel @Flamefire @lexming