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

ensure that CVS easyconfigs are included in source tarball produced by 'python setup.py sdist' #10326

Merged
merged 7 commits into from
Apr 12, 2020

Conversation

Flamefire
Copy link
Contributor

sdist by default removes CVS, RCS and various other folders
We have an EasyConfig folder named like that which got removed too.
After this we must be careful not to include e.g. .git folders and the like

Fixes #10325

sdist by default removes CVS, RCS and various other folders
We have an EasyConfig folder named like that which got removed too.
After this we must be careful not to include e.g. .git folders and the like

Fixes easybuilders#10325
@boegel boegel added the bug fix label Apr 4, 2020
@boegel boegel added this to the next release (4.2.0) milestone Apr 4, 2020
@boegel
Copy link
Member

boegel commented Apr 6, 2020

@Flamefire This results in also including the .git folder, which we should try and avoid:

$ diff -ru easybuild-easyconfigs-4.1.2.dev0_develop easybuild-easyconfigs-4.1.2.dev0
Only in easybuild-easyconfigs-4.1.2.dev0/easybuild/easyconfigs: .git
Only in easybuild-easyconfigs-4.1.2.dev0/easybuild/easyconfigs/__archive__/r: RCS
Only in easybuild-easyconfigs-4.1.2.dev0/easybuild/easyconfigs/c: CVS

Can we avoid that?

Also, we should check for this (that the CVS easyconfigs are shipped in the source tarball, but the .git folder is not) in the tests, to avoid re-introducing the issue later.

@Flamefire
Copy link
Contributor Author

Where is this .git folder even coming from? Just tried with Python 3.7 and there is neither a .git in the extracted source distribution tar nor in easybuild/easyconfigs in the repo itself

Working on a test

@Flamefire
Copy link
Contributor Author

@boegel I added a test as an extra job to GHA which checks for any .git and CVS folder. Extra job to not needlessly run this with all module tool combinations etc.

About the .git folder: I can reproduce this but it is a bug in your local tree. There is no such folder in a "good" checkout. And while there are methods to remove it, it is "hard" and will lead to warnings on "good" checkouts because the folder is not found. See https://stackoverflow.com/questions/61061233/exclude-a-directory-given-by-name-in-any-subdirectory-in-sdist why it is hard

I hence would simply ignore this because when you do a git clone ... followed by a python setup.py sdist no .git folder fill be there.

@Flamefire Flamefire force-pushed the fix_CVS_ECs_in_sdist branch 3 times, most recently from 2c1e24a to ec6f234 Compare April 6, 2020 16:26
@Flamefire
Copy link
Contributor Author

An alternative suggested from SO: Switch to setuptools instead of distutils. There you can use **/.git as well as have better support for the future. E.g. currently added zip_safe=False, is only understood by setuptools (it doesn't matter to distutils). The main difference is a created *.egg-info folder (which I don't know much about)

@boegel
Copy link
Member

boegel commented Apr 6, 2020

@Flamefire If setuptools is installed, then python setup.py sdist uses setuptools, I think?

We worked hard to get rid of setuptools as a dependency in EasyBuild 4.0, it raises all kinds of issues when you start relying on it.

@Flamefire
Copy link
Contributor Author

If setuptools is installed, then python setup.py sdist uses setuptools, I think?

Not if you don't use it:

from distutils import log
from distutils.core import setup

We worked hard to get rid of setuptools as a dependency in EasyBuild 4.0, it raises all kinds of issues when you start relying on it.

From what I understand Python world is moving to setuptools. What kind of issues? And it would be a build dependency only.

sdist by default removes CVS, RCS and various other folders
We have an EasyConfig folder named like that which got removed too.
After this we must be careful not to include e.g. .git folders and the like

Fixes easybuilders#10325
@boegel
Copy link
Member

boegel commented Apr 7, 2020

If setuptools is installed, then python setup.py sdist uses setuptools, I think?

Not if you don't use it:

from distutils import log
from distutils.core import setup

I think distutils is hijacked by setuptools (but I'm not 100% sure).

We worked hard to get rid of setuptools as a dependency in EasyBuild 4.0, it raises all kinds of issues when you start relying on it.

From what I understand Python world is moving to setuptools. What kind of issues? And it would be a build dependency only.

The Python world is actually moving to pip, but that's a different story (https://xkcd.com/1987/).

One of the biggest issue with setuptools is that on some operating systems, there's a really old version installed, sometimes so old it can't talk to PyPI anymore.

I would really like to stay away from it. My life has been significantly easier since we stopped using setuptools, and started using just distutils (except for this issue).

@Flamefire
Copy link
Contributor Author

I think distutils is hijacked by setuptools (but I'm not 100% sure).

No, setuptools uses distutils for some common stuff. But it doesn't monkeypatch it.

The Python world is actually moving to pip

That almost is the same: So, if your project uses setuptools, you must use pip to install it. (https://packaging.python.org/guides/distributing-packages-using-setuptools) and This document is being retained solely until the setuptools documentation at https://setuptools.readthedocs.io/en/latest/setuptools.html independently covers all of the relevant information currently included here. (https://docs.python.org/3/distutils/setupscript.html)

One of the biggest issue with setuptools is that on some operating systems, there's a really old version installed, sometimes so old it can't talk to PyPI anymore.

Well I proposed using get-pip for bootstrapping, which avoids this. And once you got pip all is fine. For testing I tried:

pip install --user setuptools==0.7.2
pip install easybuild-framework

Downloads EB from PyPi. When not having setuptools installed it does download too but fails to install due to pip(!) requiring setuptools.

I would really like to stay away from it. My life has been significantly easier since we stopped using setuptools, and started using just distutils (except for this issue).

Sure. Then I'd suggest to merge this as-is because it does the job well enough. Once bootstrapping uses get-pip we can reconsider using setuptools for installation/sdist. The quote above sounds to me like we should.

@boegel boegel changed the title Disable pruning in sdist ensure that CVS easyconfigs are included in source tarball produced by 'python setup.py sdist' Apr 12, 2020
@easybuilders easybuilders deleted a comment from boegelbot Apr 12, 2020
…s + also check for CVS easyconfigs with --search
@boegel
Copy link
Member

boegel commented Apr 12, 2020

Apparently I had a easybuild/easyconfigs/.git folder in my local checkout, indeed. 🤦

I made some small enhancements to the added tests in 1ad052e, this is ready to squeeze in for the EasyBuild v4.2.0 release now...

@boegel boegel merged commit 0ef40c9 into easybuilders:develop Apr 12, 2020
@Flamefire Flamefire deleted the fix_CVS_ECs_in_sdist branch April 14, 2020 09:18
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.

CVS is not included in EasyBuild releases
2 participants