Bug 47981

Summary: [NRWT] Get child process number from environment variable
Product: WebKit Reporter: Gabor Rapcsanyi <rgabor>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, commit-queue, dino, dpranke, eric, ojan, ossy, pol, tony, wsiegrist
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
proposed patch none

Description Gabor Rapcsanyi 2010-10-20 06:23:20 PDT
Get the child process number from an environment variable if it exists.
It would be useful for buildbots.
Comment 1 Gabor Rapcsanyi 2010-10-20 06:33:25 PDT
Created attachment 71284 [details]
proposed patch
Comment 2 Csaba Osztrogon√°c 2010-10-20 06:37:47 PDT
(In reply to comment #1)
> Created an attachment (id=71284) [details]
> proposed patch

LGTM. ;)
Comment 3 Ojan Vafai 2010-10-20 08:33:08 PDT
Comment on attachment 71284 [details]
proposed patch

I don't have a problem with this, but how is an environment variable easier than just passing something on the commandline?
Comment 4 Csaba Osztrogon√°c 2010-10-20 08:38:31 PDT
(In reply to comment #3)
> (From update of attachment 71284 [details])
> I don't have a problem with this, but how is an environment variable easier than just passing something on the commandline?

If you would like to add/modify a commandline parameter on the buildbot, you have to modify the buildbot master config and ask Bill to update the buildbot master. But you can easily set an environment variable locally.
Comment 5 Eric Seidel (no email) 2010-10-20 09:30:50 PDT
I guess.  Always feels strange to have environment variables for these sorts of things.

I assume the command-line wins out over the environment variable.

If you have concerns like this you could also have the buildbot run some wrapper script that you can control more easily.
Comment 6 Gabor Rapcsanyi 2010-10-20 11:35:22 PDT
(In reply to comment #5)
> I guess.  Always feels strange to have environment variables for these sorts of things.
> 
> I assume the command-line wins out over the environment variable.
> 
> If you have concerns like this you could also have the buildbot run some wrapper script that you can control more easily.

Yes, the command-line is the primary.
Now we run all of our buildbots with different and custom environment and these kind of options are not port or platform specifics so we can't set them on the buildbot master.
The wrapper is not good because the buildbot master will "call" the original script not your wrapper. So I think that's the simplest way to set different child process number on buildbots like WEBKIT_WAIT_FOR_HTTPD in ORWT to set http lock and BUILD_WEBKIT_ARGS in build-webkit script.
Comment 7 WebKit Commit Bot 2010-10-20 11:54:41 PDT
Comment on attachment 71284 [details]
proposed patch

Rejecting patch 71284 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 71284]" exit_code: 2
Cleaning working directory
Updating working directory
Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2

Full output: http://queues.webkit.org/results/4534020
Comment 8 Eric Seidel (no email) 2010-10-20 11:57:36 PDT
Comment on attachment 71284 [details]
proposed patch

I think this was a false rejection from a run-away commit-node.
Comment 9 Dirk Pranke 2010-10-20 13:39:24 PDT
Comment on attachment 71284 [details]
proposed patch

The patch looks good to me as well, although I'm not a reviewer. As with others, I'm not generally a fan of environmental variables, but it sounds like it is useful here. Ideally there'd be a comment next to this line in the code about *why* we're looking at the variable (since it's easily settable on the bots, unlike the command line)
Comment 10 WebKit Commit Bot 2010-10-20 17:01:11 PDT
The commit-queue encountered the following flaky tests while processing attachment 71284 [details]:

transitions/transition-end-event-transform.html
http/tests/appcache/fail-on-update-2.html

Please file bugs against the tests.  The author(s) of the test(s) have been CCed on this bug.  The commit-queue is continuing to process your patch.
Comment 11 WebKit Commit Bot 2010-10-21 02:35:14 PDT
Comment on attachment 71284 [details]
proposed patch

Clearing flags on attachment: 71284

Committed r70217: <http://trac.webkit.org/changeset/70217>
Comment 12 WebKit Commit Bot 2010-10-21 02:35:21 PDT
All reviewed patches have been landed.  Closing bug.