Improve handling of packages locally (file:// support)

RESOLVED FIXED

Status

Release Engineering
Mozharness
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: armenzg, Assigned: armenzg)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8490328 [details] [diff] [review]
Add --jsshell-url support as well as file:// URLs

I see an error in unit.sh, however, it is not related to my code (AFAIK).
I get the same error without my code.

armenzg-thinkpad mozharness hg:[default!] $ coverage run -a --branch '--omit='\''/usr/*,/opt/*'\''' /usr/local/bin/nosetests test/test_base_vcs_mercurial.py...E........................................
======================================================================
ERROR: test_apply_and_push_rebase_fails (test_base_vcs_mercurial.TestHg)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/armenzg/repos/mozharness/test/test_base_vcs_mercurial.py", line 524, in test_apply_and_push_rebase_fails
    lambda r, a: c(r, a, self.repodir), max_attempts=4)
  File "/home/armenzg/repos/mozharness/mozharness/base/vcs/mercurial.py", line 515, in apply_and_push
    self.update(localrepo, branch=branch)
  File "/home/armenzg/repos/mozharness/mozharness/base/vcs/mercurial.py", line 160, in update
    raise VCSException("Unable to update %s!" % dest)
VCSException: Unable to update /tmp/tmpWUYZxm/wc!
Attachment #8490328 - Flags: review?(jlund)

Comment 1

3 years ago
> armenzg-thinkpad mozharness hg:[default!] $ coverage run -a --branch
> '--omit='\''/usr/*,/opt/*'\''' /usr/local/bin/nosetests
> test/test_base_vcs_mercurial.py...E........................................
> ======================================================================
> ERROR: test_apply_and_push_rebase_fails (test_base_vcs_mercurial.TestHg)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "/home/armenzg/repos/mozharness/test/test_base_vcs_mercurial.py",
> line 524, in test_apply_and_push_rebase_fails
>     lambda r, a: c(r, a, self.repodir), max_attempts=4)
>   File "/home/armenzg/repos/mozharness/mozharness/base/vcs/mercurial.py",
> line 515, in apply_and_push
>     self.update(localrepo, branch=branch)
>   File "/home/armenzg/repos/mozharness/mozharness/base/vcs/mercurial.py",
> line 160, in update
>     raise VCSException("Unable to update %s!" % dest)
> VCSException: Unable to update /tmp/tmpWUYZxm/wc!

you must have a newer version of hg. I think this has a dep on older version and a fix has not been impl'd yet.

Comment 2

3 years ago
Comment on attachment 8490328 [details] [diff] [review]
Add --jsshell-url support as well as file:// URLs

Review of attachment 8490328 [details] [diff] [review]:
-----------------------------------------------------------------

These changes looks like they'll be really helpful!

jsurl looks good. the file:// parts are tricky as so much of mozharness touches it. I feel that part needs further discussion/tweaking. as always, open to your thoughts :)

the part that forces the r- is the lack of support for https

::: mozharness/base/script.py
@@ +186,5 @@
>  
>      def _download_file(self, url, file_name):
>          """ Helper script for download_file()
>          """
> +        if not url.startswith("http://") and \

I think there are many times (e.g. ftp.mozilla) where we use https.

@@ +188,5 @@
>          """ Helper script for download_file()
>          """
> +        if not url.startswith("http://") and \
> +                not url.startswith("file://"):
> +            assert os.path.isfile(url), "The file %s does not exist" % url

do we want to do an assert here over catching it and logging a self.fatal or self.error?

IIUC an assert will just cause the script to stop on the spot.

@@ +189,5 @@
>          """
> +        if not url.startswith("http://") and \
> +                not url.startswith("file://"):
> +            assert os.path.isfile(url), "The file %s does not exist" % url
> +            url = 'file://%s' % url

this one is tough. as this is, we are saying if we don't prepend the protocol or it is not recognized, we assume it is a file. I'm tempted to self.warn instead and leave url as is. what do you think?

maybe if url[0] == '/' (like you do in testbase.py) then log and prepending it with file protocol might make sense. not sure.
Attachment #8490328 - Flags: review?(jlund) → review-
(Assignee)

Comment 3

3 years ago
Created attachment 8490737 [details] [diff] [review]
Add --jsshell-url support as well as file:// URLs
Attachment #8490328 - Attachment is obsolete: true
(Assignee)

Comment 4

3 years ago
Comment on attachment 8490328 [details] [diff] [review]
Add --jsshell-url support as well as file:// URLs

Review of attachment 8490328 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozharness/base/script.py
@@ +186,5 @@
>  
>      def _download_file(self, url, file_name):
>          """ Helper script for download_file()
>          """
> +        if not url.startswith("http://") and \

I fixed this on my next patch.

@@ +188,5 @@
>          """ Helper script for download_file()
>          """
> +        if not url.startswith("http://") and \
> +                not url.startswith("file://"):
> +            assert os.path.isfile(url), "The file %s does not exist" % url

I switched it to self.fatal() on my next patch.

@@ +189,5 @@
>          """
> +        if not url.startswith("http://") and \
> +                not url.startswith("file://"):
> +            assert os.path.isfile(url), "The file %s does not exist" % url
> +            url = 'file://%s' % url

url[0] == '/' would only work for absolute paths.
It also only works for Unix based platforms.

With file:// we can bridge all of these issues.

We could self.warn() and saying that we're doing the swap, however, I'm leaning toward the line that it is an innocuous change.

I want to support these 4 approaches:
* http:// & https://
* file://
* absolute paths
* relative paths
(Assignee)

Comment 5

3 years ago
Comment on attachment 8490737 [details] [diff] [review]
Add --jsshell-url support as well as file:// URLs

Assuming my answers in the previous comment are satisfactory.
Attachment #8490737 - Flags: review?(jlund)

Comment 6

3 years ago
Comment on attachment 8490737 [details] [diff] [review]
Add --jsshell-url support as well as file:// URLs

Review of attachment 8490737 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozharness/base/script.py
@@ +188,5 @@
>          """ Helper script for download_file()
>          """
> +        # If our URLs look like files, prefix them with file:// so they can
> +        # be loaded like URLs.
> +        if not url.startswith("http") and not url.startswith("file://"):

https? I think this might be same patch
(Assignee)

Updated

3 years ago
Attachment #8490737 - Flags: review?(jlund)
(Assignee)

Comment 7

3 years ago
Created attachment 8491484 [details] [diff] [review]
Add --jsshell-url support as well as file:// URLs
Attachment #8490737 - Attachment is obsolete: true
Attachment #8491484 - Flags: review?(jlund)
(Assignee)

Comment 8

3 years ago
Created attachment 8491568 [details] [diff] [review]
mozharness_read_buildbot_config.diff

This fixes an issue when we're explicit about an action rather than running the default ones (e.g. I only use --run-tests)
Attachment #8491568 - Flags: review?(jlund)

Comment 9

3 years ago
Comment on attachment 8491568 [details] [diff] [review]
mozharness_read_buildbot_config.diff

lgtm: I'm guessing this is for Bug 1019962 ?
Attachment #8491568 - Flags: review?(jlund) → review+
Comment on attachment 8491484 [details] [diff] [review]
Add --jsshell-url support as well as file:// URLs

Review of attachment 8491484 [details] [diff] [review]:
-----------------------------------------------------------------

Oh sorry, I misread last patch. my bad.

::: mozharness/mozilla/testing/testbase.py
@@ -290,5 @@
> -        if self.installer_url[0] == '/':
> -            self.installer_url = 'file://%s' % self.installer_url
> -
> -        if self.test_url and self.test_url[0] == '/':
> -            self.test_url = 'file://%s' % self.test_url

I think you mentioned that this will only cover us for unix* platforms, do we want to support non cygwin win machines?
Attachment #8491484 - Flags: review?(jlund) → review+
(Assignee)

Comment 11

3 years ago
(In reply to Jordan Lund (:jlund) from comment #9)
> Comment on attachment 8491568 [details] [diff] [review]
> mozharness_read_buildbot_config.diff
> 
> lgtm: I'm guessing this is for Bug 1019962 ?
>

That is right. It was a case that I had missed.

(In reply to Jordan Lund (:jlund) from comment #10)
> Comment on attachment 8491484 [details] [diff] [review]
> Add --jsshell-url support as well as file:// URLs
> 
> Review of attachment 8491484 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Oh sorry, I misread last patch. my bad.
> 
> ::: mozharness/mozilla/testing/testbase.py
> @@ -290,5 @@
> > -        if self.installer_url[0] == '/':
> > -            self.installer_url = 'file://%s' % self.installer_url
> > -
> > -        if self.test_url and self.test_url[0] == '/':
> > -            self.test_url = 'file://%s' % self.test_url
> 
> I think you mentioned that this will only cover us for unix* platforms, do
> we want to support non cygwin win machines?

That code that I removed only worked with absolute paths for unix.
The new code should work for all platforms and for both relative and absolute paths.
(Assignee)

Updated

3 years ago
Attachment #8491484 - Flags: checked-in+
(Assignee)

Comment 12

3 years ago
Comment on attachment 8491568 [details] [diff] [review]
mozharness_read_buildbot_config.diff

https://hg.mozilla.org/build/mozharness/rev/3e7b4989f2f3

I will do a full run on Cypress.
Attachment #8491568 - Flags: checked-in+
In prod with reconfig on 2014-09-22 08:20 PT
(Assignee)

Comment 14

3 years ago
Thanks!
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

3 years ago
Created attachment 8494769 [details] [diff] [review]
local_files.diff
Attachment #8494769 - Flags: review?(jlund)

Updated

3 years ago
Attachment #8494769 - Flags: review?(jlund) → review+
(Assignee)

Updated

3 years ago
Attachment #8494769 - Flags: checked-in+
(Assignee)

Updated

3 years ago
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.