Closed Bug 1345422 Opened 3 years ago Closed 3 years ago

Thunderbird Daily l10n repacks failing for Windows as of 2017-03-06

Categories

(Thunderbird :: Build Config, defect)

x86
Windows
defect
Not set

Tracking

(thunderbird54 fixed, thunderbird55 fixed)

RESOLVED FIXED
Thunderbird 55.0
Tracking Status
thunderbird54 --- fixed
thunderbird55 --- fixed

People

(Reporter: jorgk-bmo, Assigned: ewong)

References

Details

Attachments

(5 files, 12 obsolete files)

3.94 KB, patch
ewong
: review+
Details | Diff | Splinter Review
1.04 KB, patch
iann_bugzilla
: review+
Details | Diff | Splinter Review
1.05 KB, patch
clokep
: review+
Details | Diff | Splinter Review
1.06 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
2.52 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
c:/builds/moz2_slave/tb-c-cen-w32-l10n-ntly-0000000/build/comm-central/mozilla/toolkit/locales/l10n.mk:115: recipe for target 'repackage-zip' failed
mozmake.exe[2]: Leaving directory 'c:/builds/moz2_slave/tb-c-cen-w32-l10n-ntly-0000000/build/comm-central/objdir-tb/mail/locales'
Makefile:109: recipe for target 'repackage-win32-installer' failed
mozmake.exe[1]: Leaving directory 'c:/builds/moz2_slave/tb-c-cen-w32-l10n-ntly-0000000/build/comm-central/objdir-tb/mail/locales'
Makefile:122: recipe for target 'repackage-win32-installer-af' failed
Traceback (most recent call last):
  File "c:\mozilla-build\python27\Lib\runpy.py", line 162, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "c:\mozilla-build\python27\Lib\runpy.py", line 72, in _run_code
    exec code in run_globals
  File "c:\builds\moz2_slave\tb-c-cen-w32-l10n-ntly-0000000\build\comm-central\mozilla\python\mozbuild\mozbuild\action\7z_exe_archive.py", line 35, in <module>
    sys.exit(main(sys.argv[1:]))
  File "c:\builds\moz2_slave\tb-c-cen-w32-l10n-ntly-0000000\build\comm-central\mozilla\python\mozbuild\mozbuild\action\7z_exe_archive.py", line 31, in main
    archive_exe(args[0], args[1], args[2])
  File "c:\builds\moz2_slave\tb-c-cen-w32-l10n-ntly-0000000\build\comm-central\mozilla\python\mozbuild\mozbuild\action\7z_exe_archive.py", line 16, in archive_exe
    subprocess.check_call(['upx', '--best', '-o', mozpath.join(tmpdir, '7zSD.sfx'), '../../../other-licenses/7zstub/firefox/7zSD.sfx'])
  File "c:\mozilla-build\python27\Lib\subprocess.py", line 542, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['upx', '--best', '-o', 'c:/users/cltbld/appdata/local/temp/tmpyiojho/7zSD.sfx', '../../../other-licenses/7zstub/firefox/7zSD.sfx']' returned non-zero exit status 1
mozmake.exe[2]: *** [repackage-zip] Error 1
mozmake.exe[1]: *** [repackage-win32-installer] Error 2
pymake\..\..\mozmake.exe: *** [repackage-win32-installer-af] Error 2
These localised version are more important than ever before, since due to bug 1344594 you *must* use a localised version to get the date/time format you're used to.
One line prior to #1 is more important.

upx: ../../../other-licenses/7zstub/firefox/7zSD.sfx: FileNotFoundException: ../../../other-licenses/7zstub/firefox/7zSD.sfx
Blocks: 1287881
Takanori is correct.

Bug 1287881 introduced this 7z_exe_archive.py which hardcodes
firefox's ../../../other-licenses/7zstub/firefox/7zSD.sfx

That obviously needs to be adjusted.
(In reply to Edmund Wong (:ewong) from comment #5)
> Takanori is correct.

I meant.. Takanori-san.
Attached patch [m-c] proposed patch (obsolete) — Splinter Review
this is for m-c
Attachment #8846229 - Flags: review?(mshal)
Attached patch [c-c] proposed patch for TB (obsolete) — Splinter Review
Attachment #8846230 - Flags: review?(Pidgeot18)
Obviously, tb's patch depends on the m-c patch.

Rationale for the m-c is to specify the sfx and installer paths.

Though I probably can make it 'better' since both products use
the same path depths and patterns, the MOZ_APP_NAME can be 
inserted into the sfx package. 
i.e. '../../../other-licenses/7zstub/%s/7zSD.sfx' % app_name

where app_name == MOZ_APP_NAME.

ditto with the installer path; but not quite sure where to determine
the firefox->browser part.
Attached patch [c-c] proposed patch for TB (obsolete) — Splinter Review
bad c&p.
Attachment #8846230 - Attachment is obsolete: true
Attachment #8846230 - Flags: review?(Pidgeot18)
Attachment #8846232 - Flags: review?(Pidgeot18)
Attached patch [tb] proposed patch for TB (obsolete) — Splinter Review
sorry...  incompetent at submitting patches and reading..  my apologies.
Attachment #8846232 - Attachment is obsolete: true
Attachment #8846232 - Flags: review?(Pidgeot18)
Attachment #8846233 - Flags: review?(Pidgeot18)
Attached patch [m-c] proposed patch (obsolete) — Splinter Review
realized I had not used the right final paths in archive_exe.
Attachment #8846229 - Attachment is obsolete: true
Attachment #8846229 - Flags: review?(mshal)
Attachment #8846234 - Flags: review?(mshal)
Attachment #8846233 - Attachment description: [c-c] proposed patch for TB → [tb] proposed patch for TB
Attached patch [suite] proposed patch for suite (obsolete) — Splinter Review
Attachment #8846235 - Flags: review?(iann_bugzilla)
Attached patch [im] proposed patch (obsolete) — Splinter Review
Attachment #8846236 - Flags: review?(clokep)
Assignee: nobody → ewong
Comment on attachment 8846233 [details] [diff] [review]
[tb] proposed patch for TB

Sadly we haven't see Joshua in ages :-(
Thanks for the patches!
Attachment #8846233 - Flags: review?(Pidgeot18) → review?(aleth)
Comment on attachment 8846235 [details] [diff] [review]
[suite] proposed patch for suite

Assuming the m-c patch gets an r+ r=me
Attachment #8846235 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8846236 [details] [diff] [review]
[im] proposed patch

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

Thanks! :-)
Attachment #8846236 - Flags: review?(clokep) → review+
Comment on attachment 8846234 [details] [diff] [review]
[m-c] proposed patch

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

::: toolkit/mozapps/installer/upload-files.mk
@@ +119,5 @@
>  endif
>  
>  ifeq ($(MOZ_PKG_FORMAT),SFX7Z)
>    PKG_SUFFIX	= .exe
> +  INNER_MAKE_PACKAGE = $(call py_action,7z_exe_archive,'$(MOZ_PKG_DIR)' 'app.tag' '$(PACKAGE)' '$(MOZ_SFX_PACKAGE)' '$(MOZ_INSTALLER_PATH)')

for some reasons that I have yet to understand, MOZ_SFX_PACKAGE from confvars.sh isn't being recognized.

what am I missing?
Comment on attachment 8846234 [details] [diff] [review]
[m-c] proposed patch

>+MOZ_SFX_PACKAGE=other-licenses/7zstub/firefox/7zSD.sfx
>+MOZ_INSTALLER_PATH=browser/installer/windows
...
>+    sfxpkg = '../../../%s' % sfx_package
>+    instpath = '../../../%s' % installer_path

Can you use $(topsrcdir) in the confvars.sh definitions? Or if not, maybe in the INNER_MAKE_PACKAGE definition. It's a little awkward to have the '../../..' in the python script now that the path is being passed in.

>+  INNER_MAKE_PACKAGE = $(call py_action,7z_exe_archive,'$(MOZ_PKG_DIR)' 'app.tag' '$(PACKAGE)' '$(MOZ_SFX_PACKAGE)' '$(MOZ_INSTALLER_PATH)')

1) I'd prefer if $(PACKAGE) remained the last argument, since that's the output, and app.tag and the sfx package are inputs (so it'll be [input, input, output] instead of [input, output, input]).

2) Since MOZ_INSTALLER_PATH is only used to find app.tag, we can probably just do '$(MOZ_INSTALLER_PATH)/app.tag' so that the tag and sfx header are both passed in with full paths.

I'd like to re-review after these changes, so f+ for now.
Attachment #8846234 - Flags: review?(mshal) → feedback+
(In reply to Michael Shal [:mshal] from comment #19)
> Comment on attachment 8846234 [details] [diff] [review]
> [m-c] proposed patch
> 
> >+MOZ_SFX_PACKAGE=other-licenses/7zstub/firefox/7zSD.sfx
> >+MOZ_INSTALLER_PATH=browser/installer/windows
> ...
> >+    sfxpkg = '../../../%s' % sfx_package
> >+    instpath = '../../../%s' % installer_path
> 
> Can you use $(topsrcdir) in the confvars.sh definitions? Or if not, maybe in
> the INNER_MAKE_PACKAGE definition. It's a little awkward to have the
> '../../..' in the python script now that the path is being passed in.

I'll try with topsrcdir and at worst, add it in the INNER_MAKE_PACKAGE.

> 
> >+  INNER_MAKE_PACKAGE = $(call py_action,7z_exe_archive,'$(MOZ_PKG_DIR)' 'app.tag' '$(PACKAGE)' '$(MOZ_SFX_PACKAGE)' '$(MOZ_INSTALLER_PATH)')
> 
> 1) I'd prefer if $(PACKAGE) remained the last argument, since that's the
> output, and app.tag and the sfx package are inputs (so it'll be [input,
> input, output] instead of [input, output, input]).

The problem I've come across (and have yet to figure out) is 
in comment #18 where both MOZ_SFX_PACKAGE and MOZ_INSTALLER_PATH
are empty for browser as shown in https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f4da29a4eb687fbe39a4aa0a229d974e94d69bb&selectedJob=83283382
where it can't find sfx_package (and installer_path).  I did another
try push and got:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=69284eaa91d03c5e1a410ef825af2fadcd5e036c

while it looks green, it's because I hardcoded the two paths
if neither sfx_package and installer_path were defined.

Looking at the log from that push:

02:03:58     INFO -  Compressing  core\browser\extensions\{972ce4c6-7e08-4474-a285-3208198ce6fd}.xpi
02:03:58     INFO -  Everything is Ok
02:03:58     INFO -  args:
02:03:58     INFO -      0 - firefox
02:03:58     INFO -      1 - app.tag
02:03:58     INFO -      2 - firefox-55.0a1.x-test.win32.exe
02:03:58     INFO -      3 -
02:03:58     INFO -      4 -


> 
> 2) Since MOZ_INSTALLER_PATH is only used to find app.tag, we can probably
> just do '$(MOZ_INSTALLER_PATH)/app.tag' so that the tag and sfx header are
> both passed in with full paths.

That seems straight forward enough.

Thanks
(In reply to Michael Shal [:mshal] from comment #19)
> Comment on attachment 8846234 [details] [diff] [review]
> [m-c] proposed patch
> 
> >+MOZ_SFX_PACKAGE=other-licenses/7zstub/firefox/7zSD.sfx
> >+MOZ_INSTALLER_PATH=browser/installer/windows
> ...
> >+    sfxpkg = '../../../%s' % sfx_package
> >+    instpath = '../../../%s' % installer_path
> 
> Can you use $(topsrcdir) in the confvars.sh definitions? Or if not, maybe in
> the INNER_MAKE_PACKAGE definition. It's a little awkward to have the
> '../../..' in the python script now that the path is being passed in.

I'll try with topsrcdir and at worst, add it in the INNER_MAKE_PACKAGE.

> 
> >+  INNER_MAKE_PACKAGE = $(call py_action,7z_exe_archive,'$(MOZ_PKG_DIR)' 'app.tag' '$(PACKAGE)' '$(MOZ_SFX_PACKAGE)' '$(MOZ_INSTALLER_PATH)')
> 
> 1) I'd prefer if $(PACKAGE) remained the last argument, since that's the
> output, and app.tag and the sfx package are inputs (so it'll be [input,
> input, output] instead of [input, output, input]).

The problem I've come across (and have yet to figure out) is 
in comment #18 where both MOZ_SFX_PACKAGE and MOZ_INSTALLER_PATH
are empty for browser as shown in https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f4da29a4eb687fbe39a4aa0a229d974e94d69bb&selectedJob=83283382
where it can't find sfx_package (and installer_path).  I did another
try push and got:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=69284eaa91d03c5e1a410ef825af2fadcd5e036c

while it looks green, it's because I hardcoded the two paths
if neither sfx_package and installer_path were defined.

Looking at the log from that push:

02:03:58     INFO -  Compressing  core\browser\extensions\{972ce4c6-7e08-4474-a285-3208198ce6fd}.xpi
02:03:58     INFO -  Everything is Ok
02:03:58     INFO -  args:
02:03:58     INFO -      0 - firefox
02:03:58     INFO -      1 - app.tag
02:03:58     INFO -      2 - firefox-55.0a1.x-test.win32.exe
02:03:58     INFO -      3 -
02:03:58     INFO -      4 -


> 
> 2) Since MOZ_INSTALLER_PATH is only used to find app.tag, we can probably
> just do '$(MOZ_INSTALLER_PATH)/app.tag' so that the tag and sfx header are
> both passed in with full paths.

That seems straight forward enough.

Thanks
(In reply to Michael Shal [:mshal] from comment #19)
> Comment on attachment 8846234 [details] [diff] [review]
> [m-c] proposed patch
> 
> >+MOZ_SFX_PACKAGE=other-licenses/7zstub/firefox/7zSD.sfx
> >+MOZ_INSTALLER_PATH=browser/installer/windows
> ...
> >+    sfxpkg = '../../../%s' % sfx_package
> >+    instpath = '../../../%s' % installer_path
> 
> Can you use $(topsrcdir) in the confvars.sh definitions? Or if not, maybe in
> the INNER_MAKE_PACKAGE definition. It's a little awkward to have the
> '../../..' in the python script now that the path is being passed in.
> 
> >+  INNER_MAKE_PACKAGE = $(call py_action,7z_exe_archive,'$(MOZ_PKG_DIR)' 'app.tag' '$(PACKAGE)' '$(MOZ_SFX_PACKAGE)' '$(MOZ_INSTALLER_PATH)')
> 

I suppose once I've figured out why MOZ_SFX_PACKAGE and MOZ_INSTALLER_PATH
are empty,  my concerns with having $(PACKAGE) at the end will be moot.
Attached patch [m-c] proposed patch (v2) (obsolete) — Splinter Review
This includes changes that includes both mshal and glandium's comments.
(glandium's comments from irc: mainly "don't use confvars.sh").

Here's the try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=124e43efb8a00c5959aeb72425365b2bd4b914dd
Attachment #8846234 - Attachment is obsolete: true
Attachment #8847521 - Flags: review?(mshal)
and yes, this means I'll need to revisit those c-c patches. (my apologies to
my fellow c-c contributors for needing to review c-c patches again.  I think I got 
a bit enthusiastic in submitting patches when I should've waited for m-c's patch to 
be reviewed).
Comment on attachment 8847521 [details] [diff] [review]
[m-c] proposed patch (v2)

Just some nits, but can you please verify a Windows l10n build on try before landing?

>+    if len(args) != 4:
>+        print('Usage: 7z_exe_archive.py <pkg_dir> <full path to tagfile> <sfx_package> <package>',

Nit: I think just <tagfile> is fine, unless you also want to do <full path to sfx_package> for consistency.

>+        print('args:')
>+        for i in range(0, len(args)):
>+            print('    %d - %s' % (i, args[i]))

Is this some leftover debugging code? Do you think it's helpful to leave in or can we remove it?
Attachment #8847521 - Flags: review?(mshal) → review+
(In reply to Michael Shal [:mshal] from comment #24)
> Comment on attachment 8847521 [details] [diff] [review]
> [m-c] proposed patch (v2)
> 
> Just some nits, but can you please verify a Windows l10n build on try before
> landing?

I'll do that, thanks!

> 
> >+    if len(args) != 4:
> >+        print('Usage: 7z_exe_archive.py <pkg_dir> <full path to tagfile> <sfx_package> <package>',
> 
> Nit: I think just <tagfile> is fine, unless you also want to do <full path
> to sfx_package> for consistency.
> 

Ok. I'll just do <tagfile> as it's shorter though I'm concerned it might
be problematic as we'd really need to know to pass the path of the
tagfile/sfx package relative to the root of the source and not the
full path (since this isn't possible in automation).

> >+        print('args:')
> >+        for i in range(0, len(args)):
> >+            print('    %d - %s' % (i, args[i]))
> 
> Is this some leftover debugging code? Do you think it's helpful to leave in
> or can we remove it?

Ah sorry.  Yes, that needs to be removed.
Fixed with nits.
Attachment #8847521 - Attachment is obsolete: true
Attachment #8847882 - Flags: review+
Comment on attachment 8846233 [details] [diff] [review]
[tb] proposed patch for TB

Any more changes expected here? Aleth is away, so you'll have to accept my review ;-)
Attachment #8846233 - Flags: review?(aleth) → review+
(In reply to Jorg K (GMT+1) from comment #28)
> Comment on attachment 8846233 [details] [diff] [review]
> [tb] proposed patch for TB
> 
> Any more changes expected here? Aleth is away, so you'll have to accept my
> review ;-)

Yes. 

1) Need to make sure the try run is green.  I'm getting these bad intermittent
   bustages so just retriggered them.

2) I need to redo the c-c patches.
Attached patch [tb] proposed patch (v2) (obsolete) — Splinter Review
Attachment #8846233 - Attachment is obsolete: true
Attachment #8848018 - Flags: review?(aleth)
Attachment #8846235 - Attachment is obsolete: true
Attachment #8848019 - Flags: review?(iann_bugzilla)
Attachment #8846236 - Attachment is obsolete: true
Attachment #8848020 - Flags: review?(clokep)
Attachment #8848018 - Attachment is obsolete: true
Attachment #8848018 - Flags: review?(aleth)
Attachment #8848021 - Flags: review?(aleth)
Something was infra-horky with my previous try push, so here's a new one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eb5695ca7f450a028227a88d2a00680bcf02384
Please note:

The l10n *were* successful (afaict) from 
https://archive.mozilla.org/pub/firefox/try-builds/ewong@pw-wspx.org-5eb5695ca7f450a028227a88d2a00680bcf02384/try-win64/

but on treeherder the l10n jobs were purple/red due to timeout.
:aki explained that the whole mozharness was around 5 hrs but
the command timeout is set at 10800 (3 hrs).

Can this be considered not the patch's fault and just me triggering
the l10n jobs with the whole list of locales when only a few could
do.  I'll do another try push but this time with a few locales
just in case.
Comment on attachment 8848019 [details] [diff] [review]
[suite] proposed patch for suite (v2)

r=me
Attachment #8848019 - Flags: review?(iann_bugzilla) → review+
Attachment #8848020 - Flags: review?(clokep) → review+
Comment on attachment 8848021 [details] [diff] [review]
[tb] proposed patch (v2-fix)

Thanks, looking forward to localised builds.
Attachment #8848021 - Flags: review?(aleth) → review+
Is this ready to land? If so, please do.
Flags: needinfo?(ewong)
Pushed by ewong@pw-wspx.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e22ffe74d41
Do not hardcode the sfx package and installer path. r=mshal
(In reply to Jorg K (GMT+1) from comment #39)
> Is this ready to land? If so, please do.

pushed. to m-i
Status: NEW → ASSIGNED
Flags: needinfo?(ewong)
https://hg.mozilla.org/mozilla-central/rev/2e22ffe74d41
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
can this be uplifted to aurora?
That's broken, too? Oh, yes, it is. Yes, we'll request uplift once this is proven to be working in tomorrows Daily run, oh, if I fix the Windows bustage now (bug 1349482).
This landed yesterday, so after fixing some bustage, I started a Daily/Nightly run today. Sadly the repacks didn't work. There are no Windows executables in
http://ftp.mozilla.org/pub/thunderbird/nightly/2017/03/2017-03-23-09-43-40-comm-central-l10n/
or http://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-central-l10n/.

Reading one of the logs:
http://ftp.mozilla.org/pub/thunderbird/nightly/2017/03/2017-03-23-09-43-40-comm-central-l10n/comm-central-win32-l10n-nightly-en-GB-bm74-build1-build524.txt.gz
I see:

  CARGO=c:/builds/moz2_slave/tb-c-cen-w32-l10n-ntly-0000000/build/comm-central/rustc/bin/cargo
  MOZ_ADDON_SIGNING=0
  MOZ_REQUIRE_SIGNING=0
  VSPATH=/c/builds/moz2_slave/tb-c-cen-w32-l10n-ntly-0000000/build/comm-central/vs2015u3
  MOZ_REQUIRE_ADDON_SIGNING=0
  VSWINPATH=c:/builds/moz2_slave/tb-c-cen-w32-l10n-ntly-0000000/build/comm-central/vs2015u3
  RUSTC=c:/builds/moz2_slave/tb-c-cen-w32-l10n-ntly-0000000/build/comm-central/rustc/bin/rustc
checking for llvm-config... not found
checking for a shell... C:/mozilla-build/msys/bin/sh.exe
checking for host system type... i686-pc-mingw32
checking for target system type... i686-pc-mingw32
Traceback (most recent call last):
  File "c:/builds/moz2_slave/tb-c-cen-w32-l10n-ntly-0000000/build/comm-central/configure.py", line 32, in <module>
    sys.exit(main(sys.argv))
  File "c:/builds/moz2_slave/tb-c-cen-w32-l10n-ntly-0000000/build/comm-central/configure.py", line 24, in main
    sandbox.run(os.path.join(os.path.dirname(__file__), 'moz.configure'))
  File "c:\builds\moz2_slave\tb-c-cen-w32-l10n-ntly-0000000\build\comm-central\mozilla\python\mozbuild\mozbuild\configure\__init__.py", line 362, in run
    self._value_for(option)
  File "c:\builds\moz2_slave\tb-c-cen-w32-l10n-ntly-0000000\build\comm-central\mozilla\python\mozbuild\mozbuild\configure\__init__.py", line 430, in _value_for
    return self._value_for_option(obj)
  File "c:\builds\moz2_slave\tb-c-cen-w32-l10n-ntly-0000000\build\comm-central\mozilla\python\mozbuild\mozbuild\util.py", line 925, in method_call
    cache[args] = self.func(instance, *args)
  File "c:\builds\moz2_slave\tb-c-cen-w32-l10n-ntly-0000000\build\comm-central\mozilla\python\mozbuild\mozbuild\configure\__init__.py", line 495, in _value_for_option
    % option_string.split('=', 1)[0])
mozbuild.configure.options.InvalidOptionError: RUSTC is not available in this configuration
*** Fix above errors and then restart with\
               "c:/builds/moz2_slave/tb-c-cen-w32-l10n-ntly-0000000/build/comm-central/mozmake.exe -f client.mk build"
c:/builds/moz2_slave/tb-c-cen-w32-l10n-ntly-0000000/build/comm-central/client.mk:360: recipe for target 'configure' failed
mozmake.exe[1]: Leaving directory 'c:/builds/moz2_slave/tb-c-cen-w32-l10n-ntly-0000000/build/comm-central'
client.mk:217: recipe for target 'configure' failed
mozmake.exe[1]: *** [configure] Error 1
mozmake.exe: *** [configure] Error 2
program finished with exit code 2

Maybe Rust is part of the compile environment which we've just disabled here:
https://hg.mozilla.org/comm-central/rev/0d9f3ed365b7912b6e57fcd21ad11de536f3384c#l3.9
in bug 1346020 to get repacks to work for TB 52 ESR. Actually, we reverted this change for Linux later but the Windows part seemed to have been a success for TB 52 ESR.
Status: RESOLVED → REOPENED
Flags: needinfo?(philipp)
Flags: needinfo?(ewong)
Resolution: FIXED → ---
So IIUC this is because the RUSTC variable is set in build/mozconfig.rust, which is included through mozconfig.common. I guess we could juggle around with disable-compile-environment, but if we need that option to fix other things I guess we could also unset RUSTC at the end of the l10n mozconfig, similar to how the artifact builds do it at https://dxr.mozilla.org/comm-central/source/mozilla/build/mozconfig.artifact. 

Another way would be to not include mozconfig.common and only add the most important options. This is what the source builder does: https://dxr.mozilla.org/comm-central/source/mail/config/mozconfigs/linux64/source

I would expect that RUSTC and other such variables are ignored with disable-compile-environment instead of it bailing. I don't know if it would be reasonable to change the parser behavior there. Can someone remind me why disable-compile-environment was required in the first place? It doesn't seem like it is really well supported.
Flags: needinfo?(philipp)
(In reply to Philipp Kewisch [:Fallen] from comment #47)
> Can someone remind me why disable-compile-environment was required in the first place?
You've added it here, to fix bug 1346020, no?
https://hg.mozilla.org/comm-central/rev/0d9f3ed365b7912b6e57fcd21ad11de536f3384c#l3.9
Or do you mean, someone should explain the meaning and envisaged usage of the option? Who would know? I did a bit of research here: Bug 1346020 comment #9, but obviously I was wrong since we re-added it for Linux:
https://hg.mozilla.org/comm-central/rev/0fc015a842c72b05cdd2354335eb486c14e477fd
(sorry, just ignore me if the comment isn't helpful, that's all I know.)
I think the rationale for --disable-compile-environment was that we are repacking
the packages and not really building anything; however, it seems as if the
configure step now needs the compile environment in order for it to be ok.
Flags: needinfo?(ewong)
I think it's the source step that can be done without the compile env.
The others require the compile environment.
Attachment #8851465 - Flags: review?(philipp)
Status: REOPENED → ASSIGNED
Comment on attachment 8851465 [details] [diff] [review]
[mozconfigs] remove --disable-compile-environment. (proposed patch)

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

::: mail/config/mozconfigs/win32/l10n-mozconfig
@@ -2,5 @@
>  
>  ac_add_options --enable-application=mail
>  ac_add_options --enable-update-channel=${MOZ_UPDATE_CHANNEL}
>  ac_add_options --with-l10n-base=../../l10n
> -ac_add_options --disable-compile-environment

I'm not sure why you're removing this since it was added to fix bug 1346020:
https://hg.mozilla.org/releases/comm-esr52/rev/a730476809a84b349be34db4b59013288ebc2803#l3.9
at least for ESR52. Or was that to overzealously retrofitted to C-C and C-A and C-B?
https://hg.mozilla.org/comm-central/rev/0d9f3ed365b7912b6e57fcd21ad11de536f3384c#l3.9
(In reply to Jorg K (GMT+1) from comment #51)
> Comment on attachment 8851465 [details] [diff] [review]
> [mozconfigs] remove --disable-compile-environment. (proposed patch)
> 
> Review of attachment 8851465 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/config/mozconfigs/win32/l10n-mozconfig
> @@ -2,5 @@
> >  
> >  ac_add_options --enable-application=mail
> >  ac_add_options --enable-update-channel=${MOZ_UPDATE_CHANNEL}
> >  ac_add_options --with-l10n-base=../../l10n
> > -ac_add_options --disable-compile-environment
> 
> I'm not sure why you're removing this since it was added to fix bug 1346020:
> https://hg.mozilla.org/releases/comm-esr52/rev/
> a730476809a84b349be34db4b59013288ebc2803#l3.9
> at least for ESR52. Or was that to overzealously retrofitted to C-C and C-A
> and C-B?
> https://hg.mozilla.org/comm-central/rev/
> 0d9f3ed365b7912b6e57fcd21ad11de536f3384c#l3.9

iirc, it was done for the sake of consistency and not because it was
really necessary, since I'm kinda curious as to why Linux required
the comp-env whereas Win32 and OSX64 didn't. 

I'm not seeing why Linux has to be special and not the other platforms.
It's either by design, or there's a bug somewhere(aside for this one).
(In reply to Edmund Wong (:ewong) from comment #52)
> iirc, it was done for the sake of consistency ...
I think so.

Quoting bug 1346020 comment #7:
> I created the patch with a comm-esr52 repo, but we should probably push
> this to all repos once we have confirmed it works for 52.
Note that for Linux that was already undone in bug 1349300.

So do I understand you correctly that we want to let ESR52 as it is now, but undo the "consistency" pushes to C-C, C-A and C-B? Or only C-C and C-A?
Sadly we haven't heard anything from the build peer here. There also doesn't seem to be a good understanding of the |--disable-compile-environment| option since this was added/removed on various occasions in bug 1349300, bug 1311045 and bug 1346020.

What is planned to be removed here got here due to bug 1346020 comment #7:
> I created the patch with a comm-esr52 repo, but we should probably push
> this to all repos once we have confirmed it works for 52.
In fact, it "probably" shouldn't have been pushed to all repos, so let's undo this.

We're in the untenable situation were we have reduced test coverage on C-C on Windows. There is no harm done is continuing the "trial and error game" just a little further.

While there doesn't appear to be a need for removing |--disable-compile-environment| for Mac, we can try to see whether that has negative effects.

I will land this modified patch in the next 24 hours. I prefer to keep the comments for reference.
Attachment #8851465 - Attachment is obsolete: true
Attachment #8851465 - Flags: review?(philipp)
Attachment #8853746 - Flags: review+
https://hg.mozilla.org/comm-central/rev/a55eb9828cbb9bd35599d47344d625edcc990d1f

OK, so after the Daily run tomorrow I'll check that l10n for Mac still works and that l10n for Windows has started working again, fingers crossed ;-)

NI'ing myself, so I won't forget.

Looking at http://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-aurora-l10n/, Mac is working but Linux stopped on 23rd March 2017, sigh. Related to bug 1349300 which landed on 21st March 2017? Strange, then it should already have failed on 22nd, but that day worked and from 23rd there is nothing for Linux.
Flags: needinfo?(jorgk)
(In reply to Jorg K (GMT+2) from comment #53)
> 
> So do I understand you correctly that we want to let ESR52 as it is now, but
> undo the "consistency" pushes to C-C, C-A and C-B? Or only C-C and C-A?

I think, for the sake of 'testing',  just undo c-c's consistency change.   Sorry for
the delay in response, as I'm having some issues with 2.48b1..
Part of the "consistency change" was already undone for Linux in bug 1349300, but adding a comment. Now you/I/we've don't the same for Windows, also with a comment, but we added more "consistency" for Mac, as you had suggested.

I'll check the success or failure later today. Perhaps we should start a little survey ;-)

--disable-compile-environment needed:

     |Linux |Mac    |Windows
-----+----------------------
C-C  |no    |yes/no?|no?
C-A  |no![1]|yes/no?|no?
C-B  |      |       |
ESR52|no![2]|yes    |yes![3]

with    ! means: We tried the other way and it failed.
without ! means: Known working but haven't tested otherwise

[1] Removed --disable-compile-environment, on 21st March 2017
https://hg.mozilla.org/releases/comm-aurora/rev/e808efc45600aef8ee7484399e57f6bc0cd8c15e
Accidentally backed out on 22nd March 2017:
https://hg.mozilla.org/releases/comm-aurora/rev/001ce687b7cf16c00723bbc88e0849c2033ab833
and it started failing.

[2] Bug 1349300 and bug 1311045.
[3] Bug 1346020
Flags: needinfo?(jorgk)
OK, let's see:

Yesterday, 2017-04-02
http://ftp.mozilla.org/pub/thunderbird/nightly/2017/04/2017-04-02-03-02-02-comm-central-l10n/
Linux: working.
Mac: working.
Windows: broken.

Today, 2017-04-03:
http://ftp.mozilla.org/pub/thunderbird/nightly/2017/04/2017-04-03-03-02-05-comm-central-l10n/:
Linux: no change, hence still working.
Mac: now broken, so removing --disable-compile-environment needed broke it.
Windows: still broken:
  IOError: [Errno 2] No such file or directory: 'c:\\builds\\relengapi.tok'
  However, that might not mean anything, since we saw this in bug 1352456 and it magically fixed itself.

So in conclusion we have:

--disable-compile-environment needed:

     |Linux |Mac    |Windows
-----+----------------------
C-C  |no!   |yes!   |no?
C-A  |no!   |yes!   |no?
C-B  |      |       |
ESR52|no!   |yes!   |yes![3]

[3] Bug 1346020 - And looking at that bug again I think we have no evidence that the "yes!"
    has any empirical evidence. Maybe Windows ESR52 works with both.
Now we know that Mac needs the switch, Linux and Windows don't.
Attachment #8853746 - Attachment is obsolete: true
Attachment #8853957 - Flags: review+
(In reply to Onno Ekker [:nONoNonO UTC+1] from comment #44)
> can this be uplifted to aurora?
This was referring to:
M-C and C-C:
https://hg.mozilla.org/mozilla-central/rev/2e22ffe74d41
https://hg.mozilla.org/comm-central/rev/20d90b5d3da4b80622a7f180c8b6e1dabac286d4
That doesn't need uplift.

Today
http://ftp.mozilla.org/pub/thunderbird/nightly/2017/04/2017-04-03-00-40-03-comm-aurora-l10n/
both Linux and Mac Aurora l10n worked, and with some luck, with uplift from comment 61, Windows should work, too, which I can check tomorrow.
Let's close this bug now since it already covers two issues: The original issue with the SFX package and then the removal of the erroneously added |--disable-compile-environment| from bug 1346020.

Summarising landings:
SFX issue:
https://hg.mozilla.org/mozilla-central/rev/2e22ffe74d41
https://hg.mozilla.org/comm-central/rev/20d90b5d3da4b80622a7f180c8b6e1dabac286d4

--disable-compile-environment issue:
C-C (TB 55): https://hg.mozilla.org/comm-central/rev/754fbc931bbbe681a1a41074a1a1c6fa9b011c85
C-A (TB 54): https://hg.mozilla.org/releases/comm-aurora/rev/65a133709b61bc766418476b762d55b017d1e52b
C-B (TB 53): https://hg.mozilla.org/releases/comm-beta/rev/37c4656a7a2334c27654a6380456c9a7e50dae11

I've triggered another Daily build on C-C just now, so if the |IOError: [Errno 2] No such file or directory: 'c:\\builds\\relengapi.tok'| error persists, I'll open another bug.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
(In reply to Jorg K (GMT+2) from comment #63)
> I've triggered another Daily build on C-C just now, ...

Result of Daily build:
Linux: still working.
Mac: working again.
Windows: Not working, filed bug 1353046.
To see what works, I also triggered a C-A Daily:
http://ftp.mozilla.org/pub/thunderbird/nightly/2017/04/2017-04-03-09-21-55-comm-aurora-l10n/
Linux: working again, fixed today (broke 22nd March 2017, bug 1349300 comment #11).
Mac: Still working.
Windows: Not working, same error as reported in bug 1353046.

(In reply to Jorg K (GMT+2) from comment #62)
> This was referring to:
> M-C and C-C:
> https://hg.mozilla.org/mozilla-central/rev/2e22ffe74d41
> https://hg.mozilla.org/comm-central/rev/
> 20d90b5d3da4b80622a7f180c8b6e1dabac286d4
> That doesn't need uplift.
Or maybe is does.
Comment on attachment 8847882 [details] [diff] [review]
[m-c] proposed patch (v3)

Approval Request Comment
[Feature/Bug causing the regression]: bug 1287881 
[User impact if declined]: No l10n repacks for Thunderbird on Aurora
[Is this code covered by automated tests?]: N/A
[Has the fix been verified in Nightly?]: N/A
[Needs manual test from QE? If yes, steps to reproduce]: N/A
[List of other uplifts needed for the feature/fix]: none, only this patch.
[Is the change risky?]: No, works for weeks on M-C, build config only.
[Why is the change risky/not risky?]: See above.
[String changes made/needed]: None.

This needs uplift to Aurora since "firefox" is still hard-coded here:
https://hg.mozilla.org/releases/mozilla-aurora/file/tip/python/mozbuild/mozbuild/action/7z_exe_archive.py#l16
Attachment #8847882 - Flags: approval-mozilla-aurora?
This bug has become so confusing, this is what needs to be uplifted:
https://hg.mozilla.org/mozilla-central/rev/2e22ffe74d41
RelMan has been in a work week this week, which might be slowing things down.
Flags: needinfo?(ryanvm)
Comment on attachment 8847882 [details] [diff] [review]
[m-c] proposed patch (v3)

Fix l10n repacks issue. Aurora54+.
Flags: needinfo?(gchang)
Attachment #8847882 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1360525
You need to log in before you can comment on or make changes to this bug.