-
Notifications
You must be signed in to change notification settings - Fork 0
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
Upgrade from datalad core as of 0.18.1-24-gabc454d4f #3
base: master
Are you sure you want to change the base?
Conversation
* datalad-core-vanilla: Copy of buildsupport from datalad 0.18.1-24-gabc454d4f Conflicts: setup.py - datalad core removed various pieces...
from setuptools import Command, DistutilsOptionError | ||
from setuptools.config import read_configuration | ||
|
||
import setuptools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this a duplicate import? (see lines 20 and following)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see one usage, in line 306; if Version(setuptools.__version__) >= Version('38.6.0'):
(that being said, setuptools 38.6.0 is March 2018, so positively ancient)
try: | ||
from importlib.metadata import version as importlib_version | ||
except ImportError: | ||
# TODO - remove whenever python >= 3.8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bearing the official metadata tag and ci upgrade, Python 3.7 is EOL and we should switch to require Py>=3.8
I completely support this, it would be nice to have something unified. A different reason is a pending deprecation in setup_tools which is still being used in the extension template (and all extensions). A part of this code relies on setuptool's With setuptools > 61.0.0,
The transitional backwards compatible API |
not yet sure if correctly resolved merge conflicts... might need to redo -- I might also check it on datalad-extension-template but ideally it should also get adopted by datalad-core itself.
Here is the diff I see from core after merge conflict resolution via `git diff HEAD^2 `
and here what I think it was before via `git diff 343b9a0..HEAD^`
difference was large because of fd99b4e which made this repo specificly applicable only for extensions, but then "why bother" if not to reuse with datalad core itself... so ideally we should indeed minimize the diff and make this repo used across core and extensions