Closed Bug 1188766 Opened 7 years ago Closed 7 years ago

sccache setup with UPLOAD_EXTRA_FILES makes some files uploaded multiple times


(Firefox Build System :: General, defect)

Not set


(firefox43 fixed)

Tracking Status
firefox43 --- fixed


(Reporter: glandium, Assigned: glandium)




(2 files, 2 obsolete files)

Assignee: nobody → mh+mozilla
Attachment #8640347 - Flags: review?(mshal)
Comment on attachment 8640347 [details] [diff] [review]

Mmmm this worked locally, but doesn't seem to work on try.
Attachment #8640347 - Flags: review?(mshal)
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, 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, limit this to UPLOAD_EXTRA_FILES.
Attachment #8640347 - Attachment is obsolete: true
Sorry, previous patch still had debugging $(warning)s.
Attachment #8644177 - Attachment is obsolete: true
Attachment #8644179 - Flags: review?(mshal)
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 makes me think we're going about this in a weird way.
Attachment #8644667 - Flags: review?(mh+mozilla)
Comment on attachment 8644667 [details] [diff] [review]

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, 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.
Attachment #8644667 - Flags: review?(mh+mozilla) → review-
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.
Flags: needinfo?(mh+mozilla)
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 trick works, and the only caveat it has is the fact that there is an "export" in there, which my patch removes.
Flags: needinfo?(mh+mozilla)
And as I said, I want for all mk_add_options to end up in eventually, I just don't want to deal with unexpected side effects from that just to fix this particular issue.
Comment on attachment 8644179 [details] [diff] [review]
Avoid exporting UPLOAD_EXTRA_FILES from mozconfig

Fair enough.
Attachment #8644179 - Flags: review?(mshal) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
changeset:  b1c282fe435af169b2a46df96aed132eadb95b87
user:       aleth <>
date:       Wed Aug 12 01:14:53 2015 +0200
Keep build/ in sync (port bug 1188766). rs=bustage-fix
Blocks: 1195246
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.