Add hg support to the buildbot patch uploader

RESOLVED FIXED

Status

P2
normal
RESOLVED FIXED
12 years ago
5 years ago

People

(Reporter: bhearsum, Assigned: bhearsum)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: complete)

Attachments

(2 attachments, 1 obsolete attachment)

I'm going to start working on adding hg support to the buildbot patch uploader while waiting on hardware for deployment.
(Assignee)

Updated

12 years ago
Priority: -- → P2
(Assignee)

Updated

12 years ago
Whiteboard: eta: july 20
Created attachment 272799 [details] [diff] [review]
cgi script + patch downloader with hg support

This patch adds support for building from Mercurial repositories to the Buildbot patch uploader.

I still have to write a BuildStep or two to make this work on the Buildbot end.
(Assignee)

Updated

12 years ago
Attachment #272799 - Flags: review?(rhelmer)
Comment on attachment 272799 [details] [diff] [review]
cgi script + patch downloader with hg support

Looks good; the inline comments aren't even strictly related to adding hg support, just was thinking about them though :)

General comments - 

* uploadpatch.cgi doesn't really seem like an appropriate name given this support.

* I know that we will need to support multiple repos at some point (one for tamarin, one for mozilla, maybe separate nss/nspr? not sure) but I think just one repo for now is fine. 

>Index: uploadpatch.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/buildbot-try/uploadpatch.cgi,v
>retrieving revision 1.1
>diff -u -r1.1 uploadpatch.cgi
>--- uploadpatch.cgi	14 Jul 2007 00:43:49 -0000	1.1
>+++ uploadpatch.cgi	18 Jul 2007 16:33:43 -0000
>@@ -159,76 +238,120 @@
> {
>     my $time = time();
>     # get the parameters
>-    my $name = '';
>-    $name = $ENV{'REMOTE_USER'};
>-    if ($name eq "") {
>-        $name = 'Noname';
>+    my $name = 'Noname';
>+    if ($ENV{'REMOTE_USER'}) {
>+        $name = $ENV{'REMOTE_USER'};
>     }


I think that absence of $ENV{'REMOTE_USER'}; is an error.


>+    if ($description =~ /([`]|\$\()/) {
>+        WritePage(description => $description, type => $type,
>+            branch => $branch, repoPath => $repoPath,
>+            err => 'Description must not contain backticks or "\$("');
>         return;


I think it's better to have a list of allowed chars instead of disallowed.
Attachment #272799 - Flags: review?(rhelmer) → review-
> * uploadpatch.cgi doesn't really seem like an appropriate name given this
> support.
> 

Agreed.

> * I know that we will need to support multiple repos at some point (one for
> tamarin, one for mozilla, maybe separate nss/nspr? not sure) but I think just
> one repo for now is fine. 
>

I have it in my head that the way hg support would be added to the patch uploader was by letting people point it to their own repositories. It sounds like this is incorrect though. Is hg try supposed to work the same way as the current setup (by uploading a patch)?

> I think that absence of $ENV{'REMOTE_USER'}; is an error.
>

Ok

> 
> I think it's better to have a list of allowed chars instead of disallowed.
> 

Ok. How does alphanumeric plus whitespace sound?
(In reply to comment #3)
> > * I know that we will need to support multiple repos at some point (one for
> > tamarin, one for mozilla, maybe separate nss/nspr? not sure) but I think just
> > one repo for now is fine. 
> >
> 
> I have it in my head that the way hg support would be added to the patch
> uploader was by letting people point it to their own repositories. It sounds
> like this is incorrect though. Is hg try supposed to work the same way as the
> current setup (by uploading a patch)?


No you've got it right; just we might have to support multiple repos (mozilla-central, tamarin-central, etc. etc.). But, ignore that for this iteration, and assume that you can pull an hg repo that's equiv to the CVS repo, and we can make it more complex as we need to.



> > I think it's better to have a list of allowed chars instead of disallowed.
> > 
> 
> Ok. How does alphanumeric plus whitespace sound?


Sounds great!

Comment 5

12 years ago
> No you've got it right; just we might have to support multiple repos
> (mozilla-central, tamarin-central, etc. etc.). But, ignore that for this
> iteration, and assume that you can pull an hg repo that's equiv to the CVS
> repo, and we can make it more complex as we need to.

Uh, we *already* need to pull multiple repos, because you have to specify the tamarin repo to pull in client.py. I think we can leave the default NSPR/NSS pull alone, but there is no default tamarin pull.
(In reply to comment #5)
> > No you've got it right; just we might have to support multiple repos
> > (mozilla-central, tamarin-central, etc. etc.). But, ignore that for this
> > iteration, and assume that you can pull an hg repo that's equiv to the CVS
> > repo, and we can make it more complex as we need to.
> 
> Uh, we *already* need to pull multiple repos, because you have to specify the
> tamarin repo to pull in client.py. I think we can leave the default NSPR/NSS
> pull alone, but there is no default tamarin pull.
> 

Ok. That's easy enough to add.
Created attachment 273132 [details] [diff] [review]
patch uploader with hg support

Rob & Benjamin, I've addressed your comments in this patch. I've also made the interface a bit nicer.

I generated this patch with 'cvsdo' from cvsutils because of the file renaming but the diff it generated won't remove the old files.
Attachment #272799 - Attachment is obsolete: true
Attachment #273132 - Flags: review?(rhelmer)
(Assignee)

Updated

12 years ago
Whiteboard: eta: july 20 → waiting on review
Attachment #273132 - Flags: review?(rhelmer) → review+
Can we get this checked-in?
Whiteboard: waiting on review → waiting on check-in
Created attachment 273462 [details] [diff] [review]
patch as landed

Removed downloadpatches.sh/uploadpatch.cgi as well:

Removing downloadpatches.sh;
/cvsroot/mozilla/webtools/buildbot-try/downloadpatches.sh,v  <--  downloadpatches.sh
new revision: delete; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/buildbot-try/processchanges.sh,v
done
Checking in processchanges.sh;
/cvsroot/mozilla/webtools/buildbot-try/processchanges.sh,v  <--  processchanges.sh
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/buildbot-try/sendchange.cgi,v
done
Checking in sendchange.cgi;
/cvsroot/mozilla/webtools/buildbot-try/sendchange.cgi,v  <--  sendchange.cgi
initial revision: 1.1
done
Removing uploadpatch.cgi;
/cvsroot/mozilla/webtools/buildbot-try/uploadpatch.cgi,v  <--  uploadpatch.cgi
new revision: delete; previous revision: 1.1
done
I have to write a BuildStep to process the Mercurial changes still. I can't think of a better place to put it so I will add it here once it's done.
Whiteboard: waiting on check-in → eta: july 25
rhelmer suggested using bug 387697 for the BuildStep; going to do that and close this one.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: eta: july 25 → complete
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.