Summary

post-review for P4win

Review Request #467 — Created July 24, 2008 and submitted

Information

Owner

JSlominski

Repository

Review Board SVN (deprecated)

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

I have updated post-review to set the proper perforce environment.  Before P4Tool.txt is imported into P4win, the user needs to enter the path of post-review.  After it is imported they should be able to right click on any change list and by clicking post-review, post a review of the change list in perforce.

Testing Done

Given that this is a tool that needs to be compiled, it'd be nice to make this its own top-level module in /trunk and add a Makefile for compiling it.

Also, is there any reason we need a wrapper? The script can't just run python post-review or maybe a compiled post-review.exe?

JSlominski July 25, 2008, 9:13 a.m.

The reason I out this in a wrapper is because post-review will take the client and server that is set in the P4 ENV variables.  By setting up the environment properly before running python post-review, users can submit reviews from multiple client specs without any hassle.  I’m not sure how other places use clients but we have a separate client for each branch of software we release from.  I could set up a compiled version of this but then I would need to make the command line arguments more robust.  If you think that would be a better approach I can start working on that on Monday.

chipx86 July 25, 2008, 9:47 a.m.

Maybe what we should do is have some Perforce-specific arguments in post-review for setting these variables instead of getting them from the environment.

JSlominski July 28, 2008, 4:40 a.m.

Should I abandon this and make post-review.py more robust?

chipx86 Aug. 26, 2008, 7:48 p.m.

Sorry, missed your question before.

Yeah, it might be worth making post-review more robust so that it can be used in other ways. Having these Perforce-specific arguments could be useful in all kinds of scripts.

/trunk/reviewboard/contrib/tools/P4post-review.cpp (Diff revision 1)

You can use setenv for this.

JSlominski July 25, 2008, 9:13 a.m.

I know setenv() works on Unix platforms but windows didn't like it.  I took the easy route and did this.  Ill do some more digging on Monday to see if I can find the proper function.

JS

JSlominski July 28, 2008, 4:40 a.m.
```
Fixed this for the windows platform
```

/trunk/reviewboard/contrib/tools/post-review (Diff revision 1)
```
This line can go.
```
1. JS
  
  JSlominski July 25, 2008, 9:04 a.m.
  Not really sure how this got here

/trunk/reviewboard/contrib/tools/post-review (Diff revision 3)
```
You don't need "!= None" here.
```
/trunk/reviewboard/contrib/tools/post-review (Diff revision 3)
```
You should be using isinstance() here.
```

/trunk/reviewboard/contrib/tools/post-review (Diff revision 3)

I'd prefer --p4-client and --p4-port. The variable names should be p4_client and p4_port too.

/trunk/reviewboard/contrib/tools/post-review (Diff revision 3)

I'd rather we not abbreviate "CL". This is more generic than changelists, so let's just say something like "the Perforce client name" and "the Perforce server address" or something.

Ship it!

Thanks, committed as r1550.

I don't use Windows and therefore don't use P4win, so I can't test this, but what happens if you need to log in? Does P4win provide any sort of a terminal UI where you can input this information, and does it show the resulting URL?

We may want to modify this to use -o (for opening a web browser) after this is posted.