Closed
Bug 1068263
Opened 10 years ago
Closed 10 years ago
Improve handling of packages locally (file:// support)
Categories
(Release Engineering :: Applications: MozharnessCore, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: armenzg, Assigned: armenzg)
Details
Attachments
(3 files, 2 obsolete files)
3.75 KB,
patch
|
jlund
:
review+
armenzg
:
checked-in+
|
Details | Diff | Splinter Review |
1.11 KB,
patch
|
jlund
:
review+
armenzg
:
checked-in+
|
Details | Diff | Splinter Review |
910 bytes,
patch
|
jlund
:
review+
armenzg
:
checked-in+
|
Details | Diff | Splinter Review |
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•10 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•10 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•10 years ago
|
||
Attachment #8490328 -
Attachment is obsolete: true
Assignee | ||
Comment 4•10 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•10 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•10 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•10 years ago
|
Attachment #8490737 -
Flags: review?(jlund)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8490737 -
Attachment is obsolete: true
Attachment #8491484 -
Flags: review?(jlund)
Assignee | ||
Comment 8•10 years ago
|
||
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•10 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 10•10 years ago
|
||
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•10 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•10 years ago
|
Attachment #8491484 -
Flags: checked-in+
Assignee | ||
Comment 12•10 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+
Comment 13•10 years ago
|
||
In prod with reconfig on 2014-09-22 08:20 PT
Assignee | ||
Comment 14•10 years ago
|
||
Thanks!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8494769 -
Flags: review?(jlund)
Updated•10 years ago
|
Attachment #8494769 -
Flags: review?(jlund) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8494769 -
Flags: checked-in+
Assignee | ||
Updated•10 years ago
|
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•