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.

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

don't use distutils.dir_util in copy_dir #3310

Merged
merged 4 commits into from
Apr 30, 2020

Conversation

boegel
Copy link
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
@Flamefire
Copy link
Contributor

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
Copy link
Contributor

lexming commented Apr 30, 2020

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

@boegel
Copy link
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
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
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
Contributor

lexming commented Apr 30, 2020

lexming
lexming previously approved these changes Apr 30, 2020
Copy link
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

Copy link
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

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

@lexming
Copy link
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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace usage of distutils.dir_util
3 participants