Closed Bug 1256979 Opened 4 years ago Closed 4 years ago

Move MOZ_CHROME_FILE_FORMAT to Python configure

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

The ultimate value depends on $gonkdir being set, which looks tied up with toolchain stuff, but defining the option in Python configure and piping it through with add_old_configure_assignment would be straightforward enough.
(In reply to Chris Manchester (:chmanchester) from comment #0)
> The ultimate value depends on $gonkdir being set

Actually no. It's set to omni in browser/confvars.sh and embedding/ios/confvars.sh. Then it's set in old-configure.in for b2g and android where that could have been set in b2g/confvars.sh and mobile/android/confvars.sh.
BTW, I looked at MOZ_CHROME_FILE_FORMAT already, and was trying to find a non-verbose way to set its default in $build_project/moz.configure while allowing to override it with --enable-chrome-format (we have the same concern for other confvars.sh things).
Assignee: nobody → cmanchester
This option's behaviour appears to have degraded over the years -
the default was set to 'jar' in old-configure.in, yet the due to other
checks and settings in confvars, the de-facto default has become 'omni'.
Further, this value does not actually propagate to the MOZ_CHROME_FILE_FORMAT
variable (which is always 'symlink', except on windows and android, where it
is 'flat'), but does influence MOZ_PACKAGER_FORMAT.

Additionally, MOZ_OMNIJAR is not being checked in a way influenced by
AC_DEFINE, so it is not passed to set_define in Python configure.

Review commit: https://reviewboard.mozilla.org/r/45537/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45537/
Attachment #8740070 - Flags: review?(mh+mozilla)
Comment on attachment 8740070 [details]
MozReview Request: Bug 1256979 - Move MOZ_CHROME_FILE_FORMAT to Python configure. r=glandium

https://reviewboard.mozilla.org/r/45537/#review42179

Since things are very confusing, let's start with how we ended up in the current situation.

Once upon a time, MOZ_CHROME_FILE_FORMAT could take 3 values: flat, symlink or jar. Those values directly influence how things end up in dist/bin. flat means dist/bin/chrome/* are directories with actual content. symlink means the same, but with symbolic links to the corresponding source files. jar means dist/bin/chrome* are jar files with the corresponding contents. In the packaged dist/firefox, symlink would become the same as flat, though.

That's well before omnijar came into existence, at which point things got complicated: MOZ_CHROME_FILE_FORMAT would designate *both* the format in dist/bin and the packaged format in dist/firefox. It would take 4 values: flat, symlink, jar or omni. The first three had the same meaning as before, but omni meant dist/bin would be filled as flat or symlink, depending on the building OS, and dist/firefox would have an omnijar.

Then came the new packager code, which changed things further, where dist/bin would only be either flat or symlink, while dist/firefox would be flat, jar or omni. This is when MOZ_PACKAGER_FORMAT was added into the mix, making things slightly clearer, albeit, confusing that MOZ_CHROME_FILE_FORMAT doesn't match --enable-chrome-format.

I think 3 years after the new packager code landed, it's time we clean things up. There are very few remaining uses of MOZ_CHROME_FILE_FORMAT as per its value after old-configure in the tree. In fact, there are only three:
- One is a Makefile test,
- One is a transitional thing that, 3 years later can just go away
- One is fed to jar maker (because it's jar maker that creates the dist/bin files as either files or symlinks)

Note the latter is happily ignored by the fastermake backend, that just uses install manifests, that simply use symlinks when they can.

On the long term, jar maker will die. On the medium term, jar maker could use the same infrastructure as install manifests and not care about the difference between flat and symlink itself. On the short term, we can reasonably rename MOZ_CHROME_FILE_FORMAT to something causing less confusion. I'd prefer if you did that rather than try to explain why MOZ_CHROME_FILE_FORMAT doesn't match --enable-chrome-format.

::: toolkit/moz.configure:388
(Diff revision 1)
> +    if toolkit in ('windows', 'android'):
> +        return 'flat'

toolkit == "windows" is not /exactly/ the same as OS_ARCH == "WINNT", and we should look at the intent, here. And while OS_ARCH is the wrong test for that as well, the intent is that we don't use symlinks when the OS on which we do the build is Windows. Which has nothing to do with the toolkit being "windows", especially when we'll finally have android builds working on Windows. I also think the reason the android check was added a long time ago, well before the new packager code, is not a problem anymore, so that check can very probably go away.

IOW, I think the check should now be on host.os == 'WINNT'.
Attachment #8740070 - Flags: review?(mh+mozilla)
also note that the only place that uses the MOZ_OMNIJAR subst is the same as the one using MOZ_CHROME_FILE_FORMAT for the transitional thing I mentioned could be removed. So the MOZ_OMNIJAR subst can go away as well.
Comment on attachment 8740070 [details]
MozReview Request: Bug 1256979 - Move MOZ_CHROME_FILE_FORMAT to Python configure. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45537/diff/1-2/
Attachment #8740070 - Flags: review?(mh+mozilla)
Comment on attachment 8740070 [details]
MozReview Request: Bug 1256979 - Move MOZ_CHROME_FILE_FORMAT to Python configure. r=glandium

https://reviewboard.mozilla.org/r/45537/#review42481

100% nits

::: config/config.mk:230
(Diff revision 2)
>  endif # MOZ_PROFILE_USE
>  endif # NO_PROFILE_GUIDED_OPTIMIZE
>  
>  MAKE_JARS_FLAGS = \
>  	-t $(topsrcdir) \
> -	-f $(MOZ_CHROME_FILE_FORMAT) \
> +	-f $(MOZ_TARGET_FILE_FORMAT) \

I think MOZ_JAR_MAKER_FILE_FORMAT would make it even clearer.

::: old-configure.in
(Diff revision 2)
> -if test "$MOZ_CHROME_FILE_FORMAT" = "omni"; then
> -    MOZ_OMNIJAR=1
> -    AC_DEFINE(MOZ_OMNIJAR)
> -fi
> -MOZ_PACKAGER_FORMAT="$MOZ_CHROME_FILE_FORMAT"
> -if test "$OS_ARCH" = "WINNT" -o "$MOZ_WIDGET_TOOLKIT" = "android"; then

I feel like removing the android check here should be done in a separate patch (as in removing it from old-configure.in first, and then moving the check to moz.configure).

::: toolkit/moz.configure:359
(Diff revision 2)
>  
>  set_config('MOZ_EME', eme)
>  set_define('MOZ_EME', eme)
>  set_config('MOZ_EME_MODULES', eme_modules)
> +
> +option(name='--enable-chrome-format',

while here, I think we should make the same kind of thing that was done for --enable-application, which is to add a better named option as well (which for --enable-application was --enable-project), and keep the old option for backwards compatibility (and we can then have a plan to push people from one to the other later).

I think --with-packager-format would be a better option name.

Feel free to push that to a followup.

::: toolkit/moz.configure:362
(Diff revision 2)
>  set_config('MOZ_EME_MODULES', eme_modules)
> +
> +option(name='--enable-chrome-format',
> +       help='Select FORMAT of chrome files during packaging',
> +       nargs=1,
> +       choices=('jar', 'flat', 'omni'),

Considering choices are output in help, it would be better to put omni first, I guess, although that could be done automatically by the code creating the help, but it doesn't do it at the moment.

::: toolkit/moz.configure:373
(Diff revision 2)
> +    if host.os == 'WINNT':
> +        return 'flat'
> +    return 'symlink'

You could make it a one-liner.

::: toolkit/moz.configure:390
(Diff revision 2)
> +    if toolkit == 'android':
> +        return 'assets/omni.ja'
> +    return 'omni.ja'

Likewise.

::: toolkit/mozapps/installer/upload-files.mk
(Diff revision 2)
> -# For smooth transition of comm-central
> -ifndef MOZ_PACKAGER_FORMAT
> -  ifeq ($(MOZ_CHROME_FILE_FORMAT),flat)
> -    ifdef MOZ_OMNIJAR
> -      MOZ_PACKAGER_FORMAT := omni
> -    else
> -      MOZ_PACKAGER_FORMAT := flat
> -    endif
> -  endif
> -endif

I feel this would be better separated in its own patch
Attachment #8740070 - Flags: review?(mh+mozilla) → review+
https://reviewboard.mozilla.org/r/45537/#review42481

> while here, I think we should make the same kind of thing that was done for --enable-application, which is to add a better named option as well (which for --enable-application was --enable-project), and keep the old option for backwards compatibility (and we can then have a plan to push people from one to the other later).
> 
> I think --with-packager-format would be a better option name.
> 
> Feel free to push that to a followup.

I'll push this to a follow up -- I think we're having an issue where an implied option, if supplied on the command line, will conflict with a default provided by the implier. So if I do the obvious thing and put --enable-packager-format=flat in my mozconfig, I get `mozbuild.configure.options.InvalidOptionError: '--enable-packager-format=omni' implied by '--enable-chrome-format' conflicts with '--enable-packager-format=flat' from the command-line`
Comment on attachment 8740070 [details]
MozReview Request: Bug 1256979 - Move MOZ_CHROME_FILE_FORMAT to Python configure. r=glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45537/diff/2-3/
Comment on attachment 8741079 [details]
MozReview Request: Bug 1256979 - Don't set MOZ_CHROME_FILE_FORMAT to 'flat' for android builds. r=glandium

https://reviewboard.mozilla.org/r/46177/#review42745
Attachment #8741079 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8741080 [details]
MozReview Request: Bug 1256979 - Remove MOZ_OMNIJAR from old-configure and upload-files.mk. r=glandium

https://reviewboard.mozilla.org/r/46179/#review42747

About the commit message: IIRC, MOZ_OMNIJAR had more use initially, and was not, in itself the transitioning element. The transition was to set MOZ_PACKAGER_FORMAT properly depending on MOZ_CHROME_FILE_FORMAT and MOZ_OMNIJAR, which is not needed anymore, and was the last use of MOZ_OMNIJAR.
Attachment #8741080 - Flags: review?(mh+mozilla) → review+
Blocks: 1265486
This caused bug 1265486, which is blocking us from enabling Aurora updates on mobile. Can you please fix that bug? Or should we back this out?
Flags: needinfo?(cmanchester)
(In reply to :Margaret Leibovic from comment #16)
> This caused bug 1265486, which is blocking us from enabling Aurora updates
> on mobile. Can you please fix that bug? Or should we back this out?

We have a fix there.
Flags: needinfo?(cmanchester)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.