improve submodule messages / git hooks #3275
Conversation
A much less intrusive version of ipython#3274 that has effect on checkout / pull / merge, rather than setup.py. It does require a user install step, because you cannot add active hooks to a repo.
|
There is a balance here. One one hand I want us to make things easy for people running from git. OTOH, I think it is appropriate to ask people to start to become aware of how submodules work. I don't like how this PR hides submodules from everyone and requires a very manual install step (python setup.py submodule is much easier...). |
|
Oh, I wanted to ping @fperez on this PR... |
|
This PR doesn't hide anything from anyone - it adds hooks that users may use in order to make |
|
OK i understand - these are optional - I guess all I mean then is that I think that the setup.py stuff should still be there... |
|
had lunch with @fperez, and I think we are converging on a middle-ground:
This way, we never touch the git state on the user's behalf (except in the unambiguous case that |
|
Great, I think that sounds very reasonable. On Fri, May 3, 2013 at 2:13 PM, Min RK notifications@github.com wrote:
Brian E. Granger |
`setup.py anything` will halt with an informative error if the submodules are unclean. It will fetch the submodules if they are entirely absent. Nothing will happen if it is not a git repo.
shows a warning (not an error) if components are out of date.
the asctime datefmt is also configurable now
datefmt no longer necessary
|
I've also tweaked logging so that messages at level WARNING or above get the level name in the message. The one thing I don't like about this is that our |
also rprint if no parent (`ipython kernel`)
|
I've demoted the message to logging.info, and added |
| def update_submodules(repo_dir): | ||
| """update submodules in a repo""" | ||
| subprocess.Popen("git submodule init", cwd=repo_dir, shell=True) | ||
| subprocess.Popen("git submodule update", cwd=repo_dir, shell=True) |
There was a problem hiding this comment.
No need to wait here? Also, why use shell=True?
There was a problem hiding this comment.
I meant it to be check_call, not Popen, that's why there's no wait on the
end. That's fixed. The shell=True was following pattern already set by
previous git subprocess calls in setupbase, I do not know if they are
necessary - one logical reason is that it can have PATH ramifications.
On Mon, May 6, 2013 at 5:51 PM, Takafumi Arakaki
notifications@github.comwrote:
In IPython/utils/submodule.py:
- )
- status, _ = proc.communicate()
- status = status.decode("ascii")
- for line in status.splitlines():
if status.startswith('-'):return 'missing'elif status.startswith('+'):return 'unclean'
- return 'clean'
+def update_submodules(repo_dir):
- """update submodules in a repo"""
- subprocess.Popen("git submodule init", cwd=repo_dir, shell=True)
- subprocess.Popen("git submodule update", cwd=repo_dir, shell=True)
No need to wait here? Also, why use shell=True?
—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3275/files#r4105107
.
| @@ -673,11 +670,23 @@ def _signal_stop(self, sig, frame): | |||
| def _signal_info(self, sig, frame): | |||
| print self.notebook_info() | |||
|
|
|||
| def init_components(self): | |||
There was a problem hiding this comment.
I am -1 on having this logic in the bas application class. This class is used by other projects and we eventually want to separate out this our base config/app code from our repo so other projects can use it. I know you and @fperez spent time talking about the best approach here, but I am not thrilled about putting logic into the running of ipython to manage our git repo...
There was a problem hiding this comment.
this doesn't do anything if IPython is run from any circumstances other than the IPython git repo (that is, ipython_package_dir/.. must be a git repo for this to have any effect. If IPython is installed, all this does is check that components exists and returns.
There was a problem hiding this comment.
Also, the components submodule will follow this class wherever it goes, so I don't see how there could be a problem. The only way anyone could have a problem is if they copied this class to somewhere rather than subclassing it, and didn't make any appropriate changes (which would make no sense).
|
It's worth noting that tarballs/zipballs downloaded from Github don't have the components submodule, so you can't run the notebook from those any more. Possibly we should give the user some sort of hint even if it's not run from a git repository. Then again, we don't want it to complain when, say, Debian packaging moves the static files out to /usr/share. |
|
That's a good point. I pinged GitHub about that last week, and the tarballs are created via |
|
OK, I think it's fine to leave that for now, then. On 7 May 2013 22:06, Min RK notifications@github.com wrote:
|
|
This looks good, merging. |
improve submodule messages / git hooks
improve submodule messages / git hooks
A much less intrusive version of #3274 that has effect on checkout / pull / merge, rather than setup.py. It does require a user install step, because you cannot add active hooks to a repo.