Closed Bug 978591 Opened 10 years ago Closed 9 years ago

Remove MOZ_CHROME_FILE_FORMAT from Makefile.ins

Categories

(Firefox Build System :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: Fallen, Assigned: bokeefe)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

I have two Makefiles that use MOZ_CHROME_FILE_FORMAT. Does it make sense to add this to moz.build instead? I could probably remove 1-2 makefiles this way.
(In reply to Philipp Kewisch [:Fallen] from comment #0)
> I have two Makefiles that use MOZ_CHROME_FILE_FORMAT. Does it make sense to
> add this to moz.build instead? I could probably remove 1-2 makefiles this
> way.

It looks like it should be easy to make it a passthrough variable in moz.build. Is this for testing/specialpowers and testing/mochitest? Does anyone know if the explicit MOZ_CHROME_FILE_FORMATS are still necessary there? It's possible we can just remove them entirely...
No, its for packaging Lightning locales. We use MOZ_CHROME_FILE_FORMAT=jar, otherwise the files are extracted one by one when the xpi is installed. I don't know if its been fixed, but the extraction of the xpi is done synchronous and triggers a long running script warning. If the user clicks "stop script", then Lightning is left in some limbo state which is hard to recover from. Packing the locales as .jar files reduces file size a bit and makes the extraction a lot quicker.

http://mxr.mozilla.org/comm-central/search?string=MOZ_CHR&find=calendar&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
You know xpis don't have to be extracted when installed, right?
Yes, but we still have a binary xpcom component, so it must. Trying to get rid of that, but that won't happen for the next major release.
I'm not following. You said lightning locales. Don't tell me those have a binary xpcom component.
Sorry, I should have been more elaborate. Lightning itself contains a binary component. The locales are not packaged as langpacks, they are included in the main lightning.xpi. When we finally release there is one lightning.xpi per platform, that contains the extension itself and all locales. Example directory structure, abbreviated:

lightning.xpi
├── calendar-js
├── chrome
│   ├── calendar
│   │   ├── content
│   │   └── skin
│   ├── icons
│   ├── lightning
│   │   ├── content
│   │   └── skin
│   ├── calendar-en-US.jar
│   ├── lightning-en-US.jar
│   ├── ...
│   ├── calendar-zh-TW.jar
│   └── lightning-zh-TW.jar
├── components
│   ├── ...
│   └── libcalbasecomps.so
├── defaults
├── modules
├── chrome.manifest
└── install.rdf

We use MOZ_CHROME_FILE_FORMAT to make sure the locale files, calendar-AB-CD and lightning-AB-CD are jarred.
Assignee: nobody → bokeefe
Status: NEW → ASSIGNED
Attachment #8596770 - Flags: review?(mshal)
Blocks: nomakefiles
Nice coincidence :-) I am just getting rid of MOZ_CHROME_FILE_FORMAT in calendar and now its being patched. Its good to see how easy it is to port variables, I might send a few patches myself in the future.
I think at this point we should just remove MOZ_CHROME_FILE_FORMAT.
Comment on attachment 8596770 [details] [diff] [review]
Move MOZ_CHROME_FILE_FORMAT to moz.build

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

::: python/mozbuild/mozbuild/frontend/context.py
@@ +1150,5 @@
> +
> +        Setting MOZ_CHROME_FILE_FORMAT overrides the default value set during
> +        configure with --enable-chrome-format.
> +
> +        ``MOZ_CHROME_FILE_FORMAT`may be set to one of the following:

Drop one ` and add a space

@@ +1158,5 @@
> +        There are two other possible values, which cannot be set per directory:
> +        - symlink:   Like ``flat``, but using symlinks instead of file copies.
> +                     This option is deprecated.
> +        - omni:      Like ``jar``, but files are packaged into the Omnijar.
> +                     This option can only be set at configure time.

So why mention them here?
(In reply to Mike Hommey [:glandium] from comment #9)
> I think at this point we should just remove MOZ_CHROME_FILE_FORMAT.

I pushed a patch to try that just removes the two occurrences in Makefile.ins, to get a feel for how hard that would be: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3358947a05bd

I'm pretty sure the specialpowers one is entirely unnecessary - flat packaging is already the default on Windows and Android, and symlink packaging didn't exist when it was added, but Linux and OSX didn't have a problem with that.

The one in mochitest I'm not sure about. That's building the mochikit.jar inside a mochijar extension, and was added in bug 543800. Evidently there are no Android API 9 debug tests on try, so I triggered API 11 tests, but those haven't finished yet.


(In reply to :Ms2ger from comment #10)
> @@ +1158,5 @@
> > +        There are two other possible values, which cannot be set per directory:
> > +        - symlink:   Like ``flat``, but using symlinks instead of file copies.
> > +                     This option is deprecated.
> > +        - omni:      Like ``jar``, but files are packaged into the Omnijar.
> > +                     This option can only be set at configure time.
> 
> So why mention them here?

Just for completeness. I can take them out if we end up keeping the variable; the configure option tells you what's allowed at configure time anyway.
(In reply to Brian O'Keefe [:bokeefe] from comment #11)
> (In reply to Mike Hommey [:glandium] from comment #9)
> > I think at this point we should just remove MOZ_CHROME_FILE_FORMAT.
> 
> I pushed a patch to try that just removes the two occurrences in
> Makefile.ins, to get a feel for how hard that would be:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3358947a05bd

It took a while to get earlier patches working, but try is okay with just removing them.

Properly working try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8148fcc121ec
Attachment #8596770 - Attachment is obsolete: true
Attachment #8596770 - Flags: review?(mshal)
Attachment #8597966 - Flags: review?(mh+mozilla)
Attachment #8597966 - Flags: review?(mh+mozilla) → review+
Note lightning/thunderbird likely needs changes too.
Lightning is now free of MOZ_CHROME_FILE_FORMAT, Thunderbird just has one in confvars.sh at http://mxr.mozilla.org/comm-central/source/mail/confvars.sh#11 Not sure if that needs to go in mail/moz.build or if it should stay in confvars.sh as is.
Those in confvars.sh can stay.
Keywords: checkin-needed
Summary: Add MOZ_CHROME_FILE_FORMAT to moz.build → Remove MOZ_CHROME_FILE_FORMAT from Makefile.ins
https://hg.mozilla.org/mozilla-central/rev/3993d39d6b1c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.