Closed Bug 1068263 Opened 10 years ago Closed 10 years ago

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

Categories

(Release Engineering :: Applications: MozharnessCore, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: armenzg, Assigned: armenzg)

Details

Attachments

(3 files, 2 obsolete files)

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)
> 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 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-
Attachment #8490328 - Attachment is obsolete: true
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
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 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
Attachment #8490737 - Flags: review?(jlund)
Attachment #8490737 - Attachment is obsolete: true
Attachment #8491484 - Flags: review?(jlund)
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 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+
(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.
Attachment #8491484 - Flags: checked-in+
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
Thanks!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch local_files.diffSplinter Review
Attachment #8494769 - Flags: review?(jlund)
Attachment #8494769 - Flags: review?(jlund) → review+
Attachment #8494769 - Flags: checked-in+
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: