Skip to content

Update copy_dir to include option to merge directories#3270

Merged
boegel merged 6 commits intoeasybuilders:developfrom
Darkless012:update_copy_dir
Apr 9, 2020
Merged

Update copy_dir to include option to merge directories#3270
boegel merged 6 commits intoeasybuilders:developfrom
Darkless012:update_copy_dir

Conversation

@Darkless012
Copy link
Copy Markdown
Contributor

copy_dir is using shutils.copy_tree which throws error when target directory exists.
In Python 3.8 new dirs_exist_ok parameter was added to be able to merge folders as well.

The cleanest solution was to mimic this option with distutils copy_tree which does exactly that.

This is needed for Tarball installation in Bundles, because Tarball removes the whole folder, effectively wiping previous installed bundle components. (Fix for Tarball EB will follow)

Please Squash the merge if possible.

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.

Comment thread easybuild/tools/filetools.py Outdated
Comment thread easybuild/tools/filetools.py Outdated
Comment thread easybuild/tools/filetools.py Outdated
Comment thread easybuild/tools/filetools.py Outdated
Comment thread test/framework/filetools.py Outdated
Comment thread test/framework/filetools.py Outdated
@ocaisa
Copy link
Copy Markdown
Member

ocaisa commented Apr 8, 2020

@Darkless012 Can you please take a look at easybuilders/easybuild-easyblocks#1989 that is covering similar ground with a different approach, I get the impression we will need to synchronise our approach among that and your upcoming tarball PR

@lexming
Copy link
Copy Markdown
Contributor

lexming commented Apr 8, 2020

Having the option to merge folders is a nice addition. However, I'd like to point out that it won't solve the issue with tarball easyblock as it is very unlikely that two source code trees can be merged in the same folder. I proposed to add an option to unpack the tarballs in specific subdirectories in easybuilders/easybuild-easyblocks#1989.
I think that both options are compatible and should be added to tarball easyblock as long as none of them is made default. @Darkless012 If there is anything I can do to help synchronize those changes to tarball let me know.

@Darkless012
Copy link
Copy Markdown
Contributor Author

Thank you for noticing, agreed :)
Lets discuss this in mentioned Tarball Easyblock PR.

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, thanks a lot @Darkless012!

@boegel boegel merged commit 7eaa63f into easybuilders:develop Apr 9, 2020
@boegel
Copy link
Copy Markdown
Member

boegel commented Apr 9, 2020

However, I'd like to point out that it won't solve the issue with tarball easyblock as it is very unlikely that two source code trees can be merged in the same folder.

Why is that an issue? It's say it's quite uncommon that two different software packages would include the same files, and hence problems would ensue when copying the files into an already existing directory?

This is exactly what traditional package managers do by throwing everything in /usr for example...

Am I overlooking something? (I haven't looked at easybuilders/easybuild-easyblocks#1989 in detail yet)

@lexming
Copy link
Copy Markdown
Contributor

lexming commented Apr 9, 2020

@boegel one thing is to put all executables in a common bin folder or all libs in a common lib as it can be assumed that they will have different names. But merging whole tarballs is a different matter in my opinion as it is unpredictable if files might share the same name and relative path. I see it more closely related to what is done by traditional package managers in /usr/share.

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.

4 participants