Skip to content
This repository has been archived by the owner on May 12, 2018. 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

waisbrot
Copy link
Contributor

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.

case inet:gethostname() of
{ok,HostName} ->
HostName ++ "-" ++ os:getenv("USER") ++ "-" ++ Basename ++ "-Rebar-automated-download"
end}.
Copy link

Choose a reason for hiding this comment

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

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

@waisbrot
Copy link
Contributor Author

Fixed long line and improved commit message

@ghost
Copy link

ghost 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
Copy link
Member

dizzyd commented Sep 20, 2013

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

@waisbrot
Copy link
Contributor Author

@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?

@ghost
Copy link

ghost 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
Copy link
Contributor Author

'p4' examples added

@ghost
Copy link

ghost 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
Copy link
Contributor Author

@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.

@ghost
Copy link

ghost commented Nov 28, 2013

Works for me. cc @jaredmorrow.

@jaredmorrow
Copy link
Contributor

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

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
Copy link
Contributor Author

Rebased and it looks OK to me.

@ghost
Copy link

ghost commented May 29, 2014

Is there anything holding up a merge?

@waisbrot
Copy link
Contributor Author

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?

@ghost
Copy link

ghost commented Jun 3, 2014

@jaredmorrow ping?

@ferd
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

@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
Copy link
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
Add support for the Perforce VCS client via the "p4" tool
@ferd ferd merged commit fe16668 into rebar:master Jun 13, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants