This repository has been archived by the owner. It is now read-only.

Add support for the Perforce VCS client via the "p4" tool #136

Merged
merged 1 commit into from Jun 13, 2014

Conversation

Projects
None yet
5 participants
@waisbrot
Contributor

waisbrot commented Sep 18, 2013

Perforce is a version control system.

This patch uses the command-line "p4" tool to fetch dependencies stored under Perforce. It's a little odder than the other VCS clients because of Perforce's notion of a "workspace" that's tracked by the P4 server.

Show outdated Hide outdated src/rebar_deps.erl
case inet:gethostname() of
{ok,HostName} ->
HostName ++ "-" ++ os:getenv("USER") ++ "-" ++ Basename ++ "-Rebar-automated-download"
end}.

This comment has been minimized.

@tuncer

tuncer Sep 18, 2013

Contributor

Can you please re-format init_p4_settings/1 to stay below 80 chars?

@tuncer

tuncer Sep 18, 2013

Contributor

Can you please re-format init_p4_settings/1 to stay below 80 chars?

@waisbrot

This comment has been minimized.

Show comment
Hide comment
@waisbrot

waisbrot Sep 18, 2013

Contributor

Fixed long line and improved commit message

Contributor

waisbrot commented Sep 18, 2013

Fixed long line and improved commit message

@tuncer

This comment has been minimized.

Show comment
Hide comment
@tuncer

tuncer Sep 19, 2013

Contributor

Thanks, can you also please add a sample p4 dep to rebar_deps:info/2 after extending the same section in rebar.config.sample?

Contributor

tuncer commented Sep 19, 2013

Thanks, can you also please add a sample p4 dep to rebar_deps:info/2 after extending the same section in rebar.config.sample?

@dizzyd

This comment has been minimized.

Show comment
Hide comment
@dizzyd

dizzyd Sep 20, 2013

Member

I'm +1 on this after the updates @tuncer requested.

Member

dizzyd commented Sep 20, 2013

I'm +1 on this after the updates @tuncer requested.

@waisbrot

This comment has been minimized.

Show comment
Hide comment
@waisbrot

waisbrot Sep 23, 2013

Contributor

@tuncer: Are you talking about this part of rebar.config.sample? It doesn't currently have examples for anything except Git. And is it rebar_deps:info_help/1 where you'd like that example mirrored?

Contributor

waisbrot commented Sep 23, 2013

@tuncer: Are you talking about this part of rebar.config.sample? It doesn't currently have examples for anything except Git. And is it rebar_deps:info_help/1 where you'd like that example mirrored?

@tuncer

This comment has been minimized.

Show comment
Hide comment
@tuncer

tuncer Sep 23, 2013

Contributor

Yes, those are the right places. If you want to add the missing examples, your patch is very welcome. Otherwise, I will complete it.

Contributor

tuncer commented Sep 23, 2013

Yes, those are the right places. If you want to add the missing examples, your patch is very welcome. Otherwise, I will complete it.

@waisbrot

This comment has been minimized.

Show comment
Hide comment
@waisbrot

waisbrot Sep 23, 2013

Contributor

'p4' examples added

Contributor

waisbrot commented Sep 23, 2013

'p4' examples added

@tuncer

This comment has been minimized.

Show comment
Hide comment
@tuncer

tuncer Nov 24, 2013

Contributor

@waisbrot, I've completed and fixed the existing dep examples in #172. We should probably merge that first.

Are we going to merge #135, #136, or maybe both?

Contributor

tuncer commented Nov 24, 2013

@waisbrot, I've completed and fixed the existing dep examples in #172. We should probably merge that first.

Are we going to merge #135, #136, or maybe both?

@waisbrot

This comment has been minimized.

Show comment
Hide comment
@waisbrot

waisbrot Nov 27, 2013

Contributor

@tuncer, I've closed #135. Simpler to just do it one way and this seems more correct even if I don't like dealing with the p4 tool.
Once you merge #172 I will update this pull request.

Contributor

waisbrot commented Nov 27, 2013

@tuncer, I've closed #135. Simpler to just do it one way and this seems more correct even if I don't like dealing with the p4 tool.
Once you merge #172 I will update this pull request.

@tuncer

This comment has been minimized.

Show comment
Hide comment
@tuncer

tuncer Nov 28, 2013

Contributor

Works for me. cc @jaredmorrow.

Contributor

tuncer commented Nov 28, 2013

Works for me. cc @jaredmorrow.

@jaredmorrow

This comment has been minimized.

Show comment
Hide comment
@jaredmorrow

jaredmorrow Mar 5, 2014

Contributor

@waisbrot can you rebase this and we'll go ahead and merge it. Thanks.

Contributor

jaredmorrow commented Mar 5, 2014

@waisbrot can you rebase this and we'll go ahead and merge it. Thanks.

Add 'p4' (Perforce) as a dependency type
This calls the 'p4' command-line tool to checkout and sync Perforce
trees. It involves significantly more special code in Rebar than
using 'git p4', but it eliminates the indirection of
Rebar->Git->Python->Perforce
@waisbrot

This comment has been minimized.

Show comment
Hide comment
@waisbrot

waisbrot Mar 17, 2014

Contributor

Rebased and it looks OK to me.

Contributor

waisbrot commented Mar 17, 2014

Rebased and it looks OK to me.

@tuncer

This comment has been minimized.

Show comment
Hide comment
@tuncer

tuncer May 29, 2014

Contributor

Is there anything holding up a merge?

Contributor

tuncer commented May 29, 2014

Is there anything holding up a merge?

@waisbrot

This comment has been minimized.

Show comment
Hide comment
@waisbrot

waisbrot Jun 3, 2014

Contributor

Perhaps the lack of interest in this pull request means that it should be rejected. If nobody else is stuck using Perforce as a VCS, why clutter up the code base?

Contributor

waisbrot commented Jun 3, 2014

Perhaps the lack of interest in this pull request means that it should be rejected. If nobody else is stuck using Perforce as a VCS, why clutter up the code base?

@tuncer

This comment has been minimized.

Show comment
Hide comment
@tuncer

tuncer Jun 3, 2014

Contributor

@jaredmorrow ping?

Contributor

tuncer commented Jun 3, 2014

@jaredmorrow ping?

@ferd

This comment has been minimized.

Show comment
Hide comment
@ferd

ferd Jun 6, 2014

Contributor

I am not opposed to it, but I'd personally be more afraid of having to maintain it. @waisbrot how committed are you to this?

Contributor

ferd commented Jun 6, 2014

I am not opposed to it, but I'd personally be more afraid of having to maintain it. @waisbrot how committed are you to this?

@waisbrot

This comment has been minimized.

Show comment
Hide comment
@waisbrot

waisbrot Jun 7, 2014

Contributor

@ferd I don't know what that question means. How committed are you? Maybe I'm being overly sensitive, but I find it much more discouraging for you all to kick this around in a circle while shrugging than if you just said "rejected, we don't think anyone cares about Perforce."

This code works for me, for one project with one dependency inside one Perforce repository. If a bunch of people found it useful, I'd expect them to run into some problems, complain, and I'd improve it. But the vital element here is at least one person besides me who uses the patch.

Anyway, since I'm not hearing anyone else care about this, I'll keep it to myself. If someone comes along in the future and wants it, they can always look at my fork for a starting-point.

Contributor

waisbrot commented Jun 7, 2014

@ferd I don't know what that question means. How committed are you? Maybe I'm being overly sensitive, but I find it much more discouraging for you all to kick this around in a circle while shrugging than if you just said "rejected, we don't think anyone cares about Perforce."

This code works for me, for one project with one dependency inside one Perforce repository. If a bunch of people found it useful, I'd expect them to run into some problems, complain, and I'd improve it. But the vital element here is at least one person besides me who uses the patch.

Anyway, since I'm not hearing anyone else care about this, I'll keep it to myself. If someone comes along in the future and wants it, they can always look at my fork for a starting-point.

@waisbrot waisbrot closed this Jun 7, 2014

@ferd

This comment has been minimized.

Show comment
Hide comment
@ferd

ferd Jun 9, 2014

Contributor

@waisbrot the question there is that I don't have perforce, I don't have any project using it, but I'm a collaborator to rebar (since a couple of weeks ago) and therefore directly or indirectly responsible for maintaining the code and fixing bugs in it.

Therefore, this is a feature I know I personally cannot maintain. I am not closed to the idea of having perforce support in rebar, but we shouldn't be adding features we can't reasonably keep improving. If you say you're willing to fix bugs that may show up with this, then that's fine and it should be included.

If this is a feature that is being 'dumped' on us -- sorry for the lack of better terms, I mean a pull request that is sent our way and then forgotten about by its creator -- then I am not one to want to commit to maintaining it.

Does this make more sense?

Contributor

ferd commented Jun 9, 2014

@waisbrot the question there is that I don't have perforce, I don't have any project using it, but I'm a collaborator to rebar (since a couple of weeks ago) and therefore directly or indirectly responsible for maintaining the code and fixing bugs in it.

Therefore, this is a feature I know I personally cannot maintain. I am not closed to the idea of having perforce support in rebar, but we shouldn't be adding features we can't reasonably keep improving. If you say you're willing to fix bugs that may show up with this, then that's fine and it should be included.

If this is a feature that is being 'dumped' on us -- sorry for the lack of better terms, I mean a pull request that is sent our way and then forgotten about by its creator -- then I am not one to want to commit to maintaining it.

Does this make more sense?

@ferd ferd reopened this Jun 9, 2014

@waisbrot

This comment has been minimized.

Show comment
Hide comment
@waisbrot

waisbrot Jun 12, 2014

Contributor

@ferd I think I was unclear and combined several unrelated statements into one blob. Let me try again (sorry about this getting a little long and dramatic):

Feedback about the pull-request process

I think you all do a generally excellent job of making Rebar a community project. Pull requests get dealt with quickly and the project generally feels well-loved. That's why I felt comfortable putting some significant effort into a pull request that I didn't think would necessarily get accepted.

In this case, I'm guessing that everyone thought "more VCS support is better" but nobody wanted to actually push the button for a VCS that they'd never touched. That's reasonable, but in my opinion it's disheartening to be stuck in the middle spot and I'd have preferred a snap decision one way or the other.

I don't think this is any systematic problem with how you're handling pull requests. Just a data-point for the future.

Level of commitment

I am a little defensive about this. I think that if the time I spent writing two alternate versions of this and discussing it isn't sufficient evidence that I'd try to maintain it, then me making some kind of written promise to do so certainly shouldn't be sufficient. If you feel you have to ask, just say no.

Should it be accepted? (no)

You say you can't maintain it. I assume that's true of the rest of the maintainers, too. I think that's a problem for this commit because even if I swear a blood-oath to maintain it, it's just one person away from bit-rot.

The other problem is that this has been hanging around for a while and I don't see anybody wanting it. I could add a 'base-64-enc-tarball-in-postgres-table' dependency type for Rebar, and even if it was awesome code the right thing to do would be to reject it because nobody will ever use that. The conclusion I've come to is that a Perforce dependency is in the same category.

Contributor

waisbrot commented Jun 12, 2014

@ferd I think I was unclear and combined several unrelated statements into one blob. Let me try again (sorry about this getting a little long and dramatic):

Feedback about the pull-request process

I think you all do a generally excellent job of making Rebar a community project. Pull requests get dealt with quickly and the project generally feels well-loved. That's why I felt comfortable putting some significant effort into a pull request that I didn't think would necessarily get accepted.

In this case, I'm guessing that everyone thought "more VCS support is better" but nobody wanted to actually push the button for a VCS that they'd never touched. That's reasonable, but in my opinion it's disheartening to be stuck in the middle spot and I'd have preferred a snap decision one way or the other.

I don't think this is any systematic problem with how you're handling pull requests. Just a data-point for the future.

Level of commitment

I am a little defensive about this. I think that if the time I spent writing two alternate versions of this and discussing it isn't sufficient evidence that I'd try to maintain it, then me making some kind of written promise to do so certainly shouldn't be sufficient. If you feel you have to ask, just say no.

Should it be accepted? (no)

You say you can't maintain it. I assume that's true of the rest of the maintainers, too. I think that's a problem for this commit because even if I swear a blood-oath to maintain it, it's just one person away from bit-rot.

The other problem is that this has been hanging around for a while and I don't see anybody wanting it. I could add a 'base-64-enc-tarball-in-postgres-table' dependency type for Rebar, and even if it was awesome code the right thing to do would be to reject it because nobody will ever use that. The conclusion I've come to is that a Perforce dependency is in the same category.

@ferd

This comment has been minimized.

Show comment
Hide comment
@ferd

ferd Jun 12, 2014

Contributor

There's an additional level of zaniness that tarball in a postgres table would have that p4 doesn't (I was happy to see hg eventually making it in, so p4 would be nice for their users). I'll take a note to try and build this, check a few things, and merge it when possible. I know I'm currently in a rush at work and low on time, but I'll try to merge it in.

The status for this one is: mergeable, should be merged (bar a few checks I want to make as I'll be the one to merge it in all likelihood), why wasn't it merged before (who knows).

Sorry for the long wait. I think @tsloughter and myself were added to the Rebar project in an attempt to help prevent this from happening in the future, and we're trying our best.

Contributor

ferd commented Jun 12, 2014

There's an additional level of zaniness that tarball in a postgres table would have that p4 doesn't (I was happy to see hg eventually making it in, so p4 would be nice for their users). I'll take a note to try and build this, check a few things, and merge it when possible. I know I'm currently in a rush at work and low on time, but I'll try to merge it in.

The status for this one is: mergeable, should be merged (bar a few checks I want to make as I'll be the one to merge it in all likelihood), why wasn't it merged before (who knows).

Sorry for the long wait. I think @tsloughter and myself were added to the Rebar project in an attempt to help prevent this from happening in the future, and we're trying our best.

ferd added a commit that referenced this pull request Jun 13, 2014

Merge pull request #136 from waisbrot/add-p4-support
Add support for the Perforce VCS client via the "p4" tool

@ferd ferd merged commit fe16668 into rebar:master Jun 13, 2014

2 checks passed

continuous-integration/travis-ci The Travis CI build passed
Details
default The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.