Consolidate builder into an Invoke Task #1596

Merged
merged 2 commits into from Feb 27, 2014

Conversation

Projects
None yet
3 participants
@dstufft
Member

dstufft commented Feb 26, 2014

This moves contrib/builder-installer into an invoke task which can be executed as invoke generate.installer. It also fixes a small thing where we were using get-pip.py as a temporary staging file.

dstufft added a commit that referenced this pull request Feb 27, 2014

Merge pull request #1596 from dstufft/consolidate-builder
Consolidate builder into an Invoke Task

@dstufft dstufft merged commit 2795578 into pypa:develop Feb 27, 2014

1 check passed

default The Travis CI build passed
Details

@dstufft dstufft deleted the dstufft:consolidate-builder branch Feb 27, 2014

@pfmoore

This comment has been minimized.

Show comment Hide comment
@pfmoore

pfmoore Feb 27, 2014

Member

This does bother me somewhat more than making the authors generation an invoke task, as I do on occasion run build-installer manually (for testing, making a dev copy, etc).

Can we not preserve the ability to run build-installer without needing invoke (for Windows users)? Does it have to be an all-or-nothing change?

Member

pfmoore commented Feb 27, 2014

This does bother me somewhat more than making the authors generation an invoke task, as I do on occasion run build-installer manually (for testing, making a dev copy, etc).

Can we not preserve the ability to run build-installer without needing invoke (for Windows users)? Does it have to be an all-or-nothing change?

@pfmoore

This comment has been minimized.

Show comment Hide comment
@pfmoore

pfmoore Feb 27, 2014

Member

While I don't want to get into a "competing task runners" debate, can I ask for some clarification on how invoke is impacting this code? From what I see:

  1. The functions implementing the tasks are decorated with @invoke.task.
  2. authors() runs git using invoke.run rather than (say) subprocess.check_call
  3. There are new print statements added to log progress (which is essentially irrelevant other than in terms of the format of the message).

It seems to me that invoke has a pretty minimal footprint, and it wouldn't be hard to make the code compatible with running outside of invoke:

Wrap the use of invoke in a try..except (or move the driver functions - see below - into a separate file.

Have the functions and the tasks separate:

    def authors_impl():
        ...
    authors = invoke.task(authors_impl)

Then people without invoke could call authors_impl direct from a driver script.

I have no idea what invoke.run gains over subprocess.check_output (given that we don't use the pty support) but if it's important to not use the latter when running via invoke, we could factor out the subprocess runner to use.

I'm willing to make these changes (someone should justify the use of invoke.run or I'll probably just replace it with subprocess) but only if there's agreement that future additions will follow the same pattern. If we don't want to go down this route, can we just factor out the installer task so it remains usable without invoke?

(I'm still looking into doit as an alternative to invoke for task running. If someone can list the key benefits to pip of invoke, that'll help me know what's important to replicate).

Member

pfmoore commented Feb 27, 2014

While I don't want to get into a "competing task runners" debate, can I ask for some clarification on how invoke is impacting this code? From what I see:

  1. The functions implementing the tasks are decorated with @invoke.task.
  2. authors() runs git using invoke.run rather than (say) subprocess.check_call
  3. There are new print statements added to log progress (which is essentially irrelevant other than in terms of the format of the message).

It seems to me that invoke has a pretty minimal footprint, and it wouldn't be hard to make the code compatible with running outside of invoke:

Wrap the use of invoke in a try..except (or move the driver functions - see below - into a separate file.

Have the functions and the tasks separate:

    def authors_impl():
        ...
    authors = invoke.task(authors_impl)

Then people without invoke could call authors_impl direct from a driver script.

I have no idea what invoke.run gains over subprocess.check_output (given that we don't use the pty support) but if it's important to not use the latter when running via invoke, we could factor out the subprocess runner to use.

I'm willing to make these changes (someone should justify the use of invoke.run or I'll probably just replace it with subprocess) but only if there's agreement that future additions will follow the same pattern. If we don't want to go down this route, can we just factor out the installer task so it remains usable without invoke?

(I'm still looking into doit as an alternative to invoke for task running. If someone can list the key benefits to pip of invoke, that'll help me know what's important to replicate).

@Ivoz

This comment has been minimized.

Show comment Hide comment
@Ivoz

Ivoz Feb 27, 2014

Member

I'd prefer to keep on using invoke and make it windows compatible. envoy could be a good candidate to replace invoke's current use of pexpect.

Member

Ivoz commented Feb 27, 2014

I'd prefer to keep on using invoke and make it windows compatible. envoy could be a good candidate to replace invoke's current use of pexpect.

@dstufft

This comment has been minimized.

Show comment Hide comment
@dstufft

dstufft Feb 27, 2014

Member

Sorry if maybe I got a little overzealous :) Mostly I'm trying to centralize stuff and get a single entry for this stuff that can be a little more standardized.

Besides pty support I think the only thing invoke.run() brings to the table is a nicer API that is more unified. We probably don't need pty support (I can't think of anything we'd want to work with that would need a pty) and certainly if we find something that we do want to work with that requires a pty we can revisit the use of invoke.run()vs subprocess.

As far as invoke vs something else, I admit that I didn't really bother to look around. Invoke is what I use for other projects and I'm friends with the maintainer/creator so I just reached for my standard tool. I would not be entirely opposed to switching as I think the benefit of getting all of these into one central location with one central method of calling them is still there but if Windows support can be added to invoke I'd prefer that over switching. I'm also OK with providing a contrib/build-installer that just reaches into the tasks module and calls the function.

I don't know how big of a deal adding Windows support to invoke would be (to be honest I'm not sure of the differences that would affect something like that at all) and I don't want to "volunteer" or invent busy work for you @pfmoore. What do you think is the best path forward here?

Member

dstufft commented Feb 27, 2014

Sorry if maybe I got a little overzealous :) Mostly I'm trying to centralize stuff and get a single entry for this stuff that can be a little more standardized.

Besides pty support I think the only thing invoke.run() brings to the table is a nicer API that is more unified. We probably don't need pty support (I can't think of anything we'd want to work with that would need a pty) and certainly if we find something that we do want to work with that requires a pty we can revisit the use of invoke.run()vs subprocess.

As far as invoke vs something else, I admit that I didn't really bother to look around. Invoke is what I use for other projects and I'm friends with the maintainer/creator so I just reached for my standard tool. I would not be entirely opposed to switching as I think the benefit of getting all of these into one central location with one central method of calling them is still there but if Windows support can be added to invoke I'd prefer that over switching. I'm also OK with providing a contrib/build-installer that just reaches into the tasks module and calls the function.

I don't know how big of a deal adding Windows support to invoke would be (to be honest I'm not sure of the differences that would affect something like that at all) and I don't want to "volunteer" or invent busy work for you @pfmoore. What do you think is the best path forward here?

@pfmoore

This comment has been minimized.

Show comment Hide comment
@pfmoore

pfmoore Feb 27, 2014

Member

I'm happy to help with porting invoke to Windows. (BTW, @Ivoz I don't think we need envoy - the only use of pexpect and pty in invoke is when the pty=True flag is set and I'd be perfectly happy just to raise an error in that case).

My main issue with invoke is that when I went and looked at the github project (which is annoyingly hard to find from the PyPI page, btw!) there's a month-old patch to provide Windows support. I know a month isn't long, and it's a self-described "quick and dirty" job, but it's about where I'd have started (the poster picked up a few points I'd not spotted). But other than a ping and a "me, too" message, there's no comment from anyone. I've added a comment saying I'm willing to help with Windows support, but I'm concerned whether Windows patches will get attention (let alone whether they'll get applied!)

@dstufft if you know the maintainer, maybe you could point him at pyinvoke/invoke#113? I'd be happy to have a discussion with him under that issue about how best to add Windows support while still keeping in line with how he wants the project to go. I can appreciate that if he's not a Windows user he's taking patches somewhat on trust, let's work out how we can make that OK for him. (Heck, if he wants, I can be the "annoying Windows guy" on invoke just as easily as I currently am for pip :-))

Member

pfmoore commented Feb 27, 2014

I'm happy to help with porting invoke to Windows. (BTW, @Ivoz I don't think we need envoy - the only use of pexpect and pty in invoke is when the pty=True flag is set and I'd be perfectly happy just to raise an error in that case).

My main issue with invoke is that when I went and looked at the github project (which is annoyingly hard to find from the PyPI page, btw!) there's a month-old patch to provide Windows support. I know a month isn't long, and it's a self-described "quick and dirty" job, but it's about where I'd have started (the poster picked up a few points I'd not spotted). But other than a ping and a "me, too" message, there's no comment from anyone. I've added a comment saying I'm willing to help with Windows support, but I'm concerned whether Windows patches will get attention (let alone whether they'll get applied!)

@dstufft if you know the maintainer, maybe you could point him at pyinvoke/invoke#113? I'd be happy to have a discussion with him under that issue about how best to add Windows support while still keeping in line with how he wants the project to go. I can appreciate that if he's not a Windows user he's taking patches somewhat on trust, let's work out how we can make that OK for him. (Heck, if he wants, I can be the "annoying Windows guy" on invoke just as easily as I currently am for pip :-))

@dstufft

This comment has been minimized.

Show comment Hide comment
@dstufft

dstufft Feb 27, 2014

Member

Sure I can poke him :) He's on California time so probably not awake yet :)

Member

dstufft commented Feb 27, 2014

Sure I can poke him :) He's on California time so probably not awake yet :)

@pfmoore

This comment has been minimized.

Show comment Hide comment
@pfmoore

pfmoore Feb 27, 2014

Member

No problem, I'm away this weekend so I'll probably not get back to this till next week anyway.

Member

pfmoore commented Feb 27, 2014

No problem, I'm away this weekend so I'll probably not get back to this till next week anyway.

@Ivoz

This comment has been minimized.

Show comment Hide comment
@Ivoz

Ivoz Feb 27, 2014

Member

@bitprophet seems to get around to things slowly. He's not completely useless though afaik :)

Member

Ivoz commented Feb 27, 2014

@bitprophet seems to get around to things slowly. He's not completely useless though afaik :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment