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

Astropy helpers#46

Merged
astrofrog merged 10 commits intoastropy:masterfrom
embray:astropy_helpers
May 21, 2014
Merged

Astropy helpers#46
astrofrog merged 10 commits intoastropy:masterfrom
embray:astropy_helpers

Conversation

@embray
Copy link
Member

@embray embray commented Apr 11, 2014

This PR updates the package template to use the prototype of astropy_helpers, distributed as part of the package template using the git submodule mechanism.

This PR accompanies astropy/astropy-APEs#5 which tracks the APE draft describing astropy_helpers, as well as astropy/astropy-helpers#1 which contains the initial draft implementation of the astropy_helpers package and the ah_bootstrap module, and astropy/astropy#1563 which is the analog to this PR, but for Astropy itself (that is, it converts Astropy to using astropy_helpers).

This PR should not be merged until the APE is finalized, and this PR has been updated to point the astropy_helpers submodule to the main astropy_helpers repository.

embray added 8 commits April 11, 2014 16:18
…will always copy over the latest version of ah_bootstrap (so that we can test ah_bootstrap itself), but there should still be a default copy of it in the package-template repo as well
…ludes astropy_helpers as a submodule. Yes, astropy_helpers is now a submodule of package_template which is a submodule of astropy_helpers
@astrofrog
Copy link
Member

@embray - travis is failing because the git clone is failing.

Note that this should likely be merged only after APE4 and astropy/astropy#1563 have been merged, since the clone URL will need to be updated (and the commits optionally squashed)

@mwcraig
Copy link
Member

mwcraig commented Apr 30, 2014

  • Currently, line 97 of setup.py is install_requires=['astropy'], but should, I presume, be 'astropy_helpers'? :)
  • Not sure if the initial addition of the submodule was supposed to happen automagically, but it didn't. When I manually did git submodule add https://github.com/embray/astropy_helpers.git astropy_helpers the rest worked.
  • I assume "work" in this context was supposed to mean that python setup.py install succeeded with no astropy installed...

@astrofrog
Copy link
Member

@embray - I tried this with APLpy and got:

An unexpected error occurred updating the git submodule 'astropy_helpers':
From https://github.com/astrofrog/astropy_helpers
   f852e99..010ea7e  master     -> origin/master
 * [new branch]      try-with-sphinx -> origin/try-with-sphinx
fatal: reference is not a tree: 1a4d6b1bd97ba5279634589f26e2a0c102af40f3
Unable to checkout '1a4d6b1bd97ba5279634589f26e2a0c102af40f3' in submodule path 'astropy_helpers'

when trying to setup.py install. I get the following when listing sub-modules:

$ git submodule
+f852e991725071a1dff80d7328ee4c8ec2d7a2e0 astropy_helpers (heads/master)

Then if I tried adding the submodule as @mwcraig pointed out, I get:

$ git submodule add https://github.com/embray/astropy_helpers.git astropy_helpers
'astropy_helpers' already exists in the index

Any idea what is happening?

@astrofrog
Copy link
Member

By the way, when I merged in your branch, astropy_helpers was created by default (and empty)

@mwcraig
Copy link
Member

mwcraig commented Apr 30, 2014

@astrofrog -- funny, I almost mentioned this last night but then didn't.

The underlying complaint in the second case is that there is a local copy of astropy_helpers in .git/modules (not sure how it gets there, but it did for both of us) -- that is the astropy_helpers that git is complaining about. @embray, is ah_bootstrap.py making this maybe?

Delete .git/modules/astropy_helpers, then git submodule add https://github.com/embray/astropy_helpers.git astropy_helpers should work.

Probably a good idea to check git status to make sure it doesn't know about the submodule before deleting. If git does see the submodule then do git submodule deinit astropy_helpers first.

Under no circumstances should you do git submodule deinit . -- that makes git very, very confused :)

@embray
Copy link
Member Author

embray commented Apr 30, 2014

@mwcraig No, install_requires=['astropy'] is correct. That's another one of those mis-named keywords from setuptools. Really it means "run_requires" and indicates that astropy is a runtime dependency. One should never use install_requires='astropy_helpers' by design--it is not a runtime dependency (setuptools uses "setup_requires" for that).

As for the other issues I'm not sure. I'll have to try walking through the process of setting up an affiliated package to use this myself, and document the process. I already did as much with the package-template but I think I need to do it again. I suspect the trick involves getting the exact steps for setting up the submodule right.

@astrofrog
Copy link
Member

@embray - just to be clear, all I did was take the current APLpy package, then merge in your branch, resolve conflicts, and that's it. I didn't try and set up the sub-module myself, but maybe I was supposed to?

@cdeil cdeil mentioned this pull request May 6, 2014
2 tasks
@astrofrog
Copy link
Member

@embray - I just tried with montage-wrapper and had no issues. I will try again with APLpy later this evening.

@astrofrog
Copy link
Member

@embray - I investigated more the issue with APLpy, and I think the issue was due to the fact that there were residual references to previous versions of this PR in some way (or my previous attempts to get it to work before your PR). I started from a clean PR and everything worked perfectly.

@astrofrog
Copy link
Member

@embray - given this, can we go ahead and merge all the astropy-helpers related PRs today? :)

@astrofrog
Copy link
Member

@embray - once you are ready to merge this, and once you update the submodule URL, you'll need to put the http URL for Travis to pass (the ssh clone isn't working at the moment)

@embray
Copy link
Member Author

embray commented May 13, 2014

Oh, let me try that and see if I can get the tests to pass with Travis.

@embray
Copy link
Member Author

embray commented May 13, 2014

Cool, this is passing on Travis now. The next step is to merge astropy/astropy-helpers#1 and then update this PR once more so that the submodule points to the main astropy-helpers repo.

Before I merge this PR though should I do anything with the package-template documentation to go along with it? Because the use of astropy_helpers is going to affect, somewhat, how the package-template is used. I think in general the defaults will just work, but maybe it's still worth documenting the different options pertaining to astropy-helpers?

@mwcraig
Copy link
Member

mwcraig commented May 13, 2014

👍 to a brief update of the documentation, though it could be after merging if time is short now.

@astrofrog
Copy link
Member

@embray - I agree with @mwcraig, a little documentation would be nice, but not urgent. Certainly doesn't need to hold back the 0.4 release of astropy.

@embray
Copy link
Member Author

embray commented May 16, 2014

Okay, astropy/astropy-helpers#1 has been merged so I can update this to repoint the submodule to the main astropy/astropy-helpers. Once that succeeds we can merge this and, well, we'll see how it goes.

@astrofrog
Copy link
Member

Sounds good!

@astrofrog
Copy link
Member

@embray - looks like it needs HTTP, not HTTPS

@embray
Copy link
Member Author

embray commented May 19, 2014

That makes no sense.

@embray
Copy link
Member Author

embray commented May 19, 2014

I just screwed up the URL, that's all.

@astrofrog
Copy link
Member

@embray - ah right sorry, I just assumed that because travis seemed to be asking for username/password it interpreted HTTPS as always requiring authentication.

@embray
Copy link
Member Author

embray commented May 19, 2014

Yeah, GitHub does that instead of returning a 404. This is presumably in order to protect the existence of private repositories, but it's still confusing.

Copy link
Member

Choose a reason for hiding this comment

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

@embray - just to check, is it normal that we no longer pass PACKAGENAME here?

Copy link
Member

Choose a reason for hiding this comment

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

It seems this is intentional, and works fine like this (and not with PACKAGENAME), so I think you can ignore my comment.

@astrofrog
Copy link
Member

@embray - apart from my comment above, this is good to go.

@astrofrog
Copy link
Member

@embray - actually, as per astropy/astropy#2519, we should update the submodule to ecb29056e257bbb0984d71e16d00abc697e581e2 but not to the latest commit since this breaks Sphinx builds. Could you update the submodule commit here?

@astrofrog
Copy link
Member

Ok, since this is passing, I'm going to merge and then update the astropy-helpers version in a separate pull request.

astrofrog added a commit that referenced this pull request May 21, 2014
@astrofrog astrofrog merged commit cf8b4cc into astropy:master May 21, 2014
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