Closed Bug 1239808 Opened 8 years ago Closed 8 years ago

Shared test_packages.json for Nightly builds always lists packages from last built platform

Categories

(Testing :: General, defect)

Product:
Testing
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox46 wontfix, firefox47 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(
URL
)

Details

Attachments

(4 files, 11 obsolete files)

MozReview Request: Bug 1239808 - Rename test_packages.json to include package basename as prefix. r=chmanchester, r=jlund
58 bytes, text/x-review-board-request
chmanchester
: review+
jlund
: review+
Details
MozReview Request: Bug 1239808 - Update taskcluster configs for test_package.json renaming. r=garndt
58 bytes, text/x-review-board-request
garndt
: review+
Details
MozReview Request: Bug 1239808 - Fix B2G build script and configs for test_packages.json renaming. r=chmanchester
58 bytes, text/x-review-board-request
chmanchester
: review+
Details
MozReview Request: Bug 1239808 - Fix mozharness mach command for test_packages.json retrieval. r=jlund
58 bytes, text/x-review-board-request
jlund
: review+
Details 
If you want to use the test packages url for nightly builds you will see that this is broken. Reason is that we share the folder and there can only be a single test_packages.json. As result each platform will download the tests for the last built platform, and tests will be busted. We are partly affected by this with our firefox-ui-tests.

Here an example from a linux64 host:
20:48:24     INFO - Reading from file /mozilla/code/gecko/testing/mozharness/build/test_packages.json
20:48:24     INFO - Using the following test package requirements:
20:48:24     INFO - {u'common': [u'firefox-46.0a1.en-US.win64.common.tests.zip'],
20:48:24     INFO -  u'cppunittest': [u'firefox-46.0a1.en-US.win64.common.tests.zip',
20:48:24     INFO -                   u'firefox-46.0a1.en-US.win64.cppunittest.tests.zip'],
[..]
20:48:40     INFO - We want to download this file https://ftp-ssl.mozilla.org/pub/firefox/nightly/latest-mozilla-central/firefox-46.0a1.en-US.linux-x86_64.tar.bz2

To solve this we should use the same platform prefix as for all the other test archives. Means a file name like firefox-46.0a1.en-US.linux-i686.test_packages.json should do it.

Therefore we would have to change the test packages file name:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/package-name.mk#172

And we would have to adjust mozharness to find the correct file:
https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/testbase.py#565

A change like that might also affect external CI systems which have the path hard-coded. Means we might break them. It might be wise to give an announcement on the dev.platform mailing list first.
Just to add some more information, anything related TaskCluster would not be affected given that those builds do not appear on FTP but in the TC queue.

So maybe checking the landing from Chris and adjusting all hard-coded file names to accommodate the new prefix might be enough. 

https://hg.mozilla.org/mozilla-central/rev/8deed4a4ccde
The question is if this is worth the time. No other test harness beside fx-ui-tests are running tests against nightly builds. And we don't have binaries in the test packages. As said above once we switch to TC it should not be a problem anymore.
Changing the file name in package-name.mk only seems to work pretty fine. I do not see anything failing on Treeherder:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=215d6a3960f5

That means the new filename gets propagated correctly to the test machines. 

Interestingly also for taskcluster the filename is changing to `target.test_packages.json` and that without changing `MOZ_TEST_PACKAGES_FILE_TC`. So I assume that Taskcluster is internally replacing the content of $(MOZ_INFO_BASENAME) with target.

I really wonder why we have this MOZ_TEST_PACKAGES_FILE_TC package file specification and test-packages-manifest-tc make target.

Chris or Jonas, mind giving more details? Thanks.
Flags: needinfo?(jopsen)
Flags: needinfo?(cmanchester)
Attached patch WIP v1 (obsolete) — — Splinter Review 
Here actual code which might help you both more to understand the changes and my question.
Flags: needinfo?(cmanchester)
Attachment #8712103 - Flags: feedback?(cmanchester)
(In reply to Henrik Skupin (:whimboo) from comment #3)
> Changing the file name in package-name.mk only seems to work pretty fine. I
> do not see anything failing on Treeherder:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=215d6a3960f5
> 
> That means the new filename gets propagated correctly to the test machines. 
> 
> Interestingly also for taskcluster the filename is changing to
> `target.test_packages.json` and that without changing
> `MOZ_TEST_PACKAGES_FILE_TC`. So I assume that Taskcluster is internally
> replacing the content of $(MOZ_INFO_BASENAME) with target.
> 
> I really wonder why we have this MOZ_TEST_PACKAGES_FILE_TC package file
> specification and test-packages-manifest-tc make target.
> 
> Chris or Jonas, mind giving more details? Thanks.

The test-packages-manifest-tc thing is to accommodate the uniform filenames TC expects (it writes the manifest with the simple name, so mozharness invoked by TC will find the files that are uploaded with to TC with simple filenames).

I see this has been changed to work from the MOZ_SIMPLE_PACKAGE_NAME variable, which package-name.mk actually picks up, so writing the short names in testsuite-targets.mk might no longer be necessary.
Flags: needinfo?(jopsen)
Comment on attachment 8712103 [details] [diff] [review]
WIP v1

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

Right, this is a version of mozharness support for test package manifests named per platform. But since this isn't a problem for taskcluster builds per comment 2 is this the direction we need to go?
Attachment #8712103 - Flags: feedback?(cmanchester)
(In reply to Chris Manchester [:chmanchester] from comment #5)
> I see this has been changed to work from the MOZ_SIMPLE_PACKAGE_NAME
> variable, which package-name.mk actually picks up, so writing the short
> names in testsuite-targets.mk might no longer be necessary.

Ah, that would totally explain it. Should be easy then to get the no longer used targets removed. I will have a closer look.

(In reply to Chris Manchester [:chmanchester] from comment #6)
> Right, this is a version of mozharness support for test package manifests
> named per platform. But since this isn't a problem for taskcluster builds
> per comment 2 is this the direction we need to go?

This patch will support both systems. It doesn't matter what the prefix of the build URL is. The patch simply strips off the known installer suffix and replaces it with `test_packages.json`.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> Presumably you could fetch the builds from the taskcluster index and you
> wouldn't have this problem?
> 
> Like:
> https://tools.taskcluster.net/index/artifacts/#gecko.v2.mozilla-central.
> nightly.latest.firefox/gecko.v2.mozilla-central.nightly.latest.firefox.win32-
> opt

None of our tools support taskcluster yet also because it still misses builds for beta and release. Once we see that TC gives us a way better handling across all the branches, or when the old system is about to get shutdown, we will have to get TC support implemented.

Also while checking the above link it's interesting that we do not have the simple names for indexed builds. Here we also have the full name. My try build seems to be fine:

https://tools.taskcluster.net/index/artifacts/#gecko.v2.try.revision.215d6a3960f51f2f66f40acdc016c3db2b9a3574.firefox.linux-opt
(In reply to Henrik Skupin (:whimboo) from comment #8)
> Also while checking the above link it's interesting that we do not have the
> simple names for indexed builds. Here we also have the full name. My try
> build seems to be fine:

Note that that's not a taskcluster *build*, it's a buildbot build. We just upload all of our builds to the taskcluster index nowadays. (We don't build any nightlies in taskcluster currently, nor do we do any Windows builds there yet.)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> Note that that's not a taskcluster *build*, it's a buildbot build. We just
> upload all of our builds to the taskcluster index nowadays. (We don't build
> any nightlies in taskcluster currently, nor do we do any Windows builds
> there yet.)

I see. So we do not index taskcluster builds but BBB builds then. And this is happening for mozilla-central and mozilla-aurora only for now. Thanks Ted.

So I triggered another try with the code as Chris pointed out removed. Nothing really was using it. And as I can see no single build or test job is broken due to this change:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2846133136f0
Attached patch Rename test_packages.json v1 (obsolete) — — Splinter Review 
This is the build specific changes for test packaging, which sets the new prefixed name including the removal of the unused tc settings.
Assignee: nobody → hskupin
Attachment #8712103 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8712587 - Flags: review?(ted)
Attached patch mozharness enhancements v1 (obsolete) — — Splinter Review 
mozharness updates to handle the new filename of test_packages.json for both BBB and TC jobs.
Attachment #8712589 - Flags: review?(jlund)
Attached patch mozharness enhancements v1.1 (obsolete) — — Splinter Review 
Actually we should take the same logic as we already have for the symbols URL retrieval.
Attachment #8712589 - Attachment is obsolete: true
Attachment #8712589 - Flags: review?(jlund)
Attachment #8713102 - Flags: review?(jlund)
Comment on attachment 8713102 [details] [diff] [review]
mozharness enhancements v1.1

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

I _think_ this is okay. it's been a while since I've seen any of this code. I think it would be good to get chmanchester to glance at and approve
Attachment #8713102 - Flags: review?(jlund) → review+
Comment on attachment 8713102 [details] [diff] [review]
mozharness enhancements v1.1

Sounds fine. I will request an additional review from him as best for both patches.
Attachment #8713102 - Flags: review?(cmanchester)
Attachment #8712587 - Flags: feedback?(cmanchester)
Comment on attachment 8712587 [details] [diff] [review]
Rename test_packages.json v1

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

Feel free to re-request review from me when this is updated and the other patch is finalized.

::: testing/testsuite-targets.mk
@@ -425,5 @@
> -	$(NSINSTALL) -D $(dir $(MOZ_TEST_PACKAGES_FILE_TC))
> -	$(PYTHON) $(topsrcdir)/build/gen_test_packages_manifest.py \
> -      --jsshell $(JSSHELL_NAME) \
> -      --dest-file $(MOZ_TEST_PACKAGES_FILE_TC) \
> -      --use-short-names \

Please remove the --use-short-names switch from gen_test_packages_manifest.py as well.

@@ +604,4 @@
>    stage-instrumentation-tests \
>    stage-luciddream \
>    test-packages-manifest \
> +    $(NULL)

Maybe this is just the splinter diff, but it looks like there's some weird indentation here.

::: toolkit/mozapps/installer/package-name.mk
@@ +173,1 @@
>  MOZ_TEST_PACKAGES_FILE_TC = $(DIST)/$(PKG_PATH)/test_packages_tc.json

This definition can go away too now.
Attachment #8712587 - Flags: review?(ted)
Attachment #8712587 - Flags: feedback?(cmanchester)
Comment on attachment 8713102 [details] [diff] [review]
mozharness enhancements v1.1

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

::: testing/mozharness/mozharness/mozilla/testing/testbase.py
@@ +155,3 @@
>              reference_url = self.installer_url
> +        elif self.test_packages_url:
> +            reference_url = self.test_packages_url

What's the significance of inverting the precedence between test_packages_url and installer_url? I wouldn't think the result would differ under usual circumstances.

@@ +527,5 @@
> +                    if self.installer_url.endswith(suffix):
> +                        self.test_packages_url = (self.installer_url[:-len(suffix)] +
> +                                                  '.test_packages.json'
> +                                                  )
> +                        break

This works well enough for the symbols url, but looking at package-name.mk, the symbols url uses PKG_BASENAME as its basis, while the test packages url uses MOZ_INFO_BASENAME in the other patch. Reading further up in package-name.mk it looks like these would differ in some circumstances.

Either way, I think this should be pulled into another method rather than duplicated from query_symbols_url.
Attachment #8713102 - Flags: review?(cmanchester)
Comment on attachment 8712587 [details] [diff] [review]
Rename test_packages.json v1

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

::: testing/testsuite-targets.mk
@@ -425,5 @@
> -	$(NSINSTALL) -D $(dir $(MOZ_TEST_PACKAGES_FILE_TC))
> -	$(PYTHON) $(topsrcdir)/build/gen_test_packages_manifest.py \
> -      --jsshell $(JSSHELL_NAME) \
> -      --dest-file $(MOZ_TEST_PACKAGES_FILE_TC) \
> -      --use-short-names \

Ah, that's right. This option is only used here. Given that this block is gone I will also remove it from gen_test_packages_manifest.py.

@@ +604,4 @@
>    stage-instrumentation-tests \
>    stage-luciddream \
>    test-packages-manifest \
> +    $(NULL)

No, that was my fault. It will be corrected in the next version of the patch.
Comment on attachment 8713102 [details] [diff] [review]
mozharness enhancements v1.1

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

::: testing/mozharness/mozharness/mozilla/testing/testbase.py
@@ +155,3 @@
>              reference_url = self.installer_url
> +        elif self.test_packages_url:
> +            reference_url = self.test_packages_url

It's a left-over from local testing, which I indeed missed to remove. The order of checks which we had before is what we really need. If a test package URL has been specified the appropriate folder also has to be checked first. Keep in mind that e.g. for l10n builds the tests and installers are in different folders. So we really cannot check for the installer first. I will add a comment here.

@@ +527,5 @@
> +                    if self.installer_url.endswith(suffix):
> +                        self.test_packages_url = (self.installer_url[:-len(suffix)] +
> +                                                  '.test_packages.json'
> +                                                  )
> +                        break

You mean everything below line 80?
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/package-name.mk#80

This is for release builds, right? We do not ship any test packages beside jsshell and also don't want to. See bug 1242035 as reference. So test packages will only exist for try, tinderbox, and nightly builds across branches. And that's what we all are testing. If anyone wants to test a release build it would be on his own to find the appropriate tinderbox build, and use that build for testing.

That said MOZ_INFO_BASENAME is always equal to PKG_BASENAME, and I also haven't anything else on FTP yet.

https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/package-name.mk#68

So not sure if this really needs a change. Can you please re-check your request? Thanks.
Attached patch Rename test_packages.json v2 (obsolete) — — Splinter Review 
Updated packaging patch for review comments. Would this need someone else to review?
Attachment #8712587 - Attachment is obsolete: true
Attachment #8713554 - Flags: review?(cmanchester)
Attached patch mozharness enhancements v2 (obsolete) — — Splinter Review 
Updated patch for review comments except the part wit the still open question.
Attachment #8713102 - Attachment is obsolete: true
Attachment #8713557 - Flags: review?(cmanchester)
(In reply to Henrik Skupin (:whimboo) from comment #19)
> Comment on attachment 8713102 [details] [diff] [review]
> mozharness enhancements v1.1
> 
> Review of attachment 8713102 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mozharness/mozharness/mozilla/testing/testbase.py
> @@ +155,3 @@
> >              reference_url = self.installer_url
> > +        elif self.test_packages_url:
> > +            reference_url = self.test_packages_url
> 
> It's a left-over from local testing, which I indeed missed to remove. The
> order of checks which we had before is what we really need. If a test
> package URL has been specified the appropriate folder also has to be checked
> first. Keep in mind that e.g. for l10n builds the tests and installers are
> in different folders. So we really cannot check for the installer first. I
> will add a comment here.
> 
> @@ +527,5 @@
> > +                    if self.installer_url.endswith(suffix):
> > +                        self.test_packages_url = (self.installer_url[:-len(suffix)] +
> > +                                                  '.test_packages.json'
> > +                                                  )
> > +                        break
> 
> You mean everything below line 80?
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/
> package-name.mk#80
> 
> This is for release builds, right? We do not ship any test packages beside
> jsshell and also don't want to. See bug 1242035 as reference. So test
> packages will only exist for try, tinderbox, and nightly builds across
> branches. And that's what we all are testing. If anyone wants to test a
> release build it would be on his own to find the appropriate tinderbox
> build, and use that build for testing.
> 
> That said MOZ_INFO_BASENAME is always equal to PKG_BASENAME, and I also
> haven't anything else on FTP yet.
> 
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/
> package-name.mk#68
> 
> So not sure if this really needs a change. Can you please re-check your
> request? Thanks.

If you're confident this works for all the cases you care about that's fine, but using PKG_BASENAME instead seems like an easy way to be sure.
Comment on attachment 8713554 [details] [diff] [review]
Rename test_packages.json v2

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

::: build/gen_test_packages_manifest.py
@@ -69,5 @@
>      # which will be an optimization once parts of the main zip are split to harness
>      # specific zips.
>      tests_common = args.tests_common
> -    if args.use_short_names:
> -        tests_common = 'target.common.tests.zip'

This property is checked in the loop below as well.

::: toolkit/mozapps/installer/package-name.mk
@@ +173,1 @@
>  MOZ_TEST_PACKAGES_FILE_TC = $(DIST)/$(PKG_PATH)/test_packages_tc.json

My comment about removing this variable was not addressed.
Attachment #8713554 - Flags: review?(cmanchester)
Comment on attachment 8713557 [details] [diff] [review]
mozharness enhancements v2

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

These might be better as a single patch, they're both in-tree and can't land separately.

::: testing/mozharness/mozharness/mozilla/testing/testbase.py
@@ +527,5 @@
> +                    if self.installer_url.endswith(suffix):
> +                        self.test_packages_url = (self.installer_url[:-len(suffix)] +
> +                                                  '.test_packages.json'
> +                                                  )
> +                        break

My comment about de-duplicating this loop between here and query_symbols_url was not addressed.
Attachment #8713557 - Flags: review?(cmanchester)
(In reply to Chris Manchester [:chmanchester] from comment #22)
> If you're confident this works for all the cases you care about that's fine,
> but using PKG_BASENAME instead seems like an easy way to be sure.

Well, we can. But in this case I would move up the declaration of MOZ_TEST_PACKAGES_FILE so it is beside all the other test packages. Also I would strip out the path, and move that part to testsuite-targets.mk.

(In reply to Chris Manchester [:chmanchester] from comment #23)
> ::: build/gen_test_packages_manifest.py
> @@ -69,5 @@
> >      # which will be an optimization once parts of the main zip are split to harness
> >      # specific zips.
> >      tests_common = args.tests_common
> > -    if args.use_short_names:
> > -        tests_common = 'target.common.tests.zip'
> 
> This property is checked in the loop below as well.
> 
> ::: toolkit/mozapps/installer/package-name.mk
> @@ +173,1 @@
> >  MOZ_TEST_PACKAGES_FILE_TC = $(DIST)/$(PKG_PATH)/test_packages_tc.json
> 
> My comment about removing this variable was not addressed.

Sorry, the new patch will have it fixed.

(In reply to Chris Manchester [:chmanchester] from comment #24)
> These might be better as a single patch, they're both in-tree and can't land
> separately.

Given that you are the only (??) reviewer here, it makes sense. I just wanted to make it easier for everyone to only have to review the code in their area.

> ::: testing/mozharness/mozharness/mozilla/testing/testbase.py
> @@ +527,5 @@
> > +                    if self.installer_url.endswith(suffix):
> > +                        self.test_packages_url = (self.installer_url[:-len(suffix)] +
> > +                                                  '.test_packages.json'
> > +                                                  )
> > +                        break
> 
> My comment about de-duplicating this loop between here and query_symbols_url
> was not addressed.

I still don't see why this would need a change, especially that we make use of PKG_BASENAME now. There is no difference anymore compared to the crash symbols archive. If you still want this please explain so I can understand it. Thanks.
With updates as mentioned in the last comment, and as a combined patch.
Attachment #8713554 - Attachment is obsolete: true
Attachment #8713557 - Attachment is obsolete: true
Attachment #8714830 - Flags: review?(cmanchester)
Comment on attachment 8714830 [details] [diff] [review]
Use package basename as prefix for test_packages.json v1 (combined patch)

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

::: toolkit/mozapps/installer/package-name.mk
@@ +144,4 @@
>  WP_TEST_PACKAGE = $(PKG_BASENAME).web-platform.tests.zip
>  TALOS_PACKAGE = $(PKG_BASENAME).talos.tests.zip
>  GTEST_PACKAGE = $(PKG_BASENAME).gtest.tests.zip
> +MOZ_TEST_PACKAGES_FILE = $(PKG_BASENAME).test_packages.json

Why take the $(DIST)/$(PKG_PATH) off this variable, and move it to each of its uses? This seems more error prone.
Attachment #8714830 - Flags: review?(cmanchester)
(In reply to Henrik Skupin (:whimboo) from comment #25)
> (In reply to Chris Manchester [:chmanchester] from comment #22)
> > If you're confident this works for all the cases you care about that's fine,
> > but using PKG_BASENAME instead seems like an easy way to be sure.
> 
> Well, we can. But in this case I would move up the declaration of
> MOZ_TEST_PACKAGES_FILE so it is beside all the other test packages. Also I
> would strip out the path, and move that part to testsuite-targets.mk.
> 
> (In reply to Chris Manchester [:chmanchester] from comment #23)
> > ::: build/gen_test_packages_manifest.py
> > @@ -69,5 @@
> > >      # which will be an optimization once parts of the main zip are split to harness
> > >      # specific zips.
> > >      tests_common = args.tests_common
> > > -    if args.use_short_names:
> > > -        tests_common = 'target.common.tests.zip'
> > 
> > This property is checked in the loop below as well.
> > 
> > ::: toolkit/mozapps/installer/package-name.mk
> > @@ +173,1 @@
> > >  MOZ_TEST_PACKAGES_FILE_TC = $(DIST)/$(PKG_PATH)/test_packages_tc.json
> > 
> > My comment about removing this variable was not addressed.
> 
> Sorry, the new patch will have it fixed.
> 
> (In reply to Chris Manchester [:chmanchester] from comment #24)
> > These might be better as a single patch, they're both in-tree and can't land
> > separately.
> 
> Given that you are the only (??) reviewer here, it makes sense. I just
> wanted to make it easier for everyone to only have to review the code in
> their area.
> 
> > ::: testing/mozharness/mozharness/mozilla/testing/testbase.py
> > @@ +527,5 @@
> > > +                    if self.installer_url.endswith(suffix):
> > > +                        self.test_packages_url = (self.installer_url[:-len(suffix)] +
> > > +                                                  '.test_packages.json'
> > > +                                                  )
> > > +                        break
> > 
> > My comment about de-duplicating this loop between here and query_symbols_url
> > was not addressed.
> 
> I still don't see why this would need a change, especially that we make use
> of PKG_BASENAME now. There is no difference anymore compared to the crash
> symbols archive. If you still want this please explain so I can understand
> it. Thanks.

This is separate from the question about PKG_BASENAME. There's a loop in the method in this file called query_symbols_url that this logic is apparently copied from. It would be preferable to extract this as a function rather than duplicating it.
Updated patch with the following changes:

* Reverted all latest packaging changes for paths
* Refactored query_build_dir_url() to also accept package prefixes. Given that most of the files in the nightly folders actually contain a prefix, I set it to True by default. Also added the error level so we do not necessarily fail with a fatal message if the expected URL is not found. This should actually also fix or partially fix bug 1225124.

Try run on Windows and Linux with a handful of tests is passing:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5994227a467
Attachment #8714830 - Attachment is obsolete: true
Attachment #8715194 - Flags: review?(cmanchester)
Comment on attachment 8715194 [details] [diff] [review]
Use package basename as prefix for test_packages.json v2 (combined patch)

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

I think some of these mozharness changes go beyond the scope of this bug, and I don't think the result is particularly intuitive. If you're intent on this as an api please run it by jlund, it's more than I'm comfortable reviewing in that file.

::: testing/mozharness/mozharness/mozilla/testing/testbase.py
@@ +146,5 @@
>          else:
>              return self.download_proxied_file(*args, **kwargs)
>  
> +    def query_build_dir_url(self, file_name, with_package_prefix=True,
> +                            error_level=FATAL):

I think a separate method for when we're looking for the package prefix would be fine for this.

@@ +151,4 @@
>          """
>          Resolve a file name to a potential url in the build upload directory where
>          that file can be found.
>          """

If we're going to go with these extra parameters, this docstring needs to be updated. The "file_name" parameter loses its meaning somewhat, because it might just be a suffix now.

@@ +179,5 @@
> +            if url:
> +                self._urlopen(url)
> +        except urllib2.URLError:
> +            self.log('Requested URL does not exist: %s.' % url,
> +                     level=error_level)

This seems redundant with the error handling done by whatever code is going to try to use the url.

::: toolkit/mozapps/installer/package-name.mk
@@ +136,4 @@
>  MOZHARNESS_PACKAGE = mozharness.zip
>  
>  # Test package naming
> +MOZ_TEST_PACKAGES_FILE = $(DIST)/$(PKG_PATH)/$(PKG_BASENAME).test_packages.json

Nit: no need to move this.

::: toolkit/mozapps/installer/upload-files.mk
@@ +739,4 @@
>    $(call QUOTED_WILDCARD,$(DIST)/$(PKG_PATH)$(REFTEST_PACKAGE)) \
>    $(call QUOTED_WILDCARD,$(DIST)/$(PKG_PATH)$(WP_TEST_PACKAGE)) \
>    $(call QUOTED_WILDCARD,$(DIST)/$(PKG_PATH)$(GTEST_PACKAGE)) \
> +  $(call QUOTED_WILDCARD,$(MOZ_TEST_PACKAGES_FILE)) \

Nit: no need to move this.
Attachment #8715194 - Flags: review?(cmanchester)
Jordan, what do you think about Chris' last comment regarding the changes for mozharness? Shall we do the simple version here and push the current code to a follow-up patch? It would mean that I add a temporary method which gets soon removed again. If you think that is the preferred way I will do that.
Flags: needinfo?(jlund)
Comment on attachment 8715194 [details] [diff] [review]
Use package basename as prefix for test_packages.json v2 (combined patch)

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

I agree with chris's comments on the mozharness parts. I support what you are trying to do in the bug but I have some further thoughts below.

::: testing/mozharness/mozharness/mozilla/testing/testbase.py
@@ +167,5 @@
> +        if with_package_prefix:
> +            url = None
> +            for suffix in reference_suffixes:
> +                if reference_url.endswith(suffix):
> +                    url = reference_url[:-len(suffix)] + file_name

I guess what makes it less intuitive for me is that suffix == file_name when you call below:
   self.test_packages_url = self.query_build_dir_url('.test_packages.json')

if I understand chris, I agree that it would be great to make a separate method for your query_build_dir_url use case. either as a subclass in ui_tests or with a different method name. This would reduce the complexity of having to flexible for each use case.
Flags: needinfo?(jlund)
Ok, I will then create a separate method. Jordan, do you want the extra URL check for existence being included or should I leave this for the symbols_url only? If you don't want that on this bug, maybe we can have a follow-up to make it more general. Please let me know.
Flags: needinfo?(jlund)
(In reply to Henrik Skupin (:whimboo) [away 02/13 - 02/20] from comment #34)
> Ok, I will then create a separate method. Jordan, do you want the extra URL
> check for existence being included or should I leave this for the
> symbols_url only? If you don't want that on this bug, maybe we can have a
> follow-up to make it more general. Please let me know.

if you are referring to the: `+        # Only return the URL if it really exists` block, off a glance, it looks like we would be code duplicating because we would eventually check that in download_file()[1] within self.download_unzip or when we get self.test_packages_url no?

[1] https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/base/script.py#525
Flags: needinfo?(jlund)
(In reply to Jordan Lund (:jlund) from comment #35)
> if you are referring to the: `+        # Only return the URL if it really
> exists` block, off a glance, it looks like we would be code duplicating
> because we would eventually check that in download_file()[1] within
> self.download_unzip or when we get self.test_packages_url no?

This is the case for most of the checks but not for the symbols_url. Here we don't want to add a non-existent URL which will break mozcrash.

So I will create a new method and make this extra check only being used for the symbols_url retrieval for now.
(In reply to Chris Manchester (Away 2/18 - 2/21) from comment #31)
> ::: toolkit/mozapps/installer/package-name.mk
> > +MOZ_TEST_PACKAGES_FILE = $(DIST)/$(PKG_PATH)/$(PKG_BASENAME).test_packages.json
> 
> Nit: no need to move this.
> 
> ::: toolkit/mozapps/installer/upload-files.mk
> @@ +739,4 @@
> > +  $(call QUOTED_WILDCARD,$(MOZ_TEST_PACKAGES_FILE)) \
> 
> Nit: no need to move this.

Chris, I moved both to put the definition right next to the files it belongs to. The current location seems to be out of place to me. But if that is a concern for you which causes a r- I will revert this move.
So I reverted all mentioned code and created a new method for the mozharness changes with nearly duplicate code compared to `query_build_dir_url()`. I still don't see a benefit in that but if that is what we want, so here it is.

Asking for review from Chris (build system changes) and Jordan (mozharness changes).
Attachment #8715194 - Attachment is obsolete: true
Attachment #8722491 - Flags: review?(jlund)
Attachment #8722491 - Flags: review?(cmanchester)
Comment on attachment 8722491 [details] [diff] [review]
Use package basename as prefix for test_packages.json v3 (combined patch)

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

please bear with me. I am getting up to speed and this still doesn't seem intuitive. let's discuss! :D see comments below.

::: testing/mozharness/mozharness/mozilla/testing/testbase.py
@@ +164,4 @@
>  
>          return '%s/%s' % (base_url, file_name)
>  
> +    def query_prefixed_build_dir_url(self, file_name):

so correct me if I am wrong but the benefit of creating a dedicated method is that you can change the style of the implementation to suit the use case.

e.g. `file_name` doesn't really suit what you are passing. This can be the explicit `suffix` no? and then, in the method body, couldn't you make some assumptions with less complexity?

do we need the if/else blocks with varying suffixes/urls even when you pass an explicit suffix?

I could be having an issue with the logic that was here before you..

@@ +180,5 @@
> +
> +        url = None
> +        for suffix in reference_suffixes:
> +            if reference_url.endswith(suffix):
> +                url = reference_url[:-len(suffix)] + file_name

for example, this seems odd that in the self.query_prefixed_build_dir_url('.test_packages.json') call case when self.test_packages_url is true, you are removing '.test_packages.json' from the end of reference_url, only to replace it with '.test_packages.json' no?
Comment on attachment 8722491 [details] [diff] [review]
Use package basename as prefix for test_packages.json v3 (combined patch)

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

::: testing/mozharness/mozharness/mozilla/testing/testbase.py
@@ +164,5 @@
>  
>          return '%s/%s' % (base_url, file_name)
>  
> +    def query_prefixed_build_dir_url(self, file_name):
> +        """Resolve a file name prefixed with platform and build details to a potential url

This request came from Chris and not me. So I just did it because it was wanted. As mentioned in my last comment, we have a lot of duplication, yes.

I can easily rename `file_name` to suffix. That's not a problem. Regarding complexity I assume it is the if/else block you mean? I don't think that we should simplify that and only care about the installer url. Keep in mind that there is also the option to pass in `--installer-path` and no url, so the above method would fail if we don't also check for a possible `--test_packages_url` as what `query_build_dir_url()` is also doing.

@@ +180,5 @@
> +
> +        url = None
> +        for suffix in reference_suffixes:
> +            if reference_url.endswith(suffix):
> +                url = reference_url[:-len(suffix)] + file_name

We only call `query_prefixed_build_dir_url()` with `.test_packages.json` if no `--test-packages-url` has been specified. So in that case we end-up in the elif case for the `installer_url`. So we do not have a no-op here.
Comment on attachment 8722491 [details] [diff] [review]
Use package basename as prefix for test_packages.json v3 (combined patch)

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

okay, sounds like you are confident I think I grok why we need each case. rename to `suffix` sgtm. r+ if you do that for the mh bits.
Attachment #8722491 - Flags: review?(jlund) → review+
(In reply to Henrik Skupin (:whimboo) from comment #40)
> Comment on attachment 8722491 [details] [diff] [review]
> Use package basename as prefix for test_packages.json v3 (combined patch)
> 
> Review of attachment 8722491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mozharness/mozharness/mozilla/testing/testbase.py
> @@ +164,5 @@
> >  
> >          return '%s/%s' % (base_url, file_name)
> >  
> > +    def query_prefixed_build_dir_url(self, file_name):
> > +        """Resolve a file name prefixed with platform and build details to a potential url
> 
> This request came from Chris and not me. So I just did it because it was
> wanted. As mentioned in my last comment, we have a lot of duplication, yes.

My request (for the patch in comment 23 and comment 24) was only to extract the `for suffix in INSTALLER_SUFFIXES:` loop to a small helper function rather than duplicating it.
Comment on attachment 8722491 [details] [diff] [review]
Use package basename as prefix for test_packages.json v3 (combined patch)

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

r+ on the build system stuff.
Attachment #8722491 - Flags: review?(cmanchester) → review+
Updated patch for the `suffix` parameter. Taking over r+ from jlund and cmanchester.
Attachment #8722491 - Attachment is obsolete: true
Attachment #8722909 - Flags: review+
The patch got backed-out because we have test bustage in TC. It still uses test_packages.json as value for the tests which results in a 404. So it looks like that some of the removed tc specific build system code was too much. I will work with the TC people to figure out if that can be fixed without reverting the build system code.
So we have two places which need an update and which were missed by myself:

mozharness mach command:
https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mach_commands.py#40

taskcluster test parameters:
https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/tasks/builds/firefox_base.yml#9

I cannot find any other place with hard-coded filenames so the above should be enough.
So I have seen while investigating all TC builds including Tier-3 that we have broken builds for b2g-emulator and b2g-mulet. Those jobs are still relying on a test_packages_tc.json file when moving files around, which creation we have removed. So three more files need an update.

https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/scripts/builder/build-emulator-x86.sh#63
https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/scripts/builder/build-emulator.sh#65
https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/scripts/builder/build-mulet-linux.sh#36
I got the linux64 debug tests fixed, but the b2g-emulator unittests are still failing. Reason here is that the installer is named totally different:

 09:31:05     INFO - Found symbols url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/try-builds/hskupin@mozilla.com-4b33e6bf89c7/try-emulator-debug/b2g-47.0a1.en-US.android-arm.crashreporter-symbols.zip.
 09:31:05     INFO - Found test url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/try-builds/hskupin@mozilla.com-4b33e6bf89c7/try-emulator-debug/b2g-47.0a1.en-US.android-arm.reftest.tests.zip.
09:31:05 INFO - Found installer url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/try-builds/hskupin@mozilla.com-4b33e6bf89c7/try-emulator-debug/emulator.tar.gz.

That means we use that installer URL, strip off `.tar.gz` and concatenate `.test_packages.json`. This results in `emulator.test_packages.json`.

Also when I check the folder http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/try-builds/hskupin@mozilla.com-4b33e6bf89c7/try-emulator-debug/ I do not see an uploaded *.test_package.json file.

The above is a buildbot job so independent from the taskcluster changes, and definitely needs to be fixed.
Armen and Chris helped me to find the code which is responsible to upload the test packages for emulator builds. So I indeed missed the hard-coded file path - not sure why dxr didn't show me those files.

https://dxr.mozilla.org/mozilla-central/source/b2g/config/emulator/config.json#16
https://dxr.mozilla.org/mozilla-central/source/b2g/config/emulator-ics/config.json#16

Now we only need more magic for mozharness.
Thankfully no magic for mozharness is needed. Those emulator builds get the test_package.json URL via the buildbot properties. The one for the test package was not present because we didn't upload it due to the above missing changes. The following try push will hopefully prove that:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d022f7fc1959&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=17188113
So we have bustage again but I feel that this is a real bug in the build script for emulator builds. When you check the log file you will see the following:

16:55:43     INFO - Writing buildbot properties ['testsUrl'] to /builds/slave/b2g_try_emu-d_dep-000000000000/properties/testsUrl
16:55:43     INFO - Writing to file /builds/slave/b2g_try_emu-d_dep-000000000000/properties/testsUrl
16:55:43     INFO - Contents:
16:55:43     INFO -  testsUrl:http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/try-builds/hskupin@mozilla.com-5863f3238f9e/try-emulator-debug/b2g-47.0a1.en-US.android-arm.reftest.tests.zip

Why do we set the `testsUrl` and not `test_packages_url`? Here we also only set the reftests zip archive while we are running cppunit and xpcshell tests. This feels very suspicious!

Lets have a look at the cppunit test:
https://treeherder.mozilla.org/logviewer.html#?job_id=17198415&repo=try#L416

So it found all the files as set as buildbot properties by the build step:

 16:56:21     INFO - Found symbols url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/try-builds/hskupin@mozilla.com-5863f3238f9e/try-emulator-debug/b2g-47.0a1.en-US.android-arm.crashreporter-symbols.zip.
 16:56:21     INFO - Found test url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/try-builds/hskupin@mozilla.com-5863f3238f9e/try-emulator-debug/b2g-47.0a1.en-US.android-arm.reftest.tests.zip.
16:56:21 INFO - Found installer url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/try-builds/hskupin@mozilla.com-5863f3238f9e/try-emulator-debug/emulator.tar.gz.

When I check a Cpp job from mozilla-central I can see that the common tests package is referenced there:
https://treeherder.mozilla.org/logviewer.html#?job_id=3359786&repo=mozilla-central#L416

It means we assign the wrong test package for this buildbot property.
The b2g_build.py mozharness script seems to be the only script in our source which actually sets the testsUrl buildbot property. Given that we should use the test_packages.json in all the cases I modified the build script to use this as property instead. I strongly believe that we simply missed to change that previously and it was hidden because the test_packages.json was easily found due to no prefix -  and with it the necessary referenced common_tests.zip and xpcshell.zip files. Now that we have the prefix this wont work anymore and given that the installerUrl is 'emulator.tar.gz' and does not contain the prefix we fail to determine the file. To be safe here we have to explicitly set the testPackagesUrl buildbot property.

I hope that this will solve the remaining issue. Try job is here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63df3325bfe6&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=17218476
That made it! So we have a green Cpp unittest run for the b2g emulator build. The test packages have been found and the necessary zips are getting downloaded:

 05:16:49     INFO - Found symbols url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/try-builds/hskupin@mozilla.com-63df3325bfe6/try-emulator-debug/b2g-47.0a1.en-US.android-arm.crashreporter-symbols.zip.
 05:16:49     INFO - Found a test packages url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/try-builds/hskupin@mozilla.com-63df3325bfe6/try-emulator-debug/b2g-47.0a1.en-US.android-arm.test_packages.json.
05:16:49 INFO - Found installer url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/try-builds/hskupin@mozilla.com-63df3325bfe6/try-emulator-debug/emulator.tar.gz.
[..]
 05:16:49     INFO - Downloading packages: [u'b2g-47.0a1.en-US.android-arm.common.tests.zip', u'b2g-47.0a1.en-US.android-arm.cppunittest.tests.zip'] for test suite category: cppunittest

xpcshell tests are taking longer so I will still wait for them before triggering a complete try job.
Ok, so the full try push showed lots more test failures and I feel that this is all related to the removal of the test_packages_tc.json config. It can be best seen here:

https://public-artifacts.taskcluster.net/ZiA_jq7IQvWEU0zE0L0N7Q/0/public/build/target.test_packages.json

All test packages as stored as artifacts on this task have the prefix `target.` but when you look into this json file you will notice that all test files are listed with the platform prefix: "b2g-47.0a1.en-US.linux-i686.common.tests.zip". This totally breaks everything.

So I'm going to revert those build system changes to let `gen_test_packages_manifest.py` create the correct file names.
Chris told me that I should not care about Tier-3 level failures for B2G and Mulet. Those are simply dead for now and we should concentrate on Tier-1 and Tier-2.
Tier-1 and Tier-2 are looking good now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=46f0692f2b71&selectedJob=17226374

Right now it takes ages before Windows tests are getting run on try. So given that my original push to inbound which got backed-out had no failures for Windows I will go ahead and prepare the follow-up patch for review.
Comment on attachment 8722909 [details] [diff] [review]
Use package basename as prefix for test_packages.json v3.1 (combined patch)

Chris and Jordan, I replaced the patch with a mozreview version. The parts you have already reviewed have not been changed. Anything else has been added as extra commits. Maybe you can re-review? Thanks.
Attachment #8722909 - Attachment is obsolete: true
Attachment #8723743 - Flags: review?(cmanchester) → review+
Comment on attachment 8723743 [details]
MozReview Request: Bug 1239808 - Rename test_packages.json to include package basename as prefix. r=chmanchester, r=jlund

https://reviewboard.mozilla.org/r/36691/#review33469

I don't see any build system differences from the last patch (if there are some I missed, please ping me).
Comment on attachment 8723745 [details]
MozReview Request: Bug 1239808 - Fix B2G build script and configs for test_packages.json renaming. r=chmanchester

https://reviewboard.mozilla.org/r/36695/#review33471

I don't know a lot about this particular script, but the change looks sensible. If it fixes builds let's go with it. Thanks.
Attachment #8723745 - Flags: review?(cmanchester) → review+
Comment on attachment 8723743 [details]
MozReview Request: Bug 1239808 - Rename test_packages.json to include package basename as prefix. r=chmanchester, r=jlund

https://reviewboard.mozilla.org/r/36691/#review33489

what's changed in mozharness?
Attachment #8723743 - Flags: review?(jlund) → review+
Attachment #8723746 - Flags: review?(jlund) → review+
Comment on attachment 8723746 [details]
MozReview Request: Bug 1239808 - Fix mozharness mach command for test_packages.json retrieval. r=jlund

https://reviewboard.mozilla.org/r/36697/#review33493
Comment on attachment 8723744 [details]
MozReview Request: Bug 1239808 - Update taskcluster configs for test_package.json renaming. r=garndt

https://reviewboard.mozilla.org/r/36693/#review33501
Attachment #8723744 - Flags: review?(garndt) → review+
MOZ_TEST_PACKAGES_FILE for c-c evaluates to 'dist/mac/en-US//Daily 47.0a1.test_packages.json', which rather unfortunately has a space in it.
Attachment #8725337 - Flags: review?(jlund)
Assignee: hskupin → aleth
Assignee: aleth → hskupin
(In reply to aleth [:aleth] from comment #71)
> MOZ_TEST_PACKAGES_FILE for c-c evaluates to 'dist/mac/en-US//Daily
> 47.0a1.test_packages.json', which rather unfortunately has a space in it.

Shouldn't this be a change in package-name.mk where MOZ_TEST_PACKAGES_FILE is defined? I feel its the wrong place for adding the brackets given that you would have to do it for each instance the variable is used.
Also a follow-up bug might be a better idea here given that this one is already marked as fixed.
(In reply to Henrik Skupin (:whimboo) from comment #72)
> (In reply to aleth [:aleth] from comment #71)
> > MOZ_TEST_PACKAGES_FILE for c-c evaluates to 'dist/mac/en-US//Daily
> > 47.0a1.test_packages.json', which rather unfortunately has a space in it.
> 
> Shouldn't this be a change in package-name.mk where MOZ_TEST_PACKAGES_FILE
> is defined? I feel its the wrong place for adding the brackets given that
> you would have to do it for each instance the variable is used.

I considered it, but I didn't see any other examples where such file defines contain quotes built-in?
So for what kind of builds would we need that? What is "dist/mac/en-US//Daily 47.0a1.test_packages.json"? Especially the "Daily" part here? I do not so anything like that for Nightly builds on archive.mozilla.org.
(In reply to Henrik Skupin (:whimboo) from comment #75)
> So for what kind of builds would we need that? What is
> "dist/mac/en-US//Daily 47.0a1.test_packages.json"? Especially the "Daily"
> part here? I do not so anything like that for Nightly builds on
> archive.mozilla.org.

These are Thunderbird nightly builds. The failure is from https://treeherder.mozilla.org/logviewer.html#?job_id=33751&repo=comm-central#L115748. Don't ask me who had the great idea to put a space in there, but it was probably a long time ago...
When I check the full log file of that given job I can see the following:

http://archive.mozilla.org/pub/thunderbird/nightly/2016/03/2016-03-01-03-02-43-comm-central/comm-central-macosx64-nightly-bm86-build1-build7.txt.gz

/gen_test_packages_manifest.py \
      --jsshell jsshell-mac.zip \
      --dest-file dist//thunderbird-47.0a1.en-US.mac.test_packages.json \
      --common 'thunderbird-47.0a1.en-US.mac.common.tests.zip' \
      --common 'thunderbird-47.0a1.en-US.mac.common.tests.zip' --cppunittest 'thunderbird-47.0a1.en-US.mac.cppunittest.tests.zip' --mochitest 'thunderbird-47.0a1.en-US.mac.mochitest.tests.zip' --reftest 'thunderbird-47.0a1.en-US.mac.reftest.tests.zip' --talos 'thunderbird-47.0a1.en-US.mac.talos.tests.zip' --web-platform 'thunderbird-47.0a1.en-US.mac.web-platform.tests.zip' --xpcshell 'thunderbird-47.0a1.en-US.mac.xpcshell.tests.zip'

That's how I expect it to be called. Then the package-tests step is called a second time with the strange Daily prefix:

/gen_test_packages_manifest.py \
      --jsshell jsshell-mac.zip \
      --dest-file dist/mac/en-US//Daily 47.0a1.test_packages.json \
      --common 'Daily 47.0a1.common.tests.zip' \
      --common 'Daily 47.0a1.common.tests.zip' --cppunittest 'Daily 47.0a1.cppunittest.tests.zip' --mochitest 'Daily 47.0a1.mochitest.tests.zip' --reftest 'Daily 47.0a1.reftest.tests.zip' --talos 'Daily 47.0a1.talos.tests.zip' --web-platform 'Daily 47.0a1.web-platform.tests.zip' --xpcshell 'Daily 47.0a1.xpcshell.tests.zip'

Isn't there something else wrong in the build configs of Thunderbird here? I would really suggest to start new bug for that now.
Depends on: 1252809
(In reply to Henrik Skupin (:whimboo) from comment #77)
> Isn't there something else wrong in the build configs of Thunderbird here? I
> would really suggest to start new bug for that now.

Right, that looks very strange. Thanks. Filed as Bug 1252809.
Attachment #8725337 - Attachment is obsolete: true
Attachment #8725337 - Flags: review?(jlund)
I don't think it's a good idea to backport this to aurora right before the upcoming merge next Monday. Lets wait for that so that both branches will be fixed.
status-firefox46: affected → wontfix
Depends on: 1254604
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: