1188766 - sccache setup with UPLOAD_EXTRA_FILES makes some files uploaded multiple times

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

e.g.

21:35:57     INFO -  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-daf07dbe09f4/try-macosx64/sccache.log.gz
21:35:57     INFO -  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-daf07dbe09f4/try-macosx64/mar
21:35:57     INFO -  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-daf07dbe09f4/try-macosx64/mbsdiff
21:35:57     INFO -  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-daf07dbe09f4/try-macosx64/sccache.log.gz
21:35:57     INFO -  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-daf07dbe09f4/try-macosx64/mar
21:35:57     INFO -  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-daf07dbe09f4/try-macosx64/mbsdiff
21:35:57     INFO -  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-daf07dbe09f4/try-macosx64/sccache.log.gz
21:35:57     INFO -  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-daf07dbe09f4/try-macosx64/mar
21:35:57     INFO -  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-daf07dbe09f4/try-macosx64/mbsdiff

Comment 1

[Mike Hommey [:glandium]]

Attachment: Unexport UPLOAD_EXTRA_FILES from upload-files.mk

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8640347 [details] [diff] [review]
Unexport UPLOAD_EXTRA_FILES from upload-files.mk

Mmmm this worked locally, but doesn't seem to work on try.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Avoid exporting UPLOAD_EXTRA_FILES from mozconfig

The way UPLOAD_EXTRA_FILES is currently exported makes the files added
to the list added in each directory that is being traversed recursively
because of the += in .mozconfig.mk, combined with "export".

The easy way out is to remove the export altogether, but being unsure of
the side effects of putting all non exported mk_add_options variables
in .mozconfig.mk, limit this to UPLOAD_EXTRA_FILES.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21829aaf72e1

Comment 4

[Mike Hommey [:glandium]]

Attachment: Avoid exporting UPLOAD_EXTRA_FILES from mozconfig

Sorry, previous patch still had debugging $(warning)s.

Comment 5

[Michael Shal [:mshal]]

Attachment: 0001-Bug-1188766-Use-UPLOAD_MOZCONFIG_FILES-for-files-add.patch

Can we go back to something similar to the pre-bug-1042432 way of passing the filenames to make and just use a new variable to avoid the export issue? The complicated interactions with export+make and the fact that we'd have to special-case UPLOAD_EXTRA_FILES in client.mk makes me think we're going about this in a weird way.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=545e57b20c93

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8644667 [details] [diff] [review]
0001-Bug-1188766-Use-UPLOAD_MOZCONFIG_FILES-for-files-add.patch

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

The problem is that this approach had its own problem. Note that UPLOAD_EXTRA_FILES is special cased in my patch because I don't want to figure out now what the consequences are of putting all non export variables in .mozconfig.mk, but the end goal would be for that to happen, so my patch actually removes the weird special-casing there was for UPLOAD_EXTRA_FILES, although it uses a temporary different special-case to do so.

Comment 7

[Michael Shal [:mshal]]

Can you clarify what problem that is with this approach? I realize this was problematic when we appended to "UPLOAD_EXTRA_FILES" both in the mozconfig and in the Makefile, but this uses a separate variable that is only set by the mozconfig and read by the Makefile.

Comment 8

[Mike Hommey [:glandium]]

To be honest, I don't remember what was going wrong with the state pre-bug-1042432, but the fact is that it only happened on release/beta, not on try, and that just by looking at the state pre-bug-1042432, I have no idea why this even failed in the first place, so I want to be extra cautious not to reintroduce the conditions that made it fail in the first place. We do know that the .mozconfig.mk trick works, and the only caveat it has is the fact that there is an "export" in there, which my patch removes.

Comment 9

[Mike Hommey [:glandium]]

And as I said, I want for all mk_add_options to end up in mozconfig.mk eventually, I just don't want to deal with unexpected side effects from that just to fix this particular issue.

Comment 10

[Michael Shal [:mshal]]

Comment on attachment 8644179 [details] [diff] [review]
Avoid exporting UPLOAD_EXTRA_FILES from mozconfig

Fair enough.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0288a5a47478

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd0fd190e964

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0288a5a47478
https://hg.mozilla.org/mozilla-central/rev/cd0fd190e964

Comment 14

[aleth [:aleth]]

url:        https://hg.mozilla.org/comm-central/rev/b1c282fe435af169b2a46df96aed132eadb95b87
changeset:  b1c282fe435af169b2a46df96aed132eadb95b87
user:       aleth <[e-mail]>
date:       Wed Aug 12 01:14:53 2015 +0200
description:
Keep build/ in sync (port bug 1188766). rs=bustage-fix