Bugfixing URL construction for extract build step

Bugfixing URL construction for extract build step

On Windows, if the value for the --build-url argument doesn't include
the filename, auto-generation of the filename ends up adding a backslash
due to use of os.path.join in the construction of the URL. This bugfix
simply changes it to use a forward slash, which should always be correct
for URLs.

BUG=none (but it breaks the ChromiumWebRTC waterfall)
TEST=Successsfully executed extract_build step on a slave with local
modifications to this script.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=175507

[kjellander_chromium, 2012-12-21 09:27:04]

Isaac, do you think you can take a look at this? 
I figure Chase is very busy.

[Isaac (away), 2012-12-21 09:38:27]

Happy to review.  This CL won't work as written however, because it could create
a double '/' if url already ends with a /, which is possible since that value
might come from buildbot.

We essentially want os.path.join but force it to use a '/' separator.  There is
also http://docs.python.org/2/library/urlparse.html#urlparse.urljoin but I dont
think the will work here.

[kjellander_chromium, 2012-12-21 12:31:49]

Thanks! 
Actually urlparse.urljoin seems to work fine (even if it's documentation is a
bit confusing sometimes). 
I added a unit test to ensure it's working as intended in this case.

[Isaac (away), 2012-12-21 21:54:23]

urljoin will cut off parts of the base url to replace with the new url.  Testing
with www.google.com/ doesn't show this.

> urlparse.urljoin('www.google.com/foo', 'foo.html')
'www.google.com/foo.html'

I think we should just roll our own function that makes sure there's exactly one
slash.

[kjellander_chromium, 2012-12-28 16:47:26]

I agree we shouldn't complicate this too much, and urlparse.urljoin also doesn't
support the gs:// URLs. 
What do you think about this solution instead?

https://codereview.chromium.org/11451004/diff/12001/scripts/slave/extract_bui...
File scripts/slave/extract_build.py (right):

https://codereview.chromium.org/11451004/diff/12001/scripts/slave/extract_bui...
scripts/slave/extract_build.py:80: if not replace_dict.get('parent_builddir')
and replace_dict.get('parentname'):
This avoids crashing if an empty build_properties dict is passed, which I think
should be possible.

https://codereview.chromium.org/11451004/diff/12001/scripts/slave/extract_bui...
scripts/slave/extract_build.py:88: # Append the filename to the base URL. First
strip any trailing slashes.
Simple way of avoiding double slashes being concatenated into the URL.
urlparse.urljoin cannot be used since it doesn't support gs:// URLs (in addition
to the somewhat complicated way it works).

https://codereview.chromium.org/11451004/diff/12001/scripts/slave/slave_utils.py
File scripts/slave/slave_utils.py (right):

https://codereview.chromium.org/11451004/diff/12001/scripts/slave/slave_utils...
scripts/slave/slave_utils.py:96: if 'try' in build_properties.get('mastername',
''):
Just to avoid an error if an empty build_properties dict is passed.

[Isaac (away), 2013-01-08 08:54:02]

lgtm w/ nits

https://codereview.chromium.org/11451004/diff/12001/scripts/slave/extract_bui...
File scripts/slave/extract_build.py (right):

https://codereview.chromium.org/11451004/diff/12001/scripts/slave/extract_bui...
scripts/slave/extract_build.py:80: if not replace_dict.get('parent_builddir')
and replace_dict.get('parentname'):
On 2012/12/28 16:47:31, Henrik Kjellander wrote:
> This avoids crashing if an empty build_properties dict is passed, which I
think
> should be possible.

Still might crash because replace_dict wouldn't have parent_builddir and could
fail the string format operation on line 91.  How about instead we change line
81 to have replace_dict.get('parentname', '')

https://codereview.chromium.org/11451004/diff/12001/scripts/slave/extract_bui...
scripts/slave/extract_build.py:89: url = url.strip('/')
nit, cleaner to use rstrip

[Isaac (away), 2013-01-08 09:11:21]

https://chromiumcodereview.appspot.com/11451004/diff/12001/scripts/slave/unit...
File scripts/slave/unittests/extract_build_test.py (right):

https://chromiumcodereview.appspot.com/11451004/diff/12001/scripts/slave/unit...
scripts/slave/unittests/extract_build_test.py:22: options = optparse.Values
Thanks for adding a test.  Note you could alternatively hand craft an
options-like object without optparse internals.  But this is good enough.

class MockOptions(object):
  build_properties = {}

MockOpts = MockOptions()

[kjellander_chromium, 2013-01-08 09:32:51]

Thanks for helping me getting this fixed.

https://codereview.chromium.org/11451004/diff/12001/scripts/slave/extract_bui...
File scripts/slave/extract_build.py (right):

https://codereview.chromium.org/11451004/diff/12001/scripts/slave/extract_bui...
scripts/slave/extract_build.py:80: if not replace_dict.get('parent_builddir')
and replace_dict.get('parentname'):
Good idea. Updated. 
On 2013/01/08 08:54:02, Isaac wrote:
> On 2012/12/28 16:47:31, Henrik Kjellander wrote:
> > This avoids crashing if an empty build_properties dict is passed, which I
> think
> > should be possible.
> 
> Still might crash because replace_dict wouldn't have parent_builddir and could
> fail the string format operation on line 91.  How about instead we change line
> 81 to have replace_dict.get('parentname', '')

https://codereview.chromium.org/11451004/diff/12001/scripts/slave/extract_bui...
scripts/slave/extract_build.py:89: url = url.strip('/')
On 2013/01/08 08:54:02, Isaac wrote:
> nit, cleaner to use rstrip

Done.

https://codereview.chromium.org/11451004/diff/12001/scripts/slave/unittests/e...
File scripts/slave/unittests/extract_build_test.py (right):

https://codereview.chromium.org/11451004/diff/12001/scripts/slave/unittests/e...
scripts/slave/unittests/extract_build_test.py:22: options = optparse.Values
Ah, of course. I still have a lot of typeless programming to learn :)

On 2013/01/08 09:11:21, Isaac wrote:
> Thanks for adding a test.  Note you could alternatively hand craft an
> options-like object without optparse internals.  But this is good enough.
> 
> class MockOptions(object):
>   build_properties = {}
> 
> MockOpts = MockOptions()

[commit-bot: I haz the power, 2013-01-08 09:33:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kjellander@chromium.org/11451004/25001

[commit-bot: I haz the power, 2013-01-08 09:33:09]

Failed to apply patch for build/scripts/slave/slave_utils.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file build/scripts/slave/slave_utils.py
  Hunk #1 FAILED at 93.
  1 out of 1 hunk FAILED -- saving rejects to file
build/scripts/slave/slave_utils.py.rej

Patch:       build/scripts/slave/slave_utils.py
Index: scripts/slave/slave_utils.py
diff --git build/scripts/slave/slave_utils.py build/scripts/slave/slave_utils.py
index
0a6fcfb6386567adb6b12b2c293b1b4573eac026..6a627e13eb0c7678de94f52d2cb26fcc9d00cb27
100644
--- a/build/scripts/slave/slave_utils.py
+++ b/build/scripts/slave/slave_utils.py
@@ -93,7 +93,7 @@ def GetZipFileNames(build_properties, build_dir,
webkit_dir=None,
   base_name = 'full-build-%s' % chromium_utils.PlatformName()
 
   chromium_revision = SubversionRevision(build_dir)
-  if 'try' in build_properties.get('mastername'):
+  if 'try' in build_properties.get('mastername', ''):
     if extract:
       if not build_properties.get('parent_buildnumber'):
         raise Exception('build_props does not have parent data: %s' %

[commit-bot: I haz the power, 2013-01-08 09:37:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kjellander@chromium.org/11451004/23002

[commit-bot: I haz the power, 2013-01-08 09:42:13]

Change committed as 175507