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

Updates to setup.py #5015

Closed
wants to merge 14 commits into from
Closed

Updates to setup.py #5015

wants to merge 14 commits into from

Conversation

jasmcaus
Copy link

A quick summary of what's changed:

  • a settings.ini file was created which contains all the package information (distname, maintainer, email, etc) and configurations were parsed based off these configurations
  • LONG_DESCRIPTION was refactored to be on one line (imported the io package for this)
  • Python check for >=3.6.0 was placed just after the imports to make the most logical sense

@pep8speaks
Copy link

pep8speaks commented Oct 11, 2020

Hello @jasmcaus! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 4:10: W291 trailing whitespace
Line 20:35: W291 trailing whitespace
Line 22:75: W291 trailing whitespace

Comment last updated at 2020-10-12 15:10:29 UTC

setup.py Outdated Show resolved Hide resolved
settings.ini Outdated
@@ -0,0 +1,10 @@
[DEFAULT]
Copy link
Member

Choose a reason for hiding this comment

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

Does an other package use settings.ini file for this?

Maybe we should just expand on setup.cfg

https://github.com/pypa/setuptools/blob/master/setup.cfg

Copy link
Author

Choose a reason for hiding this comment

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

I know Numpy does, so too my package Caer, but I suppose there isn't much difference using a settings.ini file over expanding over the setup.cfg. Needless, I've removed settings.ini and moved all configurations to setup.cfg.

Copy link
Member

Choose a reason for hiding this comment

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

Can we reference numpy's settings.ini or setup.cfg file by any chance? We typically follow their lead on certain things like this.

Copy link
Author

Choose a reason for hiding this comment

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

I must have got mixed up with another package. Working with far too many these days! Fastai has a settings.ini file to separate out meta information from setup.py. While they use a settings.ini file, I suggest sticking with setup.cfg (since that's where the configuration variables are defined).

setup.cfg Show resolved Hide resolved
setup.py Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@hmaarrfk
Copy link
Member

I think I've given this a thorough pass.

Generally, we are quite weary of making such drastic changes to setup.py since the changes are only really visible one we've uploaded to pypi.

This is quite costly since a release process involves many parties.

Generally, I would like to revert to the more succinct and targeted suggestion you had

#5013

Would that be OK?

@jasmcaus
Copy link
Author

I doubt this PR proffers such a drastic change to the setup. The original information remains the same; the only difference is, perhaps, where the configuration variables are located (see my previous comment). Overall, this does not (and will not) have such a huge impact, but is a neat way of, again, separating out the configuration variables.

@hmaarrfk
Copy link
Member

You are right that it is a neat way of doing something.

Unfortunately, that isn't always the goal of the project. Rather, bugs in the setup.py will only becaught by our end users once the whole release has been completed (which only happens something like twice a year).

Oddly, I'm torn by two things:

  1. I would rather not really change how setup.py is done. This has serious implications in the build and release process, something that isn't clear until release time.
  2. I think you aren't taking this PR far enough. See for example https://github.com/scipy/oldest-supported-numpy where the setup.py file is essentially blank.

If this PR is merged, I would rather converge toward 2. On the other hand, if nothing needs to be changed, I would rather move forward with the suggestions in #5013.

@jni
Copy link
Member

jni commented Oct 13, 2020

Hi @jasmcaus, thanks for contributing!

I'm mostly ok with this, but I sort of agree with @hmaarrfk that this could/should/might as well go further at this point. We can actually put the trove classifiers in the setup.cfg, and more, see e.g. napari's setup.cfg and setup.py.

I don't think this is untestable before release — we can build wheels and source distributions locally and try them out. But, since we are about to do a release, we should probably stick with the current setup until 0.18 is out (very shortly! 😬), then press on with these changes.

My proposed course of action is:

  • release 0.18
  • merge this PR more or less as is, after some local testing — I don't think it's a big deal and it takes a small step in the right direction
  • add the further changes proposed by @hmaarrfk and other projects' configs in follow-ups. Of course @jasmcaus if you get some of those changes in on this PR in the meantime, even better!

Hopefully that balances all the concerns nicely enough...?

@jasmcaus
Copy link
Author

@jni that's perfect! Will update the PR with new changes

@hmaarrfk
Copy link
Member

I'm more concerned as to how the packages will appear to users on pypi.

I think there is a pypi testing website that we should likely start using.

@jni
Copy link
Member

jni commented Oct 15, 2020

@hmaarrfk

I'm more concerned as to how the packages will appear to users on pypi.

I don't think this PR affects that? Am I missing something?

@jni
Copy link
Member

jni commented Oct 15, 2020

Also, is there something I should be worried about with https://pypi.org/project/napari/?

@jasmcaus
Copy link
Author

I highly doubt that this will affect the package metadata on PyPi. I use this same technique in my package caer and there is no difference if the configurations were hard-coded in.

@jasmcaus
Copy link
Author

Also, is there something I should be worried about with https://pypi.org/project/napari/?

Setuptools used a different way of parsing the configurations, of which I am unaware of. However, I have seen a number of popular packages use this method (which the PR is about).

@hmaarrfk
Copy link
Member

Pypi pulls the metadata from setup and setup.cfg

It's not a big deal. They should in theory be equivalent.

But in theory, theory and practice are the same.

@stefanv stefanv closed this Feb 18, 2021
@stefanv stefanv deleted the branch scikit-image:master February 18, 2021 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants