Skip to content
This repository has been archived by the owner on Aug 11, 2023. It is now read-only.

Simplify package-template usage #41

Merged
merged 6 commits into from
Mar 25, 2014
Merged

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Mar 14, 2014

This reduces the amount of changes necessary to customize the package-template for a new affiliated package.

It basically amounts to:

  1. Updating some values in setup.cfg
  2. Moving the main source directory

The important benefit of this is not that it takes less time to set up, but that this should make it easier to pull in changes to the package-template in the future. (And in that regard, we probably want to start tagging package-template releases with each astropy release, but that's for another PR...)

@mdboom
Copy link
Contributor Author

mdboom commented Mar 14, 2014

This requires only a minor change to astropy itself, in astropy/astropy#2198, and that's only to make coverage work (which was a very recent ability to begin with).

@mdboom
Copy link
Contributor Author

mdboom commented Mar 14, 2014

An open question here is how to transition existing affiliated packages to this new approach. I don't think there's any good way to automate that. The real benefit here is for newly created affiliated packages.

@embray: You might also want to look at this in light of your work on the astropy setup tools package. I don't think the this steps on the toes of that, but there's always unintended consequences...

@embray
Copy link
Member

embray commented Mar 14, 2014

Incidentally I was just about to go back to work on finishing that up. I also had some plans in mind to simplify things from the package-template end of things, but I'll take a look at what you've done so far. Chances are we're thinking along the same lines. I actually started something like this earlier but put it aside to focus more just on getting the astropy-distutils stuff working.

@embray
Copy link
Member

embray commented Mar 14, 2014

At first I wasn't sure, but I like the idea of moving all the crud from packagename/__init__.py to _astropy_init.py. This will also make it easier to implement namespace packages should someone require that.

license = BSD
url = http://astropy.org/
edit_on_github = False
github_project = astropy/astropy
Copy link
Member

Choose a reason for hiding this comment

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

So, something I was thinking of working toward eventually was switching Astropy over to using d2to1 or something like it. As I wrote in that e-mail this morning d2to1 still needs some love and care before it can really take on the task. But once it's ready to, part of its functionality is to describe all the project's metadata in its setup.cfg, just like this. Most of the stuff we implement currently in setup.py would become setup hooks, all of which would have direct access to this metadata as a dict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually wondering that myself, as I looked into this. Maybe the thing to do for now is use the same variable names as d2to1 for the metadata (and put them in a [metadata] section), and put the other stuff (edit_on_github etc.) in another section. Then the future transition to d2to1 should be easier.

Copy link
Member

Choose a reason for hiding this comment

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

You could put the non-standard stuff under [metadata] too--there's no harm there. But yes, let's go ahead and call this section [metadata] in the eventuality that we do make such a transition (and even if we don't I think it's clearer).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Done.


git fetch template
git fetch upstream
# Make your master match the upstream master. This will destroy
Copy link
Member

Choose a reason for hiding this comment

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

Can you insert a space above this line?

@astrofrog
Copy link
Member

@mdboom - this is a great idea, and I'm kicking myself for not having done this before ;) Is it ready to merge in your view?

Once this is merged, I can send out an email to affiliated package maintainers.

@mdboom
Copy link
Contributor Author

mdboom commented Mar 19, 2014

I think this is ready to merge.

@mdboom
Copy link
Contributor Author

mdboom commented Mar 25, 2014

Any objections to merging? I have an affiliated package I'd like to start based on this 😉

os.path.join(
os.path.relpath(root, PACKAGENAME), filename))
package_info['package_data'][PACKAGENAME].extend(c_files)

Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This replaces this line in MANIFEST.in:

recursive-include packagename *.pyx *.c

Without it, the c files that are generated from pyx files are not included in the tarball. I couldn't see any way to programmatically change what's in MANIFEST.in. I should add a longer comment about this, obviously.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, so the idea is that then you are not dependent on developers keeping that in the MANIFEST.in? I wonder whether this is not something that should instead be dealt with in the sdist command? (I think we already overload it?). For now, this is fine though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't override sdist -- we just opt out of setuptools sdist and use distutils sdist instead. Oh, the joys of Python packaging 😉

I agree this isn't ideal, but I was also trying to not require changes to astropy (except for one change for coverage, which was broken in all released astropy's anyway).

Copy link
Member

Choose a reason for hiding this comment

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

No problem - once we have astropy-helpers, then I suggest we create a custom sdist command and deal with that there. For now, this is good.

Copy link
Member

Choose a reason for hiding this comment

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

Technically this can be done with a setuptools.file_finders entry point without even having to muck around with subclassing or anything of the like.

@astrofrog
Copy link
Member

@mdboom - I have no objections to merging, just a question above

@astrofrog
Copy link
Member

@mdboom - thanks! Merging :)

astrofrog added a commit that referenced this pull request Mar 25, 2014
Simplify package-template usage
@astrofrog astrofrog merged commit 1a690e1 into astropy:master Mar 25, 2014
@astrofrog
Copy link
Member

@mdboom - I really like the new infrastructure, but just one small question. In APLpy, we distinguish between the 'human'-readable package name (APLpy) and the name of the python package (aplpy). Can we add a new option in setup.cfg for the 'human'-readable name?

@mdboom
Copy link
Contributor Author

mdboom commented May 12, 2014

Where do you use one vs. the other?

@astrofrog
Copy link
Member

The only place we use the 'human-readable' name is for

setup(name='APLpy',

because that's what's used for the sdist command for example. For all other setup options the lowercase version is used.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants