post-review and P4PASSWD

244 views
Skip to first unread message

RShelley

unread,
Aug 25, 2010, 11:56:28 AM8/25/10
to reviewboard
I've been struggling recently getting post-review to pick up the
P4PASSWD properly. We have post-review as part of a script and we
first run "p4 login -p" to get a ticket and then put that ticket in
the environment. Subsequent "p4 describe -P <ticket> <changelist>"
commands work fine, but post-review gives the "P4PASSWORD is invalid
or unset" error. I've tried everything I can think of. Is there any
way at all that we can get post-review to accept a "--p4-ticket"
argument and that can translate into "-P <ticket>" arguments on the P4
commands?

tag_98007work

unread,
Aug 25, 2010, 12:28:39 PM8/25/10
to reviewboard
I was having P4PASSWD issues until I patched the perforce.py file with
the changes from
http://reviews.reviewboard.org/r/1537/diff/. After I patched the file
I restarted the server (don't know if that's needed or not)

RShelley

unread,
Aug 25, 2010, 2:50:57 PM8/25/10
to reviewboard
Where would perforce.py be located? I've hunted around and can't find
it.

On Aug 25, 9:28 am, tag_98007work <tag98007w...@gmail.com> wrote:
> I was having P4PASSWD issues until I patched the perforce.py file with
> the changes fromhttp://reviews.reviewboard.org/r/1537/diff/.  After I patched the file

tag_98007work

unread,
Aug 25, 2010, 3:22:03 PM8/25/10
to reviewboard
It's located under reviewboard/scmtools. The full path for mine is: /
usr/lib/python2.5/site-packages/ReviewBoard-1.5rc1-py2.5.egg/
reviewboard/scmtools. Obviously this is a Unix install. I'm not
quite sure where it would be for windows. Does that help?
> > > commands?- Hide quoted text -
>
> - Show quoted text -

RShelley

unread,
Aug 25, 2010, 4:11:35 PM8/25/10
to reviewboard
I'm on Linux and I found the site-packages, but I don't have any
directories under it, just the eggs.

I also tried: find / -name perforce.py
and that didn't come up with anything

tag_98007work

unread,
Aug 25, 2010, 4:20:21 PM8/25/10
to reviewboard
hmm, at one point (I'm having issues and attmpting to re-install,
whole 'nother story) my ReviewBoard-1.5rc1-py2.5.egg was a top level
directory. Under that was the scmtools directory. So I'm not quite
sure how to resolve that they egg isn't a directory.
> > > - Show quoted text -- Hide quoted text -

RShelley

unread,
Aug 25, 2010, 4:37:27 PM8/25/10
to reviewboard
My head hurts... Perforce consistently gives me reasons to not like
it... Git can't come soon enough.

Thanks for trying, I appreciate it!

Dana Lacoste

unread,
Aug 25, 2010, 6:04:33 PM8/25/10
to revie...@googlegroups.com
So, to summarize in case someone is searching through these archives
later:

Perforce, when security is turned on (this is a p4d server setting) will
have different restrictions on what can and can't be done with the
password on the CLIENT (i.e. you can't necessarily simply set an
environment variable and run with it. For reference, see the P4
SysAdmin Guide at
http://www.perforce.com/perforce/r10.1/manuals/p4sag/03_superuser.html#1
081537 )

User RShelley is running into issues with this and is having difficulty.

http://reviews.reviewboard.org/r/1537/diff/ has a patch (which was
discarded) which "handles" this situation for user tag_98007work.

But, if you're on Windows (or potentially in some other environments),
you're quite possibly using a .egg/.exe "compiled" version of Review
Board and therefore it may be difficult to implement patches to the .py
Python source. If you use the interpreted version, then you can apply
the patch and things might work for you (if your environment is like
that of user tag_98007work )

Does that seem to sum things up OK?

Note that I don't see any of the problems described here, but I had to
do the following to get post-review to work for everyone:
1 - Everyone has to have the following environment set: P4PORT, P4USER,
P4CLIENT (NOT P4PASSWD!)
2 - Everyone has to be running P4V when they run post-review (I actually
added it as a tool to P4V so the users can right click on any changelist
and hit "submit to reviewboard" and nobody needs to use a command
line/shell command at all)
With perforce 2009.2/2010.1 this works fine with no problems : the
ticket from P4V is visible to the p4 command line (called from
post-review) and everything's fine (this is using latest RBTools
installed with the command "easy_install -U RBTools" and review board
1.5 beta 2)

Dana Lacoste

RShelley

unread,
Aug 25, 2010, 7:46:38 PM8/25/10
to reviewboard
Dana, that sums it up pretty well. My problem is that the post-commit
command isn't being run by the user, it's run from a script on another
box (so it's not using P4V). "p4 login" is called first to attempt to
login and obtain a ticket and then I can use the ticket for future
commands, but post-review doesn't accept a ticket parameter and p4
isn't taking it from the environment. If I could pass it to post-
review with the rest of the p4 properties, it'd be splendid.

On Aug 25, 3:04 pm, "Dana Lacoste" <dlaco...@aperio.com> wrote:
> So, to summarize in case someone is searching through these archives
> later:
>
> Perforce, when security is turned on (this is a p4d server setting) will
> have different restrictions on what can and can't be done with the
> password on the CLIENT (i.e. you can't necessarily simply set an
> environment variable and run with it.  For reference, see the P4
> SysAdmin Guide athttp://www.perforce.com/perforce/r10.1/manuals/p4sag/03_superuser.html#1
> 081537 )
>
> User RShelley is running into issues with this and is having difficulty.
>
> http://reviews.reviewboard.org/r/1537/diff/has a patch (which was

Christian Hammond

unread,
Aug 26, 2010, 4:47:14 AM8/26/10
to revie...@googlegroups.com
I think it's definitely reasonable to have a parameter for this.

The perforce.py mentioned above would be for Review Board. With post-review, you'd be looking for postreview.py in rbtools.

What I'd advise is to check out a copy of rbtools from Git and add a --p4-ticket parameter. It shouldn't be too hard. You can see how the other options work (which are defined near the bottom of the file), and make use of the passed option to pass the right thing to p4. I don't know about the ticketing stuff well enough to know what needs to be passed or set. If you can get it working, though, I'll commit a patch so you won't have to maintain a fork for too long.

Christian

--
Christian Hammond - chi...@chipx86.com
Review Board - http://www.reviewboard.org
VMware, Inc. - http://www.vmware.com


--
Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/
Happy user? Let us know at http://www.reviewboard.org/users/
-~----------~----~----~----~------~----~------~--~---
To unsubscribe from this group, send email to reviewboard...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en

RShelley

unread,
Aug 26, 2010, 7:02:01 PM8/26/10
to reviewboard
Well, I got past the P4PASSWD issue and then began getting the error
"The current directory does not contain a checkout from a supported
source code repository." After much digging, I found that the error
is actually wrong.

The PerforceClient's "def_repository_info" function checks for a P4
install first:

def get_repository_info(self):
if not check_install('p4 help'):
return None

data = execute(["p4", "info"], ignore_errors=True)

If "p4" cannot be found for "p4 help", it returns None, and the
calling function assumes that a repository wasn't found, not that P4
couldn't be found. As it turns out, the service running this script
is changing the PATH and post-review cannot find P4, so this check
fails. More specifically, this is running from Hudson, and a recent
upgrade of the app changed how environment variables cascaded to child
processes, so when Hudson was upgraded, the PATH environment variables
weren't propagated, and post-review couldn't find P4.

I'd like to suggest that this "if" check actually throw an exception,
and not simply returns None. Thoughts?

-Ryan

On Aug 26, 1:47 am, Christian Hammond <chip...@chipx86.com> wrote:
> I think it's definitely reasonable to have a parameter for this.
>
> The perforce.py mentioned above would be for Review Board. With post-review,
> you'd be looking for postreview.py in rbtools.
>
> What I'd advise is to check out a copy of rbtools from Git and add a
> --p4-ticket parameter. It shouldn't be too hard. You can see how the other
> options work (which are defined near the bottom of the file), and make use
> of the passed option to pass the right thing to p4. I don't know about the
> ticketing stuff well enough to know what needs to be passed or set. If you
> can get it working, though, I'll commit a patch so you won't have to
> maintain a fork for too long.
>
> Christian
>
> --
> Christian Hammond - chip...@chipx86.com
> Review Board -http://www.reviewboard.org
> VMware, Inc. -http://www.vmware.com
>
>
>
> On Wed, Aug 25, 2010 at 4:46 PM, RShelley <12gaugeme...@gmail.com> wrote:
> > Dana, that sums it up pretty well.  My problem is that the post-commit
> > command isn't being run by the user, it's run from a script on another
> > box (so it's not using P4V).  "p4 login" is called first to attempt to
> > login and obtain a ticket and then I can use the ticket for future
> > commands, but post-review doesn't accept a ticket parameter and p4
> > isn't taking it from the environment.  If I could pass it to post-
> > review with the rest of the p4 properties, it'd be splendid.
>
> > On Aug 25, 3:04 pm, "Dana Lacoste" <dlaco...@aperio.com> wrote:
> > > So, to summarize in case someone is searching through these archives
> > > later:
>
> > > Perforce, when security is turned on (this is a p4d server setting) will
> > > have different restrictions on what can and can't be done with the
> > > password on the CLIENT (i.e. you can't necessarily simply set an
> > > environment variable and run with it.  For reference, see the P4
> > > SysAdmin Guide athttp://
> >www.perforce.com/perforce/r10.1/manuals/p4sag/03_superuser.html#1
> > > 081537 )
>
> > > User RShelley is running into issues with this and is having difficulty.
>
> > >http://reviews.reviewboard.org/r/1537/diff/hasa patch (which was
> > Happy user? Let us know athttp://www.reviewboard.org/users/
> > -~----------~----~----~----~------~----~------~--~---
> > To unsubscribe from this group, send email to
> > reviewboard...@googlegroups.com<reviewboard%2Bunsubscribe@googlegr oups.com>

Christian Hammond

unread,
Aug 29, 2010, 6:42:44 PM8/29/10
to revie...@googlegroups.com
With the exception, would this be used to print an error and fail if p4 isn't found?

The reason we return None upon failure is that not all installs will use p4, and it actually shouldn't be failing if p4 is not installed. That check there is to see if p4 help can be run, and if not, we assume that that type of repository is just simply not supported on this system.

I might be misunderstanding, though.

What may be nice is to have some sort of option that lists all supported repository types, or checks if a certain provided type is supported, so that the calling script could verify this (basically an assertion check) before calling post-review with the necessary options.

Christian

--
Christian Hammond - chi...@chipx86.com
Review Board - http://www.reviewboard.org
VMware, Inc. - http://www.vmware.com


Happy user? Let us know at http://www.reviewboard.org/users/

-~----------~----~----~----~------~----~------~--~---
To unsubscribe from this group, send email to reviewboard...@googlegroups.com

RShelley

unread,
Sep 1, 2010, 12:48:54 PM9/1/10
to reviewboard
Ok, I can understand that. But I don't think that check should be
during the def_repository_info function. I imagine the
"check_install" function should probably happen somewhere higher up at
the beginning of the process instead of it's current location where
the error that would come back from a P4 install not finding the "p4"
executable is currently that a checkout couldn't be found. That led
me astray for quite some time.

On Aug 29, 3:42 pm, Christian Hammond <chip...@chipx86.com> wrote:
> With the exception, would this be used to print an error and fail if p4
> isn't found?
>
> The reason we return None upon failure is that not all installs will use p4,
> and it actually shouldn't be failing if p4 is not installed. That check
> there is to see if p4 help can be run, and if not, we assume that that type
> of repository is just simply not supported on this system.
>
> I might be misunderstanding, though.
>
> What may be nice is to have some sort of option that lists all supported
> repository types, or checks if a certain provided type is supported, so that
> the calling script could verify this (basically an assertion check) before
> calling post-review with the necessary options.
>
> Christian
>
> --
> > > > >http://reviews.reviewboard.org/r/1537/diff/hasapatch (which was
Reply all
Reply to author
Forward
0 new messages