Closed
Bug 1287881
Opened 8 years ago
Closed 8 years ago
add mach repackage so windows builds can be signed as separate step from build with move to taskcluster
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox50 affected, firefox54 fixed)
RESOLVED
FIXED
mozilla54
People
(Reporter: kmoir, Assigned: grenade)
References
Details
Attachments
(5 files, 2 obsolete files)
From bug 1277591 re creating a windows signing worker (aki's notes)
https://bugzilla.mozilla.org/show_bug.cgi?id=1277591#c4
To be explicit:
* `mach repackage`, or something like it, will be needed to do windows signing as a separate step from builds.
** unsigned windows packages need to be exploded, all the internal binaries signed separately, then repackaged, and the external package also signed.
* `mach repackage` is not written yet
* I'm not certain who is assigned the task of writing `mach repackage`. Its absence blocks this bug. In my mind, the actual windows signingscript + scriptworker implementations/deployment are relatively easy once linux signing scriptworker is deployed. The main difference is `mach repackage`.
Comment 1•8 years ago
|
||
Raw notes:
10:12 <ted> http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk is still the starting point
10:12 <ted> but it calls into https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.py to do a lot of the work nowadays
make-package -> make-package-internal
make-package-internal:
make-package-internal: prepare-package make-sourcestamp-file make-buildinfo-file make-mozinfo-file @echo 'Compressing...'
cd $(DIST) && $(MAKE_PACKAGE)
python/mozbuild/mozpack
(The above directory contains a packager directory, which contains an l10n.py, so mozbuild does seem to have l10n support. https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozpack/packager/l10n.py )
Comment 2•8 years ago
|
||
Related: bug 922241 and bug 576098.
I think the ideal workflow might be something like
windows build (installer, stub installer?)
|--> tests that test installer
|--> installer + stub installer signing, with repackage
in some order. It's easy to debate whether tests should test the nightly signed bits, or test unsigned bits, or test CI/test-signed bits and re-sign with the nightly key once tests pass, and then trigger more tests around the new signature. The key differences in the above model are a) signing happens outside the build process, b) we drop the zipfile and test the installers.
However, since switching all the tests to use the installers is a non-trivial task, and dropping the zipfile is a non-trivial social change with plenty of room for bikeshedding, I'm proposing we continue forward here with all 3 packages, and deal with dropping the zipfile as a non-blocking external task.
Reporter | ||
Comment 3•8 years ago
|
||
from #mozbuild
3:50 PM <aki> mshal: any 'mach repackage' progress or issues? we were talking about that in today's #tcmigration mtg, which unsurfaced https://bugzilla.mozilla.org/show_bug.cgi?id=922241 and https://bugzilla.mozilla.org/show_bug.cgi?id=576098 . i'm leaning towards dealing with those after our nightly tier 1 deadline
3:52 PM — aki adds comment to bug 1287881
4:36 PM <mshal> aki: gah, thanks for reminding me. So it turns out nobody in the build team really knows about the installer repackaging we do for l10n, so I have a little further digging to do. Too bad the easier package type is the one going away :)
4:37 PM <aki> :) ok, thanks for digging!
4:42 PM <aki> if we put off those two bugs, we'll likely need to sign the zipfile, but that should hopefully be relatively easy
4:43 PM <aki> easier than switching tests over to the installer and getting rid of the zipfile, at least :)
4:45 PM <mshal> yeah, signing the zip file looks super easy
Reporter | ||
Comment 4•8 years ago
|
||
Hi Mike
Coop asked me to scope out the work that would be required for mach repackage for q4 (after we get our nightly deliverables out of the way this quarter)
Is there an update on this issue. I note from one of the tcmigration meetings notes that you created a python script for signing, is this attached to a bug somewhere? From meeting notes:
"mach repackage (windows signing) - mshal came back with a small python script that uses 7zip directly on the exe. this isn't in-tree and isn't verified good compared to the standard packaging process."
Are there notes on how the make files need to be updated as well?
Flags: needinfo?(mshal)
Comment 5•8 years ago
|
||
I'm attaching the test script for reference, but I don't think we should be creating a new standalone script like this without de-duplicating some of the packaging logic. By this I mean I don't want to clean this script up, check it in, and only use it for 'mach repackage', since then we'll have 5 or so different ways of creating Windows installers.
Note that packager.py is really responsible for moving binaries & libraries into place, setting up omni.ja files, etc, but it doesn't know how to actually build a package. That logic is in the Makefiles, mostly as INNER_MAKE_PACKAGE (which is part of MAKE_PACKAGE). The rough workflow looks like this:
[source tree] -> 'mach build' -> [build products] -> 'packager.py' -> [staged package] -> '$(MAKE_PACKAGE) from the Makefile' -> package.zip
For getting signing in Taskcluster, my understanding is that you want to go from package.zip back to the staged package, sign things, and then create a new package.zip (and/or Windows installer, etc).
IMO we should continue moving the INNER_MAKE_PACKAGE / INNER_UNMAKE_PACKAGE variables from upload-files.mk into python files. Bug 1190522 is an example of this for the DMG package type (though it only did INNER_MAKE_PACKAGE - I think the UNMAKE should be in there as well so we can pack and unpack using just the python scripts). I'd guess a reasonable approach is to continue to add new <package-type>.py files in python/mozbuild/mozpack/
The Windows installer can be its own package type, which I think should be responsible for doing the 7zSD.sfx header and perhaps even setup.exe. Currently the logic for how to 7zip a Windows installer is repeated in a few places:
1) https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/upload-files.mk#141
2) https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/makensis.mk#54
3) https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/makensis.mk#73
4) https://github.com/mozilla-partners/repack-scripts/blob/master/repack-signed.sh#L16
This presents a problem when changing the zip parameters or package format - for example, bug 879467 tuned the compression parameters, but partner repacks still use the old parameters! So moving this logic into a win-installer.py, and using it from all of these locations means we'll have a single source of truth for unpacking and re-packing a Windows installer (and ideally all other package types as well).
With those python scripts in place, then I think it becomes pretty straightforward to add a hook in mach to do 'unpackage; resign; repackage' for any package type, including the installer.
Let me know if you have further questions, or if you want to chat about how to tackle this.
Flags: needinfo?(mshal)
Reporter | ||
Comment 6•8 years ago
|
||
mshal:
Thanks for all the information, very helpful!
Is this in a private repo, I can't see it
https://github.com/mozilla-partners/repack-scripts/blob/master/repack-signed.sh#L16
When you say
IMO we should continue moving the INNER_MAKE_PACKAGE / INNER_UNMAKE_PACKAGE variables from upload-files.mk into python files.
this refers to windows only, correct?
Flags: needinfo?(mshal)
Comment 7•8 years ago
|
||
(In reply to Kim Moir [:kmoir] from comment #6)
> Is this in a private repo, I can't see it
>
> https://github.com/mozilla-partners/repack-scripts/blob/master/repack-signed.
> sh#L16
Doh, apparently it is. I think I asked rail to get access before?
>
> When you say
>
> IMO we should continue moving the INNER_MAKE_PACKAGE / INNER_UNMAKE_PACKAGE
> variables from upload-files.mk into python files.
>
> this refers to windows only, correct?
I think we'll eventually want to move them all, but if the initial purpose of this bug is for the Windows installer only, then we should just do that to start with.
Flags: needinfo?(mshal)
Reporter | ||
Comment 8•8 years ago
|
||
coop, can I get access to https://github.com/mozilla-partners/repack-scripts/ ? for research for this bug?
Flags: needinfo?(coop)
Comment 9•8 years ago
|
||
(In reply to Kim Moir [:kmoir] from comment #8)
> coop, can I get access to
> https://github.com/mozilla-partners/repack-scripts/ ? for research for this
> bug?
I granted you read access. Please let me know if you need r/w.
Flags: needinfo?(coop)
Comment 10•8 years ago
|
||
10:25 <catlee> one thing to consider with repackage / signing, is that it's very important that mar files and installers end up with exactly the same binaries at the end
10:26 <catlee> signing introduces time-dependent data into the files
10:26 <catlee> so we need to share the binaries between installers and mars
10:27 <catlee> ideally signed binaries can be shared across locales as well where appropriate
Comment 11•8 years ago
|
||
Kim: any update on progress here?
See also bug 1318505 about Mac signing which may have some design implications here.
Flags: needinfo?(kmoir)
Reporter | ||
Comment 13•8 years ago
|
||
So I have been testing my patches on my loaner but have found that the python pefile library (new requirement) is not available in the virtualenv when I run mach package. My investigations lead me to think that this needs to be included in https://hg.mozilla.org/mozilla-build/ and mozillabuild needs to be rebuilt so I can test it. RyanVM - do you have any pointers on how to add a new python package to it? I notice you have made most of the recent commits to this repo.
I couldn't find much other than
https://wiki.mozilla.org/MozillaBuild
and the packaging scripts in the repo
Flags: needinfo?(ryanvm)
Comment 14•8 years ago
|
||
MozillaBuild is mostly dead (in that it's being deprecated by msys2 slowly but surely). It seems to me that the right path forward here would be to run |pip install pefile| from the MozillaBuild environment at some point during AMI generation.
Flags: needinfo?(ryanvm)
Reporter | ||
Comment 15•8 years ago
|
||
:grenade: Do you have any guidance on how to |pip install pefile| from the MozillaBuild environment at some point during AMI generation? I looked at the scripts for the b-2008 builders but didn't see any examples on adding python packages. And to be honest, I haven't created windows amis before.
Flags: needinfo?(rthijssen)
Assignee | ||
Comment 16•8 years ago
|
||
here's an example of how we do it for *buildbot* windows slave amis:
https://github.com/mozilla-releng/build-cloud-tools/blob/4049d878799218b489b78674f45869df71a8a81e/configs/Ec2UserdataUtils.psm1#L1465-L1469
you can pip install packages into *taskcluster* windows worker types during ami creation with a json block like this in the manifest for each worker type that needs it (maybe just gecko-3-b-win2012 if it's just for signing?):
https://github.com/mozilla-releng/OpenCloudConfig/blob/4ffb3e0a1af22accae6f42e42ba6bffcdda985fe/userdata/Manifest/gecko-3-b-win2012.json#L632-L663
for that, just submit a pr on the occ repo. once it's merged the amis will auto rebuild and it's available within an hour or two. however, that just installs the package on the system. i don't think it will be available to your venv.
if you're talking about installing a package into a venv during execution of a mach command within automation, you might not want the ami/system installation route. you might instead need something like this in-tree: https://dxr.mozilla.org/mozilla-central/rev/8387a4ada9a5c4cab059d8fafe0f8c933e83c149/testing/mozharness/scripts/fx_desktop_build.py#94
for this route, you'd also need to copy the package onto relengwebadmin (eg: `scp somepackage.zip root@relengwebadm:/mnt/netapp/relengweb/pypi/pub/`) so that it's available from http://pypi.pub.build.mozilla.org/pub/somepackage.zip since taskcluster windows builds will only install packages from the mozilla public pypi repo. it looks like you can grab the zip file for pefile here: https://pypi.python.org/pypi/pefile/
Flags: needinfo?(rthijssen)
Reporter | ||
Comment 17•8 years ago
|
||
So I added the pefile and it is loaded into the venv when running mach package. The problem now is that
pefile.py imports past.builtins always fails when running mach package
https://pypi.python.org/pypi/past/
I have tried many different combinations of adding this library so it is added to the venv but it doesn't work yet. Any suggestions?
Flags: needinfo?(mshal)
Reporter | ||
Comment 19•8 years ago
|
||
patches currently in progress (don't currently work but iterating through errors)
They
-added python future and pefile to mach_bootstrap.py
And virtualenv_packages.txt
Added libraries in tree
Added make_zip.py
This is called by toolkit/mozapps/installer/upload-files.mk instead of the zip commands directly
Make_zip.py calls python/mozbuild/mozpack/wininstaller.py which creates headers on the zips using pefile
Reporter | ||
Comment 20•8 years ago
|
||
Had a meeting with mshal today
mshal> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/makensis.mk#69
3:06 PM SFX_MODULE -> upx -> 7zSD.sfx
3:06 PM https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/upload-files.mk#144
3:06 PM 7zSD.sfx + package.7z -> package.exe
3:11 PM https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/makensis.mk#43
3:11 PM https://dxr.mozilla.org/mozilla-central/source/browser/installer/windows/Makefile.in#79
3:11 PM $(MAKE) setup.exe
He suggested that I shouldn't land the pefile libs in tree but rather leave use the existing 7zip utilities to unzip the file, extract the 7zip file, leave the old headers, resign then zip up to and create a new header. (This should be in my new python script that replaces the existing calls to the zip commands in upload-files.mk). He's going to investigate how if this can be used to replace existing calls to https://dxr.mozilla.org/mozilla-central/source/browser/installer/windows/Makefile.in#79 etc
Comment 21•8 years ago
|
||
So I still think we'll want two new python files alongside ./python/mozbuild/mozbuild/action/make_dmg.py, maybe make_windows_installer.py (or make_sfx7z.py?) and make_zip.py. The pefile package was used in a standalone script as a way to pull out the sfx header, since it was assumed we wouldn't have access to the one in the tree. However, since we're doing this in-tree, we can just use the existing header and avoid the pefile package. To create the installer, I think it would look something like this:
# input can be '-r' to mean recursively zip up the current directory, or a filename (like setup-stub.exe)
# output is the exe, so for example $(PACKAGE) or whatever
# tagfile is either app.tag or stub.tag, which are files found in the m-c repo
make_installer(input, tagfile, output):
upx --best -o [tmpdir]/7ZSD.sfx /path/to/srcdir/other-licenses/7zstub/firefox/7zSD.sfx
7z a -t7z [tmpdir]/app.7z [input] -mx -m0=BCJ2 -m1=LZMA:d25 -m2=LZMA:d19 -m3=LZMA:d19 -mb0:1 -mb0s1:2 -mb0s2:3
cat [tmpdir]/7ZSD.sfx [tagfile)] [tmpdir]/app.7z > [output.exe]
chmod 0775 [output.exe]
unmake_installer(installer):
7z x [installer] core
The equivalent for the zip format is much simpler. The make variables for this currently look like:
INNER_MAKE_PACKAGE = $(ZIP) -r9D $(PACKAGE) $(MOZ_PKG_DIR) \
-x \*/.mkdir.done
INNER_UNMAKE_PACKAGE = $(UNZIP) $(UNPACKAGE)
So just zip with some flags, and unzip.
The first step I think is to write & test the scripts against some packages and installers, which can be done without any Makefile changes. Once those are working, then we can go in and update the Makefiles referenced in #c5 to de-duplicate the upx/7z/cat/chmod logic that we have in a bunch of places. I can help with the Makefile updates - the packaging code is not very straightforward here :/
One thing I'm not sure of yet is why the installer has a directory called "core", while the .zip package has a directory called "firefox" - both contain the same contents, and we have some steps in the Makefiles to move from firefox to core when handling the installer. I'm not sure if we actually need that, and if we do, if that logic should be inside the make_installer() / unmake_installer() calls.
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → kmoir
Reporter | ||
Comment 22•8 years ago
|
||
We are at a tc migration work week in the Toronto lab this week. We discussed a new approach to packaging and signing the zip/dmg. I've attached a photo of the whiteboard.
Essentially the approach will be
3-4 task solution
Build creates unsigned tarball or zip, dmg or exe
Sign innards + signed zip and stub installer
Package task downloads signed innards
Sign package (windows only) signed exe and mar
There is also a list of tasks to complete in the trello board under the hard blockers heading
https://trello.com/b/zbZUr0hk/taskcluster-mac-windows-signing
Reporter | ||
Comment 23•8 years ago
|
||
wip patches because I'm switching focus to work on bug 1318505
Assignee | ||
Comment 24•8 years ago
|
||
i'm having some trouble getting the 7z archive command working. 7z is throwing an error which reads "The parameter is incorrect" but not sure which param it doesn't like. The error is from the try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cb3e163d69878353520dfd4dd50f4385c405c97&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=79044036:
14:03:07 INFO - (cd firefox && z:/build/build/src/obj-firefox/_virtualenv/Scripts/python.exe z:/build/build/src/config/createprecomplete.py) && z:/build/build/src/obj-firefox/_virtualenv/Scripts/python.exe -m mozbuild.action.archive_exe 'firefox' 'app.tag' 'firefox-54.0a1.x-test.win32.exe'
14:03:07 INFO - Ultimate Packer for eXecutables
14:03:07 INFO - Copyright (C) 1996 - 2013
14:03:07 INFO - UPX 3.91w Markus Oberhumer, Laszlo Molnar & John Reiser Sep 30th 2013
14:03:07 INFO - File size Ratio Format Name
14:03:07 INFO - -------------------- ------ ----------- -----------
14:03:07 INFO - 123904 -> 71680 57.85% win32/pe 7zSD.sfx
14:03:07 INFO - Packed 1 file.
14:03:07 INFO - 7-Zip [32] 15.14 : Copyright (c) 1999-2015 Igor Pavlov : 2015-12-31
14:03:07 INFO - Scanning the drive:
14:03:07 INFO - 11 folders, 107 files, 94148869 bytes (90 MiB)
14:03:07 INFO - Creating archive: c:\users\task_1487683920\appdata\local\temp\tmpz1m4im\app.7z
14:03:07 INFO - Items to compress: 118
14:03:07 INFO - System ERROR:
14:03:07 INFO - The parameter is incorrect.
14:03:07 INFO - {'tagfile': 'app.tag', 'contents': 'firefox', 'package': 'firefox-54.0a1.x-test.win32.exe'}
14:03:07 INFO - ['upx', '--best', '-o', 'c:\\users\\task_1487683920\\appdata\\local\\temp\\tmpz1m4im\\7zSD.sfx', '..\\..\\..\\other-licenses\\7zstub\\firefox\\7zSD.sfx']
14:03:07 INFO - ['7z', 'a', '-r', '-t7z', 'c:\\users\\task_1487683920\\appdata\\local\\temp\\tmpz1m4im\\app.7z', 'firefox', '-mx', '-m0=BCJ2', '-m1=LZMA:d25', '-m2=LZMA:d19', ' -m3=LZMA:d19', '-mb0:1', '-mbs1:2', '-mb0s2:3']
14:03:07 INFO - Traceback (most recent call last):
14:03:07 INFO - File "c:\mozilla-build\python\Lib\runpy.py", line 162, in _run_module_as_main
14:03:07 INFO - "__main__", fname, loader, pkg_name)
14:03:07 INFO - File "c:\mozilla-build\python\Lib\runpy.py", line 72, in _run_code
14:03:07 INFO - exec code in run_globals
14:03:07 INFO - File "z:\build\build\src\python\mozbuild\mozbuild\action\archive_exe.py", line 35, in <module>
14:03:07 INFO - sys.exit(main(sys.argv[1:]))
14:03:07 INFO - File "z:\build\build\src\python\mozbuild\mozbuild\action\archive_exe.py", line 31, in main
14:03:07 INFO - archive_exe(args[0], args[1], args[2])
14:03:07 INFO - File "z:\build\build\src\python\mozbuild\mozbuild\action\archive_exe.py", line 22, in archive_exe
14:03:07 INFO - subprocess.check_call(command)
14:03:07 INFO - File "c:\mozilla-build\python\Lib\subprocess.py", line 540, in check_call
14:03:07 INFO - raise CalledProcessError(retcode, cmd)
14:03:07 INFO - subprocess.CalledProcessError: Command '['7z', 'a', '-r', '-t7z', 'c:\\users\\task_1487683920\\appdata\\local\\temp\\tmpz1m4im\\app.7z', 'firefox', '-mx', '-m0=BCJ2', '-m1=LZMA:d25', '-m2=LZMA:d19', ' -m3=LZMA:d19', '-mb0:1', '-mbs1:2', '-mb0s2:3']' returned non-zero exit status 2
14:03:07 INFO - z:/build/build/src/toolkit/locales/l10n.mk:115: recipe for target 'repackage-zip' failed
14:03:07 INFO - mozmake.EXE[8]: *** [repackage-zip] Error 1
14:03:07 INFO - mozmake.EXE[8]: Leaving directory 'z:/build/build/src/obj-firefox/browser/locales'
14:03:07 INFO - Makefile:124: recipe for target 'repackage-win32-installer' failed
14:03:07 INFO - mozmake.EXE[7]: *** [repackage-win32-installer] Error 2
14:03:07 INFO - Makefile:137: recipe for target 'repackage-win32-installer-x-test' failed
14:03:07 INFO - mozmake.EXE[6]: *** [repackage-win32-installer-x-test] Error 2
Assignee | ||
Comment 25•8 years ago
|
||
Assignee | ||
Comment 26•8 years ago
|
||
Assignee | ||
Comment 27•8 years ago
|
||
Assignee | ||
Comment 28•8 years ago
|
||
Assignee | ||
Comment 29•8 years ago
|
||
Assignee | ||
Comment 30•8 years ago
|
||
Assignee | ||
Comment 31•8 years ago
|
||
Assignee | ||
Comment 32•8 years ago
|
||
Assignee | ||
Comment 33•8 years ago
|
||
Assignee | ||
Comment 34•8 years ago
|
||
Assignee | ||
Comment 35•8 years ago
|
||
Assignee | ||
Comment 36•8 years ago
|
||
Assignee | ||
Comment 37•8 years ago
|
||
Assignee | ||
Comment 38•8 years ago
|
||
Assignee | ||
Comment 39•8 years ago
|
||
Assignee | ||
Comment 40•8 years ago
|
||
after a bit of trial and error the 7zip archive command is working (see last few try pushes). newest problem seems to be with the cat command trying to parse the output redirect bracket as a filename ???
see: https://treeherder.mozilla.org/logviewer.html#?job_id=81116034&repo=try&lineNumber=1209670
:mshal: do you have any ideas about how i can get that last call to `cat` working?
Flags: needinfo?(mshal)
Comment 41•8 years ago
|
||
If you pass a list to `subprocess.Popen` it's going to pass that directly to CreateProcess or execv and bypass the shell. If you really want to use shell redirection like `cat x > y` you need to pass the whole thing as a string, which isn't great. It'd be better to just write Python code to do what those shell commands are doing. You can replace those calls to `mv` with `shutil.move`, and for the call to `cat`, which is just concatenating a few files you could do something like:
```
with open(package, 'wb') as o:
for i in [os.path.join(...), ...]:
shutil.copyfileobj(open(i, 'rb'), o)
```
Assignee | ||
Comment 42•8 years ago
|
||
Assignee | ||
Comment 43•8 years ago
|
||
Assignee | ||
Comment 44•8 years ago
|
||
cheers ted. i may have been running a little blinkered there... ;)
Flags: needinfo?(mshal)
Assignee | ||
Comment 45•8 years ago
|
||
Assignee | ||
Comment 46•8 years ago
|
||
Assignee | ||
Comment 47•8 years ago
|
||
hey kmoir, mshal, fwiw i've been hacking on the earlier patch here and cobbled this together. i don't know which tagfile should be used (app.tag or stub.tag) in the call from upload-files.mk to 7z_exe_archive. is this still useful or does the other mechanism of multiple tc tasks remove the need for it?
Attachment #8842943 -
Flags: feedback?(mshal)
Attachment #8842943 -
Flags: feedback?(kmoir)
Comment 48•8 years ago
|
||
Comment on attachment 8842943 [details] [diff] [review]
repackage
This is looking pretty nice!
I'm pretty sure that the INNER_MAKE_PACKAGE call for the SFX7Z format is only used when doing l10n repacks, which only operate on the full installer and not the stub installer. So app.tag is the correct thing to use there.
The reason we want to have the tag passed in as a variable is so we can do a similar $(call py_action,7z_exe_archive) call when creating the stub installer, and there we will pass in stub.tag:
https://dxr.mozilla.org/mozilla-central/rev/e91de6fb2b3dce9c932428265b0fdb630ea470d7/toolkit/mozapps/installer/windows/nsis/makensis.mk#55
I didn't do a full review yet, but I noticed the os.path.join calls everywhere. Do things break if you just do '../../../other-licenses/7zstub/firefox/7zSD.sfx' instead of the big os.path.join call? Also for the cases where you do need a join because of a variable, I'd recommend using our internal mozpath instead for consistency with the rest of our build system code (which uses forward slashes for everything). Eg:
import mozpack.path as mozpath
mozpath.join(tmpdir, '7zSD.sfx')
Attachment #8842943 -
Flags: feedback?(mshal) → feedback+
Assignee | ||
Comment 49•8 years ago
|
||
Comment hidden (mozreview-request) |
Reporter | ||
Comment 51•8 years ago
|
||
Comment on attachment 8842943 [details] [diff] [review]
repackage
I'll let mshal provide feedback so he is more familiar with this than I
Attachment #8842943 -
Flags: feedback?(kmoir)
Comment 52•8 years ago
|
||
mozreview-review |
Comment on attachment 8843273 [details]
bug 1287881 - windows repackage refactored as actions;
https://reviewboard.mozilla.org/r/117082/#review118904
This looks great - thanks for working on it! Just a few nits and then it should be ready to land.
::: python/mozbuild/mozbuild/action/7z_exe_extract.py:7
(Diff revision 1)
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +from __future__ import print_function
> +
> +import os
nit: os is unused
::: python/mozbuild/mozbuild/action/7z_exe_extract.py:11
(Diff revision 1)
> +
> +import os
> +import shutil
> +import sys
> +import subprocess
> +import tempfile
nit: tempfile is unused
::: toolkit/mozapps/installer/upload-files.mk:123
(Diff revision 1)
> - INNER_UNMAKE_PACKAGE = $(UNZIP) $(UNPACKAGE)
> endif
>
> ifeq ($(MOZ_PKG_FORMAT),SFX7Z)
> PKG_SUFFIX = .exe
> - INNER_MAKE_PACKAGE = rm -f app.7z && \
> + _ABS_MOZSRCDIR = $(shell cd $(MOZILLA_DIR) && pwd)
What is _ABS_MOZSRCDIR for? It looks like it might've been copied from the dmg case, but I don't think it does anything here.
Attachment #8843273 -
Flags: review?(mshal) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8842943 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8836808 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 54•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2825d7b622e7
windows repackage refactored as actions; r=mshal
Keywords: checkin-needed
Comment 55•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Reporter | ||
Comment 56•8 years ago
|
||
I didn't fix this - so assigning to Rob. Thanks Rob for driving this bug, so happy to see this resolved.
Assignee: kmoir → rthijssen
Updated•8 years ago
|
Component: mach → Build Config
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•