Closed Bug 1352982 Opened 3 years ago Closed 3 years ago

Removing a javascript module shouldn't require a clobber

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: florian, Assigned: mshal)

References

Details

Attachments

(1 file)

In bug 1351604, the patch was backed out and had to reland with a clobber, because after removing a .jsm file from the tree, that file was still part of new packaged builds, and so caused the browser_all_files_referenced.js test to fail.

While it makes sense that the file still exists in a local build after removing it from the source tree, I would expect mach package to only package modules that are known to the moz.build files.
It looks like the failure was only on OSX opt, right? I tried to reproduce it locally in Linux by doing:

1) checkout tree before bug 1351604 landed
2) ./mach build
 - I now have ./obj-x86_64-pc-linux-gnu/dist/bin/modules/psm/DER.jsm
3) Apply bug 1351604
4) ./mach build (incrementally)
 - I now have ./obj-x86_64-pc-linux-gnu/_tests/modules/psm/DER.jsm, and the dist/bin/modules/psm/DER.jsm file was removed.
5) ./mach package
 - The packaged omni.ja does not have a DER.jsm anymore

I'm not sure why OSX would be different, but it seems odd that the file wouldn't have been removed during install manifest processing.
Another bug where I got backed out due to not clobbering in the patch is bug 1330821, and now that I read it again, it was also a javascript module being removed, and the failure was also on OS X.
Blocks: 1330821
Blocks: 1351074
See Also: → 1350472
This is most assuredly because of the rsync we do to copy bits from dist/bin into the app bundle for OS X builds:
https://dxr.mozilla.org/mozilla-central/rev/b043233ec04f06768d59dcdfb9e928142280f3cc/browser/app/Makefile.in#89
Ahh, good call. Presumably we could just add --delete then?
Actually, --delete doesn't work because there are many rsyncs. Maybe just rm-rfing $(dist_dest) will work.
I verified this by doing a "mach build; mach package; apply patch from bug 1351604; mach build; mach package" and see that the DER.jsm file is no longer in omni.ja in the 2nd package. It seems to work, but I'm not sure if this is the right approach.
Assignee: nobody → mshal
Comment on attachment 8855408 [details]
Bug 1352982 - clobber OSX .app directory before rsyncing files in;

https://reviewboard.mozilla.org/r/127252/#review130404

Good call! Someday I'll find time to move this all to moz.build so we can just use the bundle as the FINAL_TARGET instead of dist/bin, but this is a useful improvement on the current situation.
Attachment #8855408 - Flags: review?(ted) → review+
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2dc9eb12bf84
clobber OSX .app directory before rsyncing files in; r=ted
https://hg.mozilla.org/mozilla-central/rev/2dc9eb12bf84
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.