Closed Bug 1407285 Opened 7 years ago Closed 6 years ago

Support spaces in MOZ_MACBUNDLE_NAME

Categories

(Firefox Build System :: General, defect)

Unspecified
macOS
defect
Not set
normal

Tracking

(firefox58 wontfix, firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

Otherwise, ./mach package is failing
Attached patch space.diff (obsolete) — Splinter Review
Attachment #8917028 - Attachment is obsolete: true
Depends on: 1408746
Assignee: nobody → sledru
Comment on attachment 8928888 [details]
Bug 1407285 - Support spaces in MOZ_MACBUNDLE_NAME and in various Makefile and tools

https://reviewboard.mozilla.org/r/200200/#review205438

These changes all look fine to me, but you will want to get an r+ from someone in "Build and Release Tools"[1] and/or "Build Config"[2] for these changes.

[1] https://wiki.mozilla.org/Modules/All#Build_and_Release_Tools
[2] https://wiki.mozilla.org/Modules/All#Build_Config
Attachment #8928888 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8928889 [details]
Bug 1407285 - Handle path with spaces in tools/update-packaging/

https://reviewboard.mozilla.org/r/200202/#review205440
Attachment #8928889 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8928890 [details]
Bug 1407285 - Handle path with spaces in the ident: target Makefile in browser/locales

https://reviewboard.mozilla.org/r/200204/#review205434

::: commit-message-ea1c8:1
(Diff revision 1)
> +Bug 1407285 - Handle path with spaces in ident: in browser/locales r?spohl

nit: fix commit message
Attachment #8928890 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8928888 [details]
Bug 1407285 - Support spaces in MOZ_MACBUNDLE_NAME and in various Makefile and tools

https://reviewboard.mozilla.org/r/200200/#review208826

::: browser/app/Makefile.in:86
(Diff revision 2)
>  MAC_BUNDLE_VERSION = $(shell $(PYTHON) $(srcdir)/macversion.py --version=$(MOZ_APP_VERSION) --buildid=$(DEPTH)/buildid.h)
>  
>  .PHONY: repackage
>  tools repackage:: $(DIST)/bin/$(MOZ_APP_NAME) features
>  	rm -rf $(dist_dest)
> -	$(MKDIR) -p $(dist_dest)/Contents/MacOS
> +	$(MKDIR) -p "$(dist_dest)/Contents/MacOS"

Please use single quotes.

::: old-configure.in:4505
(Diff revision 2)
>  MOZ_ARG_WITH_STRING(macbundlename-prefix,
>  [  --with-macbundlename-prefix=prefix
>                            Prefix for MOZ_MACBUNDLE_NAME],
>  [ MOZ_MACBUNDLE_NAME_PREFIX="$withval"])
>  
> -MOZ_MACBUNDLE_NAME=`echo $MOZ_APP_DISPLAYNAME | tr -d ' '`
> +MOZ_MACBUNDLE_NAME=`echo $MOZ_APP_DISPLAYNAME`

No need for `echo`

::: old-configure.in:4511
(Diff revision 2)
>  if test "$MOZ_MACBUNDLE_NAME_PREFIX"; then
>    MOZ_MACBUNDLE_NAME="${MOZ_MACBUNDLE_NAME_PREFIX}${MOZ_MACBUNDLE_NAME}"
>  fi
>  
>  if test "$MOZ_DEBUG"; then
>    MOZ_MACBUNDLE_NAME=${MOZ_MACBUNDLE_NAME}Debug.app

This should probably change to add a space. Same for MOZ_MACBUNDLE_NAME_PREFIX.
Attachment #8928888 - Flags: review?(mh+mozilla)
Comment on attachment 8928889 [details]
Bug 1407285 - Handle path with spaces in tools/update-packaging/

https://reviewboard.mozilla.org/r/200202/#review208828

This should be folded with the previous patch.
Attachment #8928889 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8928890 [details]
Bug 1407285 - Handle path with spaces in the ident: target Makefile in browser/locales

https://reviewboard.mozilla.org/r/200204/#review208830

::: browser/locales/Makefile.in:198
(Diff revision 2)
>  endif
>  
>  ident:
>  	@printf 'fx_revision '
>  	@$(PYTHON) $(topsrcdir)/config/printconfigsetting.py \
> -	    $(STAGEDIST)/application.ini App SourceStamp
> +	    "$(STAGEDIST)"/application.ini App SourceStamp

single quotes
Attachment #8928890 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8928890 [details]
Bug 1407285 - Handle path with spaces in the ident: target Makefile in browser/locales

https://reviewboard.mozilla.org/r/200204/#review208832

This should be folded too.
Attachment #8928889 - Attachment is obsolete: true
Attachment #8928890 - Attachment is obsolete: true
Comment on attachment 8928888 [details]
Bug 1407285 - Support spaces in MOZ_MACBUNDLE_NAME and in various Makefile and tools

https://reviewboard.mozilla.org/r/200200/#review210064
Attachment #8928888 - Flags: review?(mh+mozilla) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9fff8b2fcf78
Support spaces in MOZ_MACBUNDLE_NAME and in various Makefile and tools r=glandium
https://hg.mozilla.org/mozilla-central/rev/9fff8b2fcf78
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Why I didn't have this issue in autoland?
Flags: needinfo?(sledru)
(In reply to Sylvestre Ledru [:sylvestre] from comment #25)
> Why I didn't have this issue in autoland?

Try and autoland don't build l10n builds by default, Callek's working on that as we speak.

I've kicked off some try-magic on your review request on https://treeherder.mozilla.org/#/jobs?repo=try&revision=f77d00b63077ef671ff307ba8e845d277a49c63a, just to make sure we're not missing stuff.
Sylvestre, can you rebase these on top of a good m-c changeset?

I'd like to see results on Windows, and they're busted in signing, on top of the Android nightly that's busted for things underneath.
Flags: needinfo?(sledru)
Voila!
Flags: needinfo?(sledru)
NI to make sure you see it!
Flags: needinfo?(l10n)
Comment on attachment 8934486 [details]
Bug 1407285 - Support spaces in toolkit/locales/l10n.mk

https://reviewboard.mozilla.org/r/205402/#review211386
Attachment #8934486 - Flags: review?(l10n) → review+
Thanks for the try builds. I didn't find any windows codepaths that have spaces. Windows is sadly way more erratic on quoting, but as long as we don't go down there, this should be good.
Flags: needinfo?(l10n)
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f62a6e41778
Support spaces in toolkit/locales/l10n.mk r=Pike
https://hg.mozilla.org/integration/autoland/rev/72a55f390248
Support spaces in MOZ_MACBUNDLE_NAME and in various Makefile and tools r=glandium
https://hg.mozilla.org/mozilla-central/rev/6f62a6e41778
https://hg.mozilla.org/mozilla-central/rev/72a55f390248
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Tom, please open a new bug for follow up changes. Thanks
Flags: needinfo?(mozilla)
Comment on attachment 8935233 [details]
Bug 1407285: Use `'` consistently in make, and fix package-compare;

https://reviewboard.mozilla.org/r/206110/#review212048
Attachment #8935233 - Flags: review?(mh+mozilla) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0bc64e08d846
Use `'` consistently in make, and fix package-compare; r=glandium
Flags: needinfo?(mozilla)
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/comm-central/rev/564d515b57a1
Port bug 1407285 to Thunderbird and SeaMonkey; rs=bustage-fix
Blocks: 1424098
It looks like this breaks updates on OS X in at least one case. We're seeing this on maple when generating complete MARs:
[task 2017-12-07T22:18:03.511Z] 22:18:03     INFO -  ~/workspace/build/src/obj-firefox/dist/firefox/Firefox Nightly.app ~/workspace/build/src/obj-firefox/tools/update-packaging
[task 2017-12-07T22:18:03.512Z] 22:18:03     INFO -  ~/workspace/build/src/obj-firefox/tools/update-packaging
[task 2017-12-07T22:18:03.512Z] 22:18:03     INFO -  Adding type instruction to update manifests
[task 2017-12-07T22:18:03.512Z] 22:18:03     INFO -         type complete
[task 2017-12-07T22:18:03.513Z] 22:18:03     INFO -  Adding file add instructions to update manifests
[task 2017-12-07T22:18:03.513Z] 22:18:03     INFO -          add "Contents/Resources/updater.ini"
[task 2017-12-07T22:18:03.513Z] 22:18:03     INFO -  /builds/worker/workspace/build/src/tools/update-packaging/common.sh: line 93: [: too many arguments
[task 2017-12-07T22:18:03.513Z] 22:18:03     INFO -   add-if-not "Contents/Resources/update-settings.ini" "Contents/Resources/update-settings.ini"
[task 2017-12-07T22:18:03.514Z] 22:18:03     INFO -  /builds/worker/workspace/build/src/tools/update-packaging/common.sh: line 127: $filev3: ambiguous redirect
[task 2017-12-07T22:18:03.514Z] 22:18:03     INFO -          add "Contents/Resources/update-settings.ini" (forced)
[task 2017-12-07T22:18:03.515Z] 22:18:03     INFO -          add "Contents/Resources/res/cursors/zoomOut@2x.png"
[task 2017-12-07T22:18:03.515Z] 22:18:03     INFO -  /builds/worker/workspace/build/src/tools/update-packaging/common.sh: line 93: [: too many arguments
[task 2017-12-07T22:18:03.515Z] 22:18:03     INFO -          add "Contents/Resources/res/cursors/zoomOut.png"

... and we end up with an empty updatev3.manifest (which ends up removing the entire install).

It seems like this code has successfully been used for mozilla-central nightlies already, so I'm not quite sure what the difference is. We're doing Betas and DevEdition on maple, so it could be something that only shows up after an uplift?
Depends on: 1424294
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: