plugins and extensions moved to wrong directory by mozharness

RESOLVED FIXED in Firefox 36

Status

Testing
General
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

unspecified
mozilla37
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed, firefox38 fixed)

Details

Attachments

(4 attachments, 8 obsolete attachments)

6.34 KB, patch
armenzg
: review+
Details | Diff | Splinter Review
579 bytes, patch
armenzg
: review+
Details | Diff | Splinter Review
2.20 KB, patch
ted
: review+
Details | Diff | Splinter Review
2.00 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
Sorry if there is already a bug on this or if I am missing something, but I think the plugins and extensions are not moved from the test package to the right directory. The target directory is defined in [1] like so:

abs_app_plugins_dir = os.path.join(abs_app_dir, 'plugins')
abs_app_extensions_dir = os.path.join(abs_app_dir, 'extensions')

which queries abs_app_dir, defined this way (line 219):

self.abs_app_dir = os.path.dirname(self.binary_path)

which is more than likely Contents/MacOS. So two questions:

1) Should this be Contents/Resources instead?
2) Will putting files there break the signature, given extensions and plugins can/will be binary? If so, does this affect running the tests?



[1] https://github.com/mozilla/build-mozharness/blob/master/scripts/desktop_unittest.py#L410
(Assignee)

Updated

4 years ago
Blocks: 1083374
Adding information.

From dev.platform:

"On 8/28/14, 1:04 AM, Dave Townsend wrote:
>
> If my reading of the patches are correct then the extension manager will
> start looking in the new location in the app bundle for extensions
> (Contents/Resources/browser/extensions) automatically. We'll have to
> support this as that is where the default theme is to be found.

We should support this as little as possible. If anyone actually *did*
drop extensions into that location, it may break the application
signature for users.  And even worse, the signature break might not show
up immediately, but whenever MacOS decides to re-check the signature.

I suggest that to the extent we have to have a default theme there, we
should explicitly limit this location to only the default theme.

--BDS"

On the other hand, OS X should only check the signature after a fresh install, so the fact that extensions break the signature after you've used the application probably won't be that big a deal, since the signature won't get checked again. Of course, it would be better if that didn't happen.

According to https://developer.mozilla.org/en-US/Add-ons/Installing_extensions, there is a specific location to install extensions, so perhaps we are suppose to use that instead?
Is this causing test jobs to fail somewhere?  If so, a link would be helpful.
(Assignee)

Comment 4

4 years ago
Specifically in that log, click on any test failure and notice:

Loading/builds/slave/talos-slave/test/build/application/Daily.app/Contents/Resources/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/chrome.manifest

Notice how it (correctly) checks in Contents/Resources. This manifest is likely not found since the mozharness code copies to Contents/MacOS, therefore subsequent loads of calUtils.jsm throw NS_ERROR_NOT_AVAILABLE.

Thanks for pitching in Josiah!
(In reply to Philipp Kewisch [:Fallen] from comment #0)
> Sorry if there is already a bug on this

Bug 1080338 seems related, at least.

> or if I am missing something, but I
> think the plugins and extensions are not moved from the test package to the
> right directory. The target directory is defined in [1] like so:
> 
> abs_app_plugins_dir = os.path.join(abs_app_dir, 'plugins')
> abs_app_extensions_dir = os.path.join(abs_app_dir, 'extensions')
> 
> which queries abs_app_dir, defined this way (line 219):
> 
> self.abs_app_dir = os.path.dirname(self.binary_path)
> 
> which is more than likely Contents/MacOS. So two questions:
> 
> 1) Should this be Contents/Resources instead?

Yes

> 2) Will putting files there break the signature, given extensions and
> plugins can/will be binary?

Yes. Not because those files will be binary, but because they weren't signed along with the application bundle.

> If so, does this affect running the tests?

Possibly, if the test system is set up with default Gatekeeper settings. Henrik, do you know what's expected to happen here?


(In reply to Josiah Bruner [:JosiahOne] (needinfo > CC) from comment #1)
> Adding information.
> 
> From dev.platform:
> 
> "On 8/28/14, 1:04 AM, Dave Townsend wrote:
> >
> > If my reading of the patches are correct then the extension manager will
> > start looking in the new location in the app bundle for extensions
> > (Contents/Resources/browser/extensions) automatically. We'll have to
> > support this as that is where the default theme is to be found.
> 
> We should support this as little as possible. If anyone actually *did*
> drop extensions into that location, it may break the application
> signature for users.  And even worse, the signature break might not show
> up immediately, but whenever MacOS decides to re-check the signature.
> 
> I suggest that to the extent we have to have a default theme there, we
> should explicitly limit this location to only the default theme.
> 
> --BDS"
> 
> On the other hand, OS X should only check the signature after a fresh
> install, so the fact that extensions break the signature after you've used
> the application probably won't be that big a deal, since the signature won't
> get checked again. Of course, it would be better if that didn't happen.

This is true for now, but it will most likely not stay this way. The use of various native APIs will force the signature to be rechecked. One API that keeps being mentioned is the Keychain API. Once we use the Keychain (or similar) API, it will be necessary to ensure a proper signature even after the first start. We should not assume that breaking the signature after a first run is okay.

> According to
> https://developer.mozilla.org/en-US/Add-ons/Installing_extensions, there is
> a specific location to install extensions, so perhaps we are suppose to use
> that instead?

It looks like that'd be the ideal solution. This would avoid the signature being broken during tests.
Flags: needinfo?(hskupin)
(Assignee)

Comment 6

4 years ago
(In reply to Stephen Pohl [:spohl] from comment #5)
> > According to
> > https://developer.mozilla.org/en-US/Add-ons/Installing_extensions, there is
> > a specific location to install extensions, so perhaps we are suppose to use
> > that instead?
> 
> It looks like that'd be the ideal solution. This would avoid the signature
> being broken during tests.

I think this install location is disabled by default? I haven't actually checked, but I recall that some install locations are disabled and only a few select locations actually work.

This might be an easy fix for the test runners, but I'm not sure it can serve as a full replacement. The extension would be provided for all instances of Thunderbird/Firefox, even though its only intended for that specific application. If more than one app version is installed and run on the machine, then there will be a conflict if the extension works only for one of those versions.

I'm also worried we will have to introduce special casing in mozharness, since Thunderbird puts its profile data in ~/Library/Thunderbird and Firefox uses ~/Library/Application Support/Firefox. Then there is the issue about cleanup, and possibly failing cleanup blocking subsequent builds. I believe its at least the case that a machine will not run multiple builds at once?

Also, does this user-wide install work for plugins?
(In reply to Stephen Pohl [:spohl] from comment #5)
> Possibly, if the test system is set up with default Gatekeeper settings.
> Henrik, do you know what's expected to happen here?

Sorry but I do not know that much about the test archive and how it should work.
Flags: needinfo?(hskupin)
(Assignee)

Comment 9

4 years ago
Stephen, how do we proceed here?
Flags: needinfo?(spohl.mozilla.bugs)
I think we should adjust the target directory for plugins and extensions to be Contents/Resources. We should then test and see if Gatekeeper is giving us any trouble, by not letting us run the tests due to a broken signature for example. If that's the case, we may have to explore the option of not installing into the .app bundle but in the user profile instead.

If the patch in bug 1080338 is helpful here, by all means feel free to use it or land it while keeping in mind bug 1080338 comment 5 and bug 1080338 comment 6.
Flags: needinfo?(spohl.mozilla.bugs)
(Assignee)

Comment 11

4 years ago
Created attachment 8509726 [details] [diff] [review]
mozharness-extensions-dir.diff

How is this? I've also moved over the components directory, I think that is also needed. I kept the xpcshell binary in the MacOS directory though.

I did read the comments you referenced, but I personally am against making this another config option. The directory for this kind of stuff is not going to change around often, and I don't think its necessary to introduce yet another config option for this.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #8509726 - Flags: feedback?(spohl.mozilla.bugs)
(Assignee)

Comment 12

4 years ago
(Totally untested because I haven't yet managed to set up tests locally, hence the f? instead of r?)
Comment on attachment 8509726 [details] [diff] [review]
mozharness-extensions-dir.diff

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

This looks great! One minor comment below.

Once tested, you will want to get a review from an appropriate module owner (and someone who's definitely more familiar with our test python scripts than me).

If you feel so inclined, it'd be great if you could also look into bug 1080338. I'm assuming that once you have your test environment set up, it should be pretty straightforward to fix that as well.

::: scripts/desktop_unittest.py
@@ +229,5 @@
> +            return self.abs_res_dir
> +
> +        abs_app_dir = self.query_abs_app_dir()
> +        if mozinfo.isMac:
> +            self.abs_res_dir = os.path.normpath(os.path.join(abs_app_dir, "..", "Resources"))

We've typically been using os.path.join(os.path.dirname(abs_app_dir), "Resources") instead.
Attachment #8509726 - Flags: feedback?(spohl.mozilla.bugs) → feedback+
One issue I see here, is this going to be backwards compatible with the ESR branches - i.e. not all branches have moved to the new layout yet, so we need to support both.
(Assignee)

Comment 15

4 years ago
You mean branches not using mozharness? The code would be different of course, but it should be easy at well.
(Assignee)

Comment 16

4 years ago
Created attachment 8515462 [details] [diff] [review]
Fix - v2

I've had some trouble getting mozharness to run locally, but I've made a few small bugfixes on top of the other patch. Unfortunately every time I ask in #releng or #automation, no one is around that could help me. Maybe someone who knows how to run mozharness locally can ping me when they are available, or we can run this on the mozharness try branch?

My latest error is:

00:17:28    FATAL - The in-tree configuration file '/Users/kewisch/mozilla/hg/mozharness/build/tests/config/mozharness/mac_config.py' does not exist!It must be added to 'gecko/testing/config/mozharness/mac_config.py'. See bug 1035551 for more details.

The mentioned bug hasn't helped me find the solution, and copying the file to the requested location doesn't help since it gets clobbered on each run. Trying --no-clobber gives me other errors. Here is how I execute it:

python scripts/desktop_unittest.py --cfg unittests/mac_unittest.py --xpcshell-suite xpcshell --blob-upload-branch comm-central --download-symbols true --virtualenv /usr/local/bin/virtualenv
(Assignee)

Updated

4 years ago
Attachment #8509726 - Attachment is obsolete: true
Can you attach a log which includes the failures?
(Assignee)

Comment 18

4 years ago
Created attachment 8516151 [details]
mozharness.log

Here is the log, this run without the download disabled since I've already done that.
(Assignee)

Comment 19

4 years ago
To get it to go this far, I also had to make a few small changes in mozharness/mozilla/mozbase.py

@@ -28,12 +28,12 @@
         mozbase_dir = os.path.join('tests', 'mozbase')
         self.register_virtualenv_module(
             'manifestparser',
-            url=os.path.join(mozbase_dir, 'manifestdestiny')
+            url='manifestdestiny'
         )
 
         for m in ('mozfile', 'mozlog', 'mozinfo', 'moznetwork', 'mozhttpd',
                   'mozcrash', 'mozinstall', 'mozdevice', 'mozprofile',
                   'mozprocess', 'mozrunner'):
             self.register_virtualenv_module(
-                m, url=os.path.join(mozbase_dir, m)
+                m, url=m
             )
Attachment #8516151 - Attachment mime type: text/x-log → text/plain
The mozharness config you're using, configs/unittests/mac_unittest.py, specifies an in-tree config file of config/mozharness/mac_config.py.  In mozilla-central, this file is http://dxr.mozilla.org/mozilla-central/source/testing/config/mozharness/mac_config.py.

You'll need to add a corresponding file in comm-central, presumably, unless you want to use a different mozharness script that works differently from the Firefox/mozilla-central one.
Note that the path, "config/mozharness/mac_config.py", is relative to the tests.zip package produced for Firefox.  Not sure how it works with comm-central.
(Assignee)

Comment 22

4 years ago
Ok, I have it running now, the problem was that I added --no-download-and-extract because I didn't want to download the tests package on each run, but the clobber step removed everything. By the way, mixing --no-download-and-extract with --no-clobber doesn't help either, it then fails somewhere else.

> We've typically been using os.path.join(os.path.dirname(abs_app_dir), "Resources") instead.
This gives me Contents/MacOS/Resources, I've stuck with what I had before.


With my local patch, all calendar tests run again and the plugins, components and extensions are copied to Contents/Resources. 

There is still one issue with the components/ directory though. This is the one that contains httpd.js and friends. Should that be copied into Contents/Resources/components or Contents/MacOS/components? Right now its being copied into the latter, if I move it into the former then the file cannot be loaded because runxpcshelltests.py doesn't handle this. If it needs to go in Contents/Resources, then we need to fix this in the test runner.
(Assignee)

Updated

4 years ago
Flags: needinfo?(spohl.mozilla.bugs)
(In reply to Philipp Kewisch [:Fallen] from comment #22)
> > We've typically been using os.path.join(os.path.dirname(abs_app_dir), "Resources") instead.
> This gives me Contents/MacOS/Resources, I've stuck with what I had before.

Huh? What's abs_app_dir then?

> There is still one issue with the components/ directory though. This is the
> one that contains httpd.js and friends. Should that be copied into
> Contents/Resources/components or Contents/MacOS/components? Right now its
> being copied into the latter, if I move it into the former then the file
> cannot be loaded because runxpcshelltests.py doesn't handle this. If it
> needs to go in Contents/Resources, then we need to fix this in the test
> runner.

The components directory should be in Contents/Resources, so runxpcshelltests.py should be updated.
Flags: needinfo?(spohl.mozilla.bugs)
(Assignee)

Comment 24

4 years ago
(In reply to Stephen Pohl [:spohl] from comment #23)
> (In reply to Philipp Kewisch [:Fallen] from comment #22)
> > > We've typically been using os.path.join(os.path.dirname(abs_app_dir), "Resources") instead.
> > This gives me Contents/MacOS/Resources, I've stuck with what I had before.
> 
> Huh? What's abs_app_dir then?

Sorry, my fault. Your line works, I just changed it wrong locally. Using what you mentioned.


> The components directory should be in Contents/Resources, so
> runxpcshelltests.py should be updated.

Ok, will also update that and attach the patch here. Who would be a good reviewer?
(In reply to Philipp Kewisch [:Fallen] from comment #24)
> > The components directory should be in Contents/Resources, so
> > runxpcshelltests.py should be updated.
> 
> Ok, will also update that and attach the patch here. Who would be a good
> reviewer?

Going solely by https://wiki.mozilla.org/Modules/All#Core, it looks like :ted or :jmaher would be good reviewers.
(Assignee)

Comment 26

4 years ago
Created attachment 8518343 [details] [diff] [review]
mozharness changes - v3

Ok, I've tested this locally plus the runxpcshelltests.py changes I will upload in a second and all tests pass except for a few network tests failing because my mac firewall blocked them.

I didn't run anything but xpcshell tests with the Thunderbird tests package, and also didn't test other platforms, but I trust we can run this on some sort of mozharness try, I believe there was a branch set up for it.

Ted, if you'd like someone else to review please forward accordingly.
Attachment #8515462 - Attachment is obsolete: true
Attachment #8516151 - Attachment is obsolete: true
Attachment #8518343 - Flags: review?(ted)
(Assignee)

Comment 27

4 years ago
Created attachment 8518344 [details] [diff] [review]
m-c changes - v1
Attachment #8518344 - Flags: review?(ted)
Philipp, did you try this on something like esr where the package hasn't been changed?
Flags: needinfo?(philipp)
(Assignee)

Comment 29

4 years ago
ESR should probably use a different branch of mozharness where this patch is not applied, or we need to make the subdir to the res dir configurable using the in-tree config. Given its mac only, I'd probably prefer the branching. What do folks think?
Flags: needinfo?(philipp)
We have not used a branched model for mozharness anywhere to date.  We generally handle these kinds of differences via config options.
(Assignee)

Comment 31

4 years ago
Created attachment 8518765 [details] [diff] [review]
mozharness changes - v4

Ok, here is the patch with a slight change that adds a treeconfig option "mac_res_subdir" which defaults to "Resources" but we can set to "MacOS" on ESR.
Attachment #8518343 - Attachment is obsolete: true
Attachment #8518343 - Flags: review?(ted)
Attachment #8518765 - Flags: review?(ted)

Comment 32

4 years ago
ted: review ping. The tb mac tree is red/orange due to this (bug 1083374), so it would be nice to get it fixed asap.
Comment on attachment 8518344 [details] [diff] [review]
m-c changes - v1

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

Sorry for the delay.
Attachment #8518344 - Flags: review?(ted) → review+
I'm happy to review the mozharness change, however, can we push to ash-mozharness [1] and push m-c to ash [2]? (so it triggers a new set of builds and tests your code).

I want to make sure that we don't regress desktop.

[1] http://hg.mozilla.org/build/ash-mozharness
[2] https://hg.mozilla.org/projects/ash
Attachment #8518765 - Flags: review?(ted) → review?(armenzg)
(Assignee)

Comment 35

4 years ago
Is this just a matter of pushing to the branch, or do I have to do something special? I've never used mozilla's project branches before.
(Assignee)

Comment 37

4 years ago
It seems a lot of things are failing. For some I am not sure if its my fault, but the most obvious issue is that mozinfo is not being found. Should mozinfo be available and is this just a config issue? If so what do I have to do? Alternatively, I could check sys.platform == "darwin" if importing mozinfo is not an option.
Flags: needinfo?(armenzg)
Try to push to try and see if you get the same issues. Too many people are pushing to Ash and it is a big mess.

The other option is to wait a bit until we have support for mozharness on Try (which should come out this week).

In the new shiny future, you will be able to have your own user mozharness repo and push to try to test changes in there.
I will soon be blogging about it.
Flags: needinfo?(armenzg)
Comment on attachment 8518765 [details] [diff] [review]
mozharness changes - v4

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

Thank you for the patch. I'm sorry that mozharness is a bit messy to hack!

::: scripts/desktop_unittest.py
@@ +16,4 @@
>  import copy
>  import shutil
>  import glob
> +import mozinfo

In mozharness you can't simply import one of the mozbase modules and use them.

Some of the basic functionality can be found in mozharness; see below.

@@ +234,5 @@
> +            raise Exception(res_subdir)
> +            self.abs_res_dir = os.path.join(os.path.dirname(abs_app_dir), res_subdir)
> +        else:
> +            self.abs_res_dir = abs_app_dir
> +        return self.abs_res_dir

You can see a function called _is_darwin in here that you can use:
http://hg.mozilla.org/build/mozharness/file/default/mozharness/base/script.py#l131

Please add this functionality inside of query_abs_dirs and call is where needed like this:
 dirs = self.query_abs_dirs()
 dirs["abs_res_dir"]

@@ +292,4 @@
>                  'binary_path': self.binary_path,
>                  'symbols_path': self._query_symbols_url(),
>                  'abs_app_dir': abs_app_dir,
> +                'abs_res_dir': abs_res_dir,

Not sure if we need this in here.

This is normally needed if there is a parameter that needs substitution.
For instance under here [1] we have values that need substitution, e.g.:
     6     "reftest_options": [
     7         "--appname=%(binary_path)s", "--utility-path=tests/bin",
     8         "--extra-profile-file=tests/bin/plugins", "--symbols-path=%(symbols_path)s"
     9     ],

[1] http://hg.mozilla.org/mozilla-central/file/b8240bb9ae4f/testing/config/mozharness/mac_config.py#l7
Attachment #8518765 - Flags: review?(armenzg) → review-
(Assignee)

Comment 40

4 years ago
(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #39)
> @@ +234,5 @@
> > +            raise Exception(res_subdir)
> > +            self.abs_res_dir = os.path.join(os.path.dirname(abs_app_dir), res_subdir)
> > +        else:
> > +            self.abs_res_dir = abs_app_dir
> > +        return self.abs_res_dir
> 
> You can see a function called _is_darwin in here that you can use:
> http://hg.mozilla.org/build/mozharness/file/default/mozharness/base/script.
> py#l131
Great, this is a good alternative.

> 
> Please add this functionality inside of query_abs_dirs and call is where
> needed like this:
>  dirs = self.query_abs_dirs()
>  dirs["abs_res_dir"]
So you are saying I should build the res dir inside query_abs_dirs? Doesn't the same comment apply as for the app dir?

        """We can't set this in advance, because OSX install directories
        change depending on branding and opt/debug.
        """

If query_abs_dirs is called sufficiently late I agree it makes sense, but in that case I should probably also migrate query_abs_app_dir(). What do you think?


> 
> @@ +292,4 @@
> >                  'binary_path': self.binary_path,
> >                  'symbols_path': self._query_symbols_url(),
> >                  'abs_app_dir': abs_app_dir,
> > +                'abs_res_dir': abs_res_dir,
> 
> Not sure if we need this in here.
> 
> This is normally needed if there is a parameter that needs substitution.
> For instance under here [1] we have values that need substitution, e.g.:
I think this is actually a parameter that needs substitution. ESR 31 builds of Thunderbird did not switch to the new Contents/Resources/ directory, so for these builds we need to set abs_res_dir = abs_app_dir.
(Assignee)

Updated

4 years ago
Flags: needinfo?(armenzg)
By the way, you should be able to test these changes locally by trying a job like mochitest but with --cfg developer_config.py. That will require adding --test-url and --installer-url.
You can read more in here:
https://wiki.mozilla.org/ReleaseEngineering/Mozharness/How_to_run_tests_as_a_developer

On another note, you can go ahead and try to set up a user repo for mozharness like this:
http://hg.mozilla.org/users/armenzg_mozilla.com/mozharness/

Once you have that set up you should be able to test on try your user repo changes by changing testing/mozharness/mozharness.json.

That will be possible in the next 24-48 hours once my change makes it into the production branch:
http://hg.mozilla.org/build/buildbot-configs/graph (96a1b24e0067)
http://armenzg.blogspot.ca/2014/11/pinning-mozharness-from-in-tree-aka.html

(In reply to Philipp Kewisch [:Fallen] from comment #40)
> (In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from
> comment #39)

> > 
> > Please add this functionality inside of query_abs_dirs and call is where
> > needed like this:
> >  dirs = self.query_abs_dirs()
> >  dirs["abs_res_dir"]
> So you are saying I should build the res dir inside query_abs_dirs? Doesn't
> the same comment apply as for the app dir?
> 
From the note in the function it seems that it can only be determined upon runtime since it differs from opt to debug and on branding.

>         """We can't set this in advance, because OSX install directories
>         change depending on branding and opt/debug.
>         """
> 
> If query_abs_dirs is called sufficiently late I agree it makes sense, but in
> that case I should probably also migrate query_abs_app_dir(). What do you
> think?
> 

I think it is a matter of trying and see if it works or not. If you can then great!

> 
> > 
> > @@ +292,4 @@
> > >                  'binary_path': self.binary_path,
> > >                  'symbols_path': self._query_symbols_url(),
> > >                  'abs_app_dir': abs_app_dir,
> > > +                'abs_res_dir': abs_res_dir,
> > 
> > Not sure if we need this in here.
> > 
> > This is normally needed if there is a parameter that needs substitution.
> > For instance under here [1] we have values that need substitution, e.g.:
> I think this is actually a parameter that needs substitution. ESR 31 builds
> of Thunderbird did not switch to the new Contents/Resources/ directory, so
> for these builds we need to set abs_res_dir = abs_app_dir.
Flags: needinfo?(armenzg)
(Assignee)

Comment 42

4 years ago
(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #41)
> You can read more in here:
> https://wiki.mozilla.org/ReleaseEngineering/Mozharness/
> How_to_run_tests_as_a_developer
Thanks for linking this one. I locally executed the command directly from the logs without developer config before and it was very troublesome.

> Once you have that set up you should be able to test on try your user repo
> changes by changing testing/mozharness/mozharness.json.
Great, I'll give it a try (no pun intended)


> > > Please add this functionality inside of query_abs_dirs and call is where
> > > needed like this:
> > >  dirs = self.query_abs_dirs()
> > >  dirs["abs_res_dir"]
I've tried this, but it errors out early. This code is called before binary_path is set, hence it errors out:

http://hg.mozilla.org/build/mozharness/file/0117d7a3ab8c/mozharness/base/script.py#l1328

The only thing I could do is do the detection at the top of query_abs_dirs, before it checks if self.abs_dirs exists. This would mean an extra check if self.abs_dirs is not None and 'abs_app_dir' not in self.abs_dir. Alternatively, I could create a new method that works like query_abs_dirs named query_abs_bin_dirs(), returning an object with abs_app_dir and abs_res_dir. What do you prefer?

> > > @@ +292,4 @@
> > > >                  'binary_path': self.binary_path,
> > > >                  'symbols_path': self._query_symbols_url(),
> > > >                  'abs_app_dir': abs_app_dir,
> > > > +                'abs_res_dir': abs_res_dir,
> > > 
> > > Not sure if we need this in here.
I've double checked this and we might not actually need it, the ESR builds use a different key in the in-tree config. I don't think it hurts to be added though.
Flags: needinfo?(armenzg)
(In reply to Philipp Kewisch [:Fallen] from comment #42)
> (In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from
> comment #41)
> 
> > > > Please add this functionality inside of query_abs_dirs and call is where
> > > > needed like this:
> > > >  dirs = self.query_abs_dirs()
> > > >  dirs["abs_res_dir"]
> I've tried this, but it errors out early. This code is called before
> binary_path is set, hence it errors out:
> 
> http://hg.mozilla.org/build/mozharness/file/0117d7a3ab8c/mozharness/base/
> script.py#l1328
> 
> The only thing I could do is do the detection at the top of query_abs_dirs,
> before it checks if self.abs_dirs exists. This would mean an extra check if
> self.abs_dirs is not None and 'abs_app_dir' not in self.abs_dir.
> Alternatively, I could create a new method that works like query_abs_dirs
> named query_abs_bin_dirs(), returning an object with abs_app_dir and
> abs_res_dir. What do you prefer?
> 
Go with your original approach rather than inside of query_abds_dirs().
My apologies for diverting you!

> > > > @@ +292,4 @@
> > > > >                  'binary_path': self.binary_path,
> > > > >                  'symbols_path': self._query_symbols_url(),
> > > > >                  'abs_app_dir': abs_app_dir,
> > > > > +                'abs_res_dir': abs_res_dir,
> > > > 
> > > > Not sure if we need this in here.
> I've double checked this and we might not actually need it, the ESR builds
> use a different key in the in-tree config. I don't think it hurts to be
> added though.

Please leave out; let's see it in action on a try push.

Thanks again!
Flags: needinfo?(armenzg)
(Assignee)

Comment 44

4 years ago
Created attachment 8529190 [details] [diff] [review]
mozharness changes - v5

No worries about the diversion, it would have been a nice consolidation if it had worked! Here is the updated patch, plus a shiny new tryserver run:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ed57115f7e16
Attachment #8518765 - Attachment is obsolete: true
Attachment #8529190 - Flags: review?(armenzg)
Comment on attachment 8529190 [details] [diff] [review]
mozharness changes - v5

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

If you want to put this patch to the test on Try, you also have to modify testing/mozharness/mozharness.json to point to your user repo.
You would also have to land this patch on your user repo.

I will r- for now since I would like the small changes to happen as well as seeing a try push which tests this mozharness code (as explained above).

::: scripts/desktop_unittest.py
@@ +157,4 @@
>          self.installer_path = c.get('installer_path')
>          self.binary_path = c.get('binary_path')
>          self.abs_app_dir = None
> +        self.abs_res_dir = None

Can you please change the variable name to abs_resources_dir?
Can you also please add a note that it only applies to Mac?

@@ +226,5 @@
> +        if self.abs_res_dir:
> +            return self.abs_res_dir
> +
> +        abs_app_dir = self.query_abs_app_dir()
> +        if self.is_darwin():

Code has landed this morning which changes this to be "_is_darwin()" instead of "is_darwin()".

@@ +227,5 @@
> +            return self.abs_res_dir
> +
> +        abs_app_dir = self.query_abs_app_dir()
> +        if self.is_darwin():
> +            res_subdir = self.tree_config.get("mac_res_subdir", "Resources")

I don't see this value in the tree: http://dxr.mozilla.org/mozilla-central/search?q=mac_res_subdir&case=true

@@ +424,5 @@
>          abs_app_dir = self.query_abs_app_dir()
> +        abs_res_dir = self.query_abs_res_dir()
> +        abs_res_components_dir = os.path.join(abs_res_dir, 'components')
> +        abs_res_plugins_dir = os.path.join(abs_res_dir, 'plugins')
> +        abs_res_extensions_dir = os.path.join(abs_res_dir, 'extensions')

Do we need to rename abs_app_* to abs_res_*?
Isn't the Resources directory something that it only appears on Mac installs?

I could be wrong. I would like to know.

@@ +478,4 @@
>                  cmd = abs_base_cmd[:]
>                  replace_dict = {
>                      'abs_app_dir': abs_app_dir,
> +                    'abs_res_dir': abs_res_dir,

Unless there is abs_res_dir in the in-tree mozharness configs then there is no need to add this.

See: http://dxr.mozilla.org/mozilla-central/search?q=abs_app_dir&case=true&redirect=true

This is a mechanism to substitute variables once determined during execution.
Attachment #8529190 - Flags: review?(armenzg) → review-
(Assignee)

Comment 46

4 years ago
(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #45)
> Comment on attachment 8529190 [details] [diff] [review]
> mozharness changes - v5
> 
> Review of attachment 8529190 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If you want to put this patch to the test on Try, you also have to modify
> testing/mozharness/mozharness.json to point to your user repo.
> You would also have to land this patch on your user repo.

I did this in the changeset that contains the try message, sorry for not being more explicit.


> Can you please change the variable name to abs_resources_dir?
Consequently I'd also have to rename abs_app_dir to abs_application_dir, but I'd rather not. Really want me to use abs_resources_dir?


> Can you also please add a note that it only applies to Mac?
Ok, will do.


> Code has landed this morning which changes this to be "_is_darwin()" instead
> of "is_darwin()".
Ah, then my try run will probably fail nevertheless :-)


> > +            res_subdir = self.tree_config.get("mac_res_subdir", "Resources")
> 
> I don't see this value in the tree:
> http://dxr.mozilla.org/mozilla-central/search?q=mac_res_subdir&case=true
Its not in the tree yet, but will be used for Thunderbird ESR31 builds and set to "MacOS" or %(abs_app_dir)s there, see comment 28 - 31.


> 
> @@ +424,5 @@
> >          abs_app_dir = self.query_abs_app_dir()
> > +        abs_res_dir = self.query_abs_res_dir()
> > +        abs_res_components_dir = os.path.join(abs_res_dir, 'components')
> > +        abs_res_plugins_dir = os.path.join(abs_res_dir, 'plugins')
> > +        abs_res_extensions_dir = os.path.join(abs_res_dir, 'extensions')
> 
> Do we need to rename abs_app_* to abs_res_*?
> Isn't the Resources directory something that it only appears on Mac installs?
Yes, the Resources directory is only something for mac, but since the OSX signing changes the app dir (MacOS) and res dir (Resources) are different. For non-mac platforms, both variables will point to the directory containing the binaries but we need a way to differentiate the dir that binaries will be in and the dir that extensions and plugins will go into.

> Unless there is abs_res_dir in the in-tree mozharness configs then there is
> no need to add this.
Ok, I'll remove this one too.
Flags: needinfo?(armenzg)
(In reply to Philipp Kewisch [:Fallen] from comment #46)
> (In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from
> comment #45)
> > Comment on attachment 8529190 [details] [diff] [review]
> > mozharness changes - v5
> > 
> > Review of attachment 8529190 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > If you want to put this patch to the test on Try, you also have to modify
> > testing/mozharness/mozharness.json to point to your user repo.
> > You would also have to land this patch on your user repo.
> 
> I did this in the changeset that contains the try message, sorry for not
> being more explicit.
> 

My apologies! I missed that.

Unfortunately the code that I was expecting to land and be live has not happened until 15 minutes ago.
Right now your code has been tested against the official mozharness repo.

Would you mind pushing again the same thing?

The other option is to go and re-trigger every desktop test job on that try push.

You can verify that the job has used your user repo by clicking on a job and looking at the summary to the left bottom:
> script_revlink https://hg.mozilla.org/build/mozharness/rev/e1b33eec379f

Once you get the try push running your user repo code I will review the push.
I will review this on Friday as I will be on PTO tomorrow.

> > > +            res_subdir = self.tree_config.get("mac_res_subdir", "Resources")
> > 
> > I don't see this value in the tree:
> > http://dxr.mozilla.org/mozilla-central/search?q=mac_res_subdir&case=true
> Its not in the tree yet, but will be used for Thunderbird ESR31 builds and
> set to "MacOS" or %(abs_app_dir)s there, see comment 28 - 31.
> 
I wonder how we can then test this code path.
I guess once it lands on ESR31?

> 
> > 
> > @@ +424,5 @@
> > >          abs_app_dir = self.query_abs_app_dir()
> > > +        abs_res_dir = self.query_abs_res_dir()
> > > +        abs_res_components_dir = os.path.join(abs_res_dir, 'components')
> > > +        abs_res_plugins_dir = os.path.join(abs_res_dir, 'plugins')
> > > +        abs_res_extensions_dir = os.path.join(abs_res_dir, 'extensions')
> > 
> > Do we need to rename abs_app_* to abs_res_*?
> > Isn't the Resources directory something that it only appears on Mac installs?
> Yes, the Resources directory is only something for mac, but since the OSX
> signing changes the app dir (MacOS) and res dir (Resources) are different.
> For non-mac platforms, both variables will point to the directory containing
> the binaries but we need a way to differentiate the dir that binaries will
> be in and the dir that extensions and plugins will go into.
> 
So we only need those variables when running on Mac? but we can use abs_app_dir for the others?

It just becomes a bit confusing to read and we set that implicit value "sneakly" (as someone used for one of my patches(:
> else:
>   self.abs_res_dir = abs_app_dir

I guess it is fine. I won't over-rotate on this.

> > Unless there is abs_res_dir in the in-tree mozharness configs then there is
> > no need to add this.
> Ok, I'll remove this one too.

If you say it will be used for esr31 then we will need it. I think.
(Assignee)

Comment 48

4 years ago
(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #47)
> (In reply to Philipp Kewisch [:Fallen] from comment #46)
> > (In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from
> > comment #45)
> > > Comment on attachment 8529190 [details] [diff] [review]
> > > mozharness changes - v5
> > > 
> > > Review of attachment 8529190 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > If you want to put this patch to the test on Try, you also have to modify
> > > testing/mozharness/mozharness.json to point to your user repo.
> > > You would also have to land this patch on your user repo.
> > 
> > I did this in the changeset that contains the try message, sorry for not
> > being more explicit.
> > 
> 
> My apologies! I missed that.
> 
> Unfortunately the code that I was expecting to land and be live has not
> happened until 15 minutes ago.
> Right now your code has been tested against the official mozharness repo.
> 
> Would you mind pushing again the same thing?
> 
> The other option is to go and re-trigger every desktop test job on that try
> push.
To avoid forgetting some desktop test job, I pushed a new job:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d50694844765



> > Its not in the tree yet, but will be used for Thunderbird ESR31 builds and
> > set to "MacOS" or %(abs_app_dir)s there, see comment 28 - 31.
> > 
> I wonder how we can then test this code path.
> I guess once it lands on ESR31?
Yeah, I don't see a good way to test that unless we can testrun custom mozharness with ESR31 builds in combination with the in-tree config change.


> > 
> So we only need those variables when running on Mac? but we can use
> abs_app_dir for the others?
Exactly. We could use _is_darwin() in more places, but I'd rather just make the split in the variable used instead of adding in if self._is_darwin() everywhere.

> 
> It just becomes a bit confusing to read and we set that implicit value
> "sneakly" (as someone used for one of my patches(:
> > else:
> >   self.abs_res_dir = abs_app_dir
> 
> I guess it is fine. I won't over-rotate on this.
Ok, just let me know if you want me to do something differently.


> 
> > > Unless there is abs_res_dir in the in-tree mozharness configs then there is
> > > no need to add this.
> > Ok, I'll remove this one too.
> 
> If you say it will be used for esr31 then we will need it. I think.
No, it should be fine. On esr31 we need to set mac_res_subdir = "MacOS" or use %(abs_app_dir)s. We don't need %(abs_res_dir)s in there.
(Assignee)

Comment 49

4 years ago
The try run mostly passed, I'm not sure if the failures are an effect of my patches. Most of the bc1 tests failed and a few other jobs that ran into timeouts. Let me know if thats ok and I will upload a new patch with the last minor changes fixed.
Would you mind pushing one more time?
It seems that your try push is polluted with some of the infra issues from yesterday.
Wrt to the browser chrome issues I assume it was based on a bad m-c change but I would like to be sure about it.
Flags: needinfo?(armenzg)
(Assignee)

Comment 52

4 years ago
I've pushed this again without any changes. There are still some failures, but far less. I therefore think its an infra issue:  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=aabcf4a46f35
(Assignee)

Updated

4 years ago
Flags: needinfo?(armenzg)
I'm not clearing the needinfo.
Just want to say that I will have a hard time getting to this promptly as the coincidental work week will make things difficult.
I will be looking at this today. My apologies for the delay. It was difficult last week with all the flying and work week.
Comment on attachment 8529190 [details] [diff] [review]
mozharness changes - v5

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

Please land the code with the renaming of the variable and adding the comments requested.

Thanks for your help and again my apologies for the slow turn around.

::: scripts/desktop_unittest.py
@@ +157,4 @@
>          self.installer_path = c.get('installer_path')
>          self.binary_path = c.get('binary_path')
>          self.abs_app_dir = None
> +        self.abs_res_dir = None

I would like the renaming of the variable as I mentioned.

@@ +227,5 @@
> +            return self.abs_res_dir
> +
> +        abs_app_dir = self.query_abs_app_dir()
> +        if self.is_darwin():
> +            res_subdir = self.tree_config.get("mac_res_subdir", "Resources")

I understand this now.

@@ +424,5 @@
>          abs_app_dir = self.query_abs_app_dir()
> +        abs_res_dir = self.query_abs_res_dir()
> +        abs_res_components_dir = os.path.join(abs_res_dir, 'components')
> +        abs_res_plugins_dir = os.path.join(abs_res_dir, 'plugins')
> +        abs_res_extensions_dir = os.path.join(abs_res_dir, 'extensions')

Let's then leave it the same for all platforms.

Could you please add a note explaining this?

@@ +478,4 @@
>                  cmd = abs_base_cmd[:]
>                  replace_dict = {
>                      'abs_app_dir': abs_app_dir,
> +                    'abs_res_dir': abs_res_dir,

As you say, this will be needed for esr31 and it does not cause any issues in here.

Can you please add a note that this is specific to Mac?
Attachment #8529190 - Flags: review- → review+
The issues you saw on your try push, after re-triggering they went green.
The Linux 32 debug jobs are not likely to go green since they're frequently timing out.
Flags: needinfo?(armenzg)
(Assignee)

Comment 57

4 years ago
Created attachment 8534329 [details] [diff] [review]
mozharness changes - v6

(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #55)
> 
> Thanks for your help and again my apologies for the slow turn around.
No problem, thanks for the review!

> 
> ::: scripts/desktop_unittest.py
> @@ +157,4 @@
> >          self.installer_path = c.get('installer_path')
> >          self.binary_path = c.get('binary_path')
> >          self.abs_app_dir = None
> > +        self.abs_res_dir = None
> 
> I would like the renaming of the variable as I mentioned.
Re-requesting review to make sure I've renamed them as you'd like it. As mentioned earlier, if we name the variable for abs_res_dir to abs_resources_dir, then for consistency we should also rename abs_app_dir to abs_application_dir. I've renamed the variables only, not the substitutions, to not break code that relies on the old subsitutions. Is this how you expected it or did you want me to rename only the variable abs_res_dir to abs_resources_dir (keeping the substitution at abs_res_dir)?


I've added the comments about mac specific things as requested.
Attachment #8529190 - Attachment is obsolete: true
Attachment #8534329 - Flags: review?(armenzg)
Comment on attachment 8534329 [details] [diff] [review]
mozharness changes - v6

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

After looking at this please either choose to go back with abs_res or fix other variables based on the first one.

My apologies for the back and forth that this produced. Fyi I'm back to fast turnarounds for reviews.

::: scripts/desktop_unittest.py
@@ +431,5 @@
> +        # platforms abs_resources_dir will point to abs_application_dir.
> +        abs_resource_dir = self.query_abs_res_dir()
> +        abs_res_components_dir = os.path.join(abs_resource_dir, 'components')
> +        abs_res_plugins_dir = os.path.join(abs_resource_dir, 'plugins')
> +        abs_res_extensions_dir = os.path.join(abs_resource_dir, 'extensions')

I think we should keep consistency one way or the other.

I'm OK with abs_res_dir if you prefer. As long as we have the comments people should be able to realize that "res" comes from the Resources dir that only applies to Mac.
Attachment #8534329 - Flags: review?(armenzg) → review-
(Assignee)

Comment 59

4 years ago
Created attachment 8534519 [details] [diff] [review]
[checked in] mozharness changes - v7

(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #58)
> After looking at this please either choose to go back with abs_res or fix
> other variables based on the first one.
I've went back to abs_res_dir, which I prefer. I agree the comments are sufficient. Even if uncommented, someone who doesn't know what abs_res_dir is would go back to query_abs_res_dir() and read about it in the function header.

I've also gotten rid of the 'extensions/*' part in minimum_tests_zip_dirs as discussed on IRC. We only need this for Thunderbird right now and thats taken care of in thunderbird_extra.py.
Attachment #8534329 - Attachment is obsolete: true
Attachment #8534519 - Flags: review?(armenzg)
Comment on attachment 8534519 [details] [diff] [review]
[checked in] mozharness changes - v7

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

Please land once you are confident with the testing of this patch on try.

Thank you!
Attachment #8534519 - Flags: review?(armenzg) → review+
(Assignee)

Comment 61

4 years ago
I've made another test run at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f3b4f79ec1f6 and it looks like all the issues are infra issues. I've retriggered a few builds and most of them went green. Its just one Linux xpcshell ASAN test that is worrysome, but since its timeouts and there are bugs filed for same or similar issues, I think its ok.

I will land this now, after I figure out how to make sure the mozharness and m-c changes land together.
(In reply to Philipp Kewisch [:Fallen] from comment #61)
> 
> I will land this now, after I figure out how to make sure the mozharness and
> m-c changes land together.

Do they need to land together? If so we might be breaking other release branches.

If you're not sure, push an empty push to Try that still points to your mozharness repo.

On that same note, can your m-c change land before the mozharness change?

The problem in here would be if both patches need each other. It is fine if the m-c change needs the mozharness change to be live first.

I hope I made sense! :P
(Assignee)

Comment 63

4 years ago
(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #62)
> (In reply to Philipp Kewisch [:Fallen] from comment #61)
> > 
> > I will land this now, after I figure out how to make sure the mozharness and
> > m-c changes land together.
> 
> Do they need to land together? If so we might be breaking other release
> branches.
They do. All branches that don't follow the OSX packaging schema (TB ESR for example) need to have the mac_res_subdir option set in their configs. All other branches would likely need the m-c patch landed. Looks like we have a problem here :-/

I guess I could add intermediate code that looks in both locations, or have mozharness copy it into both locations, or is there a way to land it everywhere?


> 
> If you're not sure, push an empty push to Try that still points to your
> mozharness repo.
I tried this in https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=24261c7dcfb8

Unfortunately the OSX jobs fail, because its not finding the components for httpd.js.


> 
> On that same note, can your m-c change land before the mozharness change?

I'm trying this now, but I fear it will have the same problem the other way around, expecting files in the wrong directory.


> 
> The problem in here would be if both patches need each other. It is fine if
> the m-c change needs the mozharness change to be live first.
> 
> I hope I made sense! :P
The solution is to make your mozharness change support the old way and the new way.
You can hack your user repo and retry the jobs that are failing on your empty push.

Until we have bug 1110286 you won't be able to break backward compatibility.
I would not ask to land that change until January since many people are trying to wrap their quarterly goals and I would hate it if it caused some sort of unexpected tree closure.
(Assignee)

Comment 65

4 years ago
I had an idea how to unblock this without adding extra code and still not breaking anything. I think I was too brief when describing it on IRC, so here is the long version.

In addition to the patches here, branches like ESR will need the change to their in-tree config that sets mac_res_subdir = 'MacOS'. This will cause the files to be placed in their old location, the mac equivalent of the binary path.

The mac_res_subdir key is new, so if we land that in-tree config change on ALL branches (including m-c) it won't break anything for existing builds. Then we wait a few days like you normally do after uplifting and land the mozharness change. From now on, builds including the mac_res_subdir changes will not behave differently, because files will still be copied to the old location. We can check this is happening in the logs.

Once this is done, we can finally follow through with the change to m-c. We land a changeset that removes the mac_res_subdir from the in-tree config and also changes runxpcshelltests.py as in the already attached patch. This way, both builds before and after the changeset will be working.

People pushing on try that did not pick up the mac_res_subdir patch during the grace days you usually give are out of luck, but I guess its that way with any other mozharness change.

Hope this explains it better and resolves your concerns. If we can shave off a few days by lowering the grace time and maybe tree info, maybe we can get this in before everyone leaves for xmas. I really want the tests working again for Thunderbird.
(Assignee)

Comment 66

4 years ago
To prove my theory, I've made a test run [1] that has mac_res_subdir set and uses the mozharness changes that are r+ as is. It is working well, as you can see at [2] at about 23:35:34 the plugins directory is being copied to the old location Contents/MacOS while the previous try run [3] from comment 61 uses Contents/Resources at about 14:17:46. 

The only xpcshell failure in [1] seems to be a known issue, there is a bug about it. I've retriggered nevertheless.

[1] https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ea83ea57a87f
[2] http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@kewis.ch-ea83ea57a87f/try-macosx64-debug/try_snowleopard-debug_test-xpcshell-bm106-tests1-macosx-build4687.txt.gz
[3] http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mozilla@kewis.ch-f3b4f79ec1f6/try-macosx64-debug/try_snowleopard-debug_test-xpcshell-bm107-tests1-macosx-build3820.txt.gz
It seems that you got a good grasp on how mozharness-mc dependent landings comprise of. Good luck with it!
(Assignee)

Comment 68

4 years ago
Created attachment 8538873 [details] [diff] [review]
[checked in] mozharness in-tree config - v1

Here is the in-tree config bit. With your r+ I will land this as a=NPOTB on m-c, m-a and m-b. I am not sure about m-r and m-esr31 or any other branches we might need, can you advise?
Attachment #8538873 - Flags: review?(armenzg)
(Assignee)

Comment 69

4 years ago
Obviously we need m-esr31 per comment 65, but please let me know about other branches. Also, how many grace days should I plan before I land the mozharness changes?
Comment on attachment 8538873 [details] [diff] [review]
[checked in] mozharness in-tree config - v1

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

Please land it on every tree that we run Mac desktop jobs.

Thank you!
Attachment #8538873 - Flags: review?(armenzg) → review+
https://hg.mozilla.org/mozilla-central/rev/a2dc4866b44f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
(Assignee)

Comment 73

4 years ago
Sorry I didn't mark this leave-open, need to keep it that way until the remaining steps from comment 65 are taken care of.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 74

4 years ago
https://hg.mozilla.org/build/mozharness/rev/463fa1fd7497

Now we just need to merge to production, make sure it works and then we can update runxpcshelltests.py and revert the mac_res_dir config changes on m-c.
Status: REOPENED → ASSIGNED
I was asked to merge the mozharness bits to prod for this bug but I just have one question first. it appears like the location of mac_res_subdir is in two different config locations:

http://hg.mozilla.org/mozilla-central/file/a2dc4866b44f/testing/config/mozharness/mac_config.py#l12
https://hg.mozilla.org/releases/mozilla-aurora/file/40c8e653429f/testing/config/mozharness/mac_config.py#l43

I know this is to support the new m-c way vs old way however, IIUC, this seems wrong: http://hg.mozilla.org/build/mozharness/file/463fa1fd7497/scripts/desktop_unittest.py#l235

sanity check, m-c has a mac_res_subdir, but line 235 won't find it. is that intentional? are you wanting to default to 'Resources' for m-c?
Flags: needinfo?(philipp)
(Assignee)

Comment 76

3 years ago
(In reply to Jordan Lund (:jlund) from comment #75)
> I was asked to merge the mozharness bits to prod for this bug but I just
> have one question first. it appears like the location of mac_res_subdir is
> in two different config locations:
> 
> http://hg.mozilla.org/mozilla-central/file/a2dc4866b44f/testing/config/
> mozharness/mac_config.py#l12
> https://hg.mozilla.org/releases/mozilla-aurora/file/40c8e653429f/testing/
> config/mozharness/mac_config.py#l43

Yeah, bug 1070041 was pushed after all of the reviews here. Unfortunately it seems I need to make more modifications.

> sanity check, m-c has a mac_res_subdir, but line 235 won't find it. is that
> intentional? are you wanting to default to 'Resources' for m-c?
Eventually, yes. For this mozharness push m-c should also use MacOS though, moving to "Resources" needs to happen in a separate push. I will upload a new patch that handles the old and new structure.

Thanks for double checking, I'll get back to you when its ready.
Flags: needinfo?(philipp)
(Assignee)

Comment 77

3 years ago
Created attachment 8542164 [details] [diff] [review]
[checked in] mozharness in-tree config correction for m-c - v1

Unfortunately my ad-hoc changes to the config for m-c were not quite correct after bug 1070041 landed. I had though that the per-suite config was the new way to define all in-tree config options, but actually I can retrieve the mac_res_subdir option top-level.

This patch should be pushed to m-c only, the mozharness patch can be kept the way it is. I believe armenzg is away for the holidays since I haven't seen him on IRC the past few days, maybe I can find another reviewer in the meanwhile.
Attachment #8542164 - Flags: review?(armenzg)
(Assignee)

Updated

3 years ago
Attachment #8538873 - Attachment description: mozharness in-tree config - v1 → [checked in] mozharness in-tree config - v1
Attachment #8542164 - Flags: review?(armenzg) → review+
(Assignee)

Comment 80

3 years ago
I think the mozharness patch is ready for production now. Jordan, do you think you could take care?
Flags: needinfo?(jlund)
(In reply to Philipp Kewisch [:Fallen] from comment #80)
> I think the mozharness patch is ready for production now. Jordan, do you
> think you could take care?

mozharness default should be merged to production at some point today :)
Flags: needinfo?(jlund)
mozharness has been merged to production. patches are live :)
(Assignee)

Updated

3 years ago
Attachment #8534519 - Attachment description: mozharness changes - v7 → [checked in] mozharness changes - v7
(Assignee)

Updated

3 years ago
Attachment #8542164 - Attachment description: mozharness in-tree config correction for m-c - v1 → [checked in] mozharness in-tree config correction for m-c - v1
(Assignee)

Comment 83

3 years ago
I've tested the last remaining m-c patch on try with the latest mozharness which is in production and it seems all is well:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=976bea9858bd

I have therefore pushed the last patch to m-c. Thank you all for the help and support!

https://hg.mozilla.org/integration/mozilla-inbound/rev/ced934d1b13e
Keywords: leave-open
(Assignee)

Comment 84

3 years ago
Created attachment 8543445 [details] [diff] [review]
[checked in] m-c changes - v2

patch as checked in, just for the record
Attachment #8518344 - Attachment is obsolete: true
Attachment #8543445 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ced934d1b13e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago3 years ago
Resolution: --- → FIXED
See Also: → bug 1117637
(Assignee)

Comment 86

3 years ago
Comment on attachment 8543445 [details] [diff] [review]
[checked in] m-c changes - v2

I would like to backport this to beta to resolve test failures on comm-beta. Here is why:

Approval Request Comment
[Feature/regressing bug #]: bug 1053820

[User impact if declined]: Possible unrelated regressions due to busted xpcshell tests on comm-beta

[Describe test coverage new/current, TreeHerder]: Works on *-central and *-aurora builds where this patch has ridden the train, fails on https://treeherder.mozilla.org/ui/#/jobs?repo=comm-beta

[Risks and why]: Risk of bustage on desktop beta builds in the unlikely case I missed something.

[String/UUID change made/needed]: none
Attachment #8543445 - Flags: approval-mozilla-beta?
status-firefox36: --- → affected
status-firefox37: --- → fixed
status-firefox38: --- → fixed
Attachment #8543445 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 87

3 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/64fb35ee1af6
status-firefox36: affected → fixed
You need to log in before you can comment on or make changes to this bug.