Skip to content

Replaced distutils to shutil when copying files in a tree#21222

Merged
opencv-pushbot merged 1 commit intoopencv:4.xfrom
asenyaev:asen/replace_distutils_copy_tree
Dec 10, 2021
Merged

Replaced distutils to shutil when copying files in a tree#21222
opencv-pushbot merged 1 commit intoopencv:4.xfrom
asenyaev:asen/replace_distutils_copy_tree

Conversation

@asenyaev
Copy link
Copy Markdown
Contributor

@asenyaev asenyaev commented Dec 9, 2021

This PR fixes following issue #21141 for files in 4.x branch which are not located in 3.4 branch.

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

copied_dest_item = os.path.join(os.path.join(dstdir, "Modules"), item)
if os.path.exists(copied_dest_item):
try:
shutil.rmtree(copied_dest_item)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should not cleanup destination directories.
AFAIK, artifacts of different archs are merged in one location.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We just remove files which we want to add to this location, I mean overwrite. But yes, it removes files in the directory which we want to copy if it already exists. I'll fix it.

Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

gen_objc.py

This script can be tested even on Linux through gen_opencv_objc_source target.

return "/**\n " + "\n ".join(lines) + "\n */" if hasValues else ""

# copytree from shutil cannot handle an error when directory exists in Python < 3.8
def _mkdir(newdir):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added it to the function.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I mean that we could replace this function implementation with pathlib.Path.mkdir call (available since Python 3.4).

if tail:
os.mkdir(newdir)

def copytree(src, dst, symlinks=False):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe we could use https://docs.python.org/3/library/shutil.html#shutil.copytree

May be with Python 3.8 if we need dirs_exist_ok parameter.

BTW,
macosx-1 has Python 3.9
macosx-2 has Python 3.7

Copy link
Copy Markdown
Contributor Author

@asenyaev asenyaev Dec 9, 2021

Choose a reason for hiding this comment

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

Yes, but I thought dirs_exist_ok can work only with python>=3.8, which is not good for versions before 3.8.

Also, copytree cannot write to the directory, if it's already exist, because it tries to use mkdir there.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

copytree cannot write to the directory, if it's already exist

This is why I mention about dirs_exist_ok parameter.

I expecting only this change:

if sys.version_info >= (3, 8):  # Python 3.8+
    def copy_tree(src, dst):
        shutil.copytree(src, dst, dirs_exist_ok=True)
else:
    from distutils.dir_util import copy_tree

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is it a good idea to support distutils anyway? As far as I know, it will be deprecated in Python 3.12, what is okay to use this (your suggestion) workaround.

@asenyaev asenyaev marked this pull request as ready for review December 10, 2021 08:47
Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM 👍

@opencv-pushbot opencv-pushbot merged commit e3e04f5 into opencv:4.x Dec 10, 2021
@asenyaev asenyaev deleted the asen/replace_distutils_copy_tree branch December 10, 2021 16:16
@alalek alalek mentioned this pull request Dec 30, 2021
@alalek alalek mentioned this pull request Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants