1357797 - Windows artifact builds appear to be packaging non-windows files

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mark Banner (:standard8)]

I did an artifact build on try server (https://treeherder.mozilla.org/#/jobs?repo=try&revision=d49a34bf9b82c8719f2648e542df9e9754e87594) and got failures such as:

https://treeherder.mozilla.org/logviewer.html#?job_id=92429423&repo=try&lineNumber=2766

TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 2, expected 0
TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: resource://gre/chrome/devtools/modules/devtools/shared/shims/fronts/timeline.js
TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: resource://gre/res/langGroups.properties

The devtools one could be related, however langGroups.properties is currently not packaged on Windows, only on Mac:

https://dxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in#699

It appears that artifact builds ignore this.

Note that there are other bugs to remove these files, so they may go away over time, but also this does imply that artifact builds are packaged incorrectly.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1357797 - Do not attempt to package based on empty variables in artifact builds.

Artifact builds can't rely on variables such as MSVC_C_RUNTIME_DLL when
packaging because they haven't run the parts of configure that sets
them. Existing references to these variables in package-manifest.in
are causing us to package entire directories we shouldn't when these
variables are empty. This commit prevents us from relying on these
variables when packaging an artifact build.

Review commit: https://reviewboard.mozilla.org/r/132184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132184/

Comment 2

[Mark Banner (:standard8)]

Chris: I don't see how the patch would prevent files being packaged on platforms they aren't intended for. Can you explain?

Comment 3

[Chris Manchester (limited bugmail, email directly)]

(In reply to Mark Banner (:standard8) from comment #2)
> Chris: I don't see how the patch would prevent files being packaged on
> platforms they aren't intended for. Can you explain?

The issue is that MOZ_PACKAGE_MSVC_DLLS is set, but MSVC_C(XX)_RUNTIME_DLL is set to the empty string, so when we get to

#if MOZ_PACKAGE_MSVC_DLLS
@BINPATH@/@MSVC_C_RUNTIME_DLL@
@BINPATH@/@MSVC_CXX_RUNTIME_DLL@
#endif

in package-manifest.in we end up packaging the entire bin/ directory erroneously.

Comment 4

[Michael Shal [:mshal]]

Comment on attachment 8860157 [details]
Bug 1357797 - Do not attempt to package based on empty variables in artifact builds.

https://reviewboard.mozilla.org/r/132184/#review135422

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f44e4c7777b8
Do not attempt to package based on empty variables in artifact builds. r=mshal

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f44e4c7777b8

Comment 7

[Mark Banner (:standard8)]

Chris, thanks that works well. Could you request uplift to beta on it? I'm guessing it is fairly safe and we'll probably want it for Screenshot's tests to pass.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Mark, we're only running artifact builds in automation on try, would uplifting to benefit be to any benefit?

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Mark, we're only running artifact builds in automation on try, would uplifting to beta be to any benefit?

Comment 10

[Mark Banner (:standard8)]

(In reply to Chris Manchester (:chmanchester) from comment #9)
> Mark, we're only running artifact builds in automation on try, would
> uplifting to beta be to any benefit?

Good point, I'd forgotten about that. Lets skip it.