-
-
Notifications
You must be signed in to change notification settings - Fork 389
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Allow to pack a Windows app as zip file #1183
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! The broad strokes of this look good; I've left some comments inline about some of the specifics.
@@ -30,9 +31,27 @@ def package_command(tmp_path): | |||
return command | |||
|
|||
|
|||
@pytest.fixture | |||
def package_zip_command(tmp_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason a second fixture is needed here? Why not add the files as part of the base package_command
fixture (or create a package_command_with_files
fixture that extends the base fixture, rather than duplicating the definition)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you're right. Regarding creating a new fixture that extends an existing one I failed. Fixtures cannot be called directly and I didn't find the way to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
@@ -260,6 +260,16 @@ def package_app( | |||
:param timestamp_digest: Hashing algorithm to request from the timestamp server. | |||
""" | |||
|
|||
# Just pack the 'binary' folders in a zip file: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be refactored. Zip packaging shouldn't be a "shortcut path" on the generic packaging command - zip and msi are two different packaging formats, which should have 2 clearly different implementations (self._package_zip()
and self._package_msi()
).
It's also problematic that this implementation comes before app signing (L273-286). Although signing won't be as significant for zip-packaged apps, it's still something we can and should do if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding app signing: Should the final zip file also be signed (if signing is requested), like the msi file?
tests/platforms/conftest.py
Outdated
@@ -18,6 +18,7 @@ def first_app_config(): | |||
sources=["src/first_app"], | |||
requires=["foo==1.2.3", "bar>=4.5"], | |||
test_requires=["pytest"], | |||
packaging_format="", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't smell right to me. ""
isn't a valid packaging format anywhere; if this is needed to make a specific test pass, it should be added as a local test modification (first_app_config.packaging_format = 'whatever'
); and if a lot of tests need the same annotation, defined as a local fixture override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a local conftest.py
file in the platform/windows folder.
if app.packaging_format == "zip": | ||
self.logger.info("Building zip file...", prefix=app.app_name) | ||
self.tools.shutil.make_archive( | ||
self.distribution_path(app).with_suffix(""), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The distribution path should be returning the final filename. This means the distribution path should be incorporating the .zip
suffix. This manifests in the completion message - if you package with -p zip
, it tells you that you've just packaged MyApp-1.2.3.msi
.
This also builds a zip file that doesn't have a root folder. That means it will dump a lot of files in the same directory as the zip file. I'd suggest that there should be a "MyApp-1.2.3" folder in the zip file - it's easy to remove if you don't need it, but it's difficult to clean up if we don't provide it.
You're probably also looking for pathlib.stem
, rather than pathlib.with_suffix("")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about a root folder, wasn't sure if this is wanted. The path issue origins from the fact that the distribution path as defined earlier in the code does already contain the .msi
suffix.
After writing the shutil command from scratch I finally realized that zip packaging is also used in the Web backend, so I took it from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The web backend also does zip packaging, but the use case there explicitly doesn't need the root folder. You unpack a web project into the folder where you want it to be served.
As for the issue with distribution_path
already containing msi
- that's my point - it shouldn't contain msi
if you're packaging a zip file. At the very least, it's returning the wrong extension. distribution_path
should be adapting to the selected packaging format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I now define distribution_path
depending on the package_format
.
For the moment, I see two options to make a root folder:
- creating a folder in the build folder (e.g. on the same level as src) and copy the content of the src folder into it (this I have implemented now). In this case, should we delete the root folder afterward or just keep it?
- temporarily rename the build/.../scr folder to
MyApp-1.2.3
. This seems a little bit 'hackish' to me.
What would you recommend? Unfortunately, neither ZipFile
nor shutil.make_archive
have a 'put in a root folder before zipping' option...
archive_file = tmp_path / "base_path" / "dist" / "First App-0.0.1.zip" | ||
assert archive_file.exists() | ||
with ZipFile(archive_file) as archive: | ||
assert archive.namelist() == ["First App.exe"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets you coverage, but won't differentiate between "the compressed EXE file is the only thing in the zip" and "We actually included all the files you need for the exe to work.". I'd suggest extending the fixture to include a couple of representative files in app
and app_packages
so we can verify the full package tree is being archived.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not only give you coverage, it will also show you that creating a zip folder with the correct name does work.
My first guess was that within the testing environment I find some real hello world project that is used for building, running, packaging and which I can use. When I realized that e.g. the msi packager tests does not actually create an MSI installer, I wondered how important testing the zip content is considered crucial here. Of course, I can put the correct folder structure in the zip and add some more mock files, still, this wouldn't show that the packed app will run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, for sure - any time you mock anything you lose the ability to verify actual behavior. What I'm aiming at here is trying to avoid a predictable mode of failure - or, more generally, how could the implementation change in a way that would pass this test, but fail in practice.
In this case, it would be easy to modify the implementation so that it only compressed the executable, and this test would still pass. Since we know that an app can't run without an app and app_packages folder, ensuring that the archived product has included those in the archive will protect against that mode of failure.
changes/457.feature.rst
Outdated
@@ -0,0 +1 @@ | |||
On Windows, it is now possible to build a zip folder for distribution by using ``package -p zip``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Windows, it is now possible to build a zip folder for distribution by using ``package -p zip``. | |
Support for packaging Windows apps as simple Zip files was added. |
@@ -14,6 +14,9 @@ enabled. To ensure .NET Framework 3.5 is enabled: | |||
3. Select "Turn Windows features On or Off" | |||
4. Ensure that ".NET framework 3.5 (includes .NET 2.0 and 3.0)" is selected. | |||
|
|||
Instead of building an MSI installer, you can also pack the folders for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be incorporated into the earlier paragraph introducing WiX; something like:
Briefcase supports 2 packaging formats for a Windows app:
1. As an MSI installer
2. As a Zipfile containing all files needed to run the app
then lead into the description of how WiX is used to build MSIs.
A similar comment/update is needed on the VisualStudio format as well.
Dear @freakboy3742 , Thank you very much for your detailed review and I'm sorry for confronting you with a PR with so much issues. One of the problems I had was that I had a running implementation on briefcase 0.3.12 (where app signing and distribution folders didn't exist) but then I was pushed back with each new release of briefcase since they had some significant changes for Windows packaging. Most of the time for this PR I spend trying to understand the testing environment (and obviously I did not well), for two days I believed that my implementation is not longer working because the tests failed until I realized that the implementation was OK and the issues were 'only' the tests... :-) |
Not a problem at all - and although there's a lot of comments, I don't consider the issues to be that bad - it's lots of little things that are, in many cases, wrapped up in details that you probably don't know about unless you've been soaking in the code for years like I have. This is a really good first-time project contribution - the fact that you've submitted a PR that fully passes CI on your first attempt (even if there's room for improving those tests) puts you at the top of the class in terms of contributions :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting really close - a couple more comments inline.
@@ -7,7 +7,8 @@ The default output format for Windows is an :doc:`app <./app>`. | |||
Briefcase also supports creating a :doc:`Visual Studio project <./visualstudio>` | |||
which in turn can be used to build an app. | |||
|
|||
Both output formats support packaging as an MSI. | |||
Both output formats support packaging as a Microsoft software installer (MSI) or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both output formats support packaging as a Microsoft software installer (MSI) or | |
Both output formats support packaging as a Microsoft Software Installer (MSI) or |
app=app, | ||
filepath=self.distribution_path(app), | ||
**sign_options, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will attempt to sign the .zip file, which won't work because .zip files don't allow for signing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I asked this in the conversation above, but then I didn't wait for your answer. Sorry. Google search results have let me believe that this is possible. Seems that it is not possible with Windows' SDK SignTool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like the only outstanding item.
self.bundle_path(app) / self.packaging_root, | ||
zip_root_path, | ||
dirs_exist_ok=True, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, but it (a) results in a copy of the entire project, which might not be small, and (b) could be prone to stale files on the second build.
At the very least, the copied folder should be a one-use temporary folder, or deleted after use; however a better approach may be to not use shutil
, and use zip file directly - that gives you much better control over the structure of the final zip file. For example, see this StackOverflow answer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have considered some options. The 'problem' here is that we want to put the files in a root folder that does not already exist before zipping. This is different to the situation described in the StackOverflow question/answer (that I also found). My impression was that neither shutil.make_archive
nor ZipFile
allows the creation/building of a root folder 'on the fly' while packing, so I was thinking about either to temporarily rename the src
folder to MyApp-1.2.3
(what I found 'hackish') or to make a plain copy and delete it afterwards (what I'm doing now).
OK, after reading this StackOverflow answer, I see that it is possible.
1200977
to
f280921
Compare
862f458
to
b92bf73
Compare
Possibly, there was an unrelated error during the Python 3.9 test on macOs (CI / Unit test). |
hmm... |
source = self.bundle_path(app) / self.packaging_root # /src | ||
try: | ||
os.mkdir(self.dist_path) # /dist | ||
except FileExistsError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this except
is missing coverage.
@freakboy3742 @rmartin16: I saw that testing creates several temporary folders, for each test one (?). So when I want to test a condition that a folder was already created (like here), I need to test this within the same test_...()
function, otherwise I end up in a fresh test folder, is this correct?
Or I create the dist
folder in the fixture from the beginning, so no need to run the _package_zip()
two times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that testing creates several temporary folders, for each test one (?).
Yes; Pytest provides the tmp_path
fixture for each test such that it's a unique empty directory that's ultimately cleaned up by Pytest.
So when I want to test a condition that a folder was already created (like here), I need to test this within the same
test_...()
function, otherwise I end up in a fresh test folder, is this correct?Or I create the
dist
folder in the fixture from the beginning, so no need to run the_package_zip()
two times.
I'd probably need to see a specific test example...but moreover, you shouldn't need to create the dist_path
directory here; it is already created by the base PackageCommand
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I got again caught in a situation where I thought that something is not working while the problem was indeed a different one. However, I'm quite confident that the dist
folder was not created, and the resulting zip file was laying flat next to the build
folder.
Using Path.mkdir(exist_ok=True)
would also save me the try
clause...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mkdir call definitely isn't needed. It will be created by the base package command. That doesn't guarantee it exists for your test - so you may need to create the dist folder as part a fixture or other test setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more cleanups - but we're getting really close!
pass | ||
|
||
with ZipFile(self.distribution_path(app), "w", ZIP_DEFLATED) as archive: | ||
for path, _, files in os.walk(source): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally prefer the pathlib
form of these commands to the os
form - so source.glob("*/**")
in this case. Similar updates are needed for the other os
calls in this block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, did so.
source = self.bundle_path(app) / self.packaging_root # /src | ||
try: | ||
os.mkdir(self.dist_path) # /dist | ||
except FileExistsError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mkdir call definitely isn't needed. It will be created by the base package command. That doesn't guarantee it exists for your test - so you may need to create the dist folder as part a fixture or other test setup.
@@ -27,12 +28,50 @@ def package_command(tmp_path): | |||
version="81.2.1.0", | |||
arch="groovy", | |||
) | |||
command.tmp_path = tmp_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't needed - tmp_path
is guaranteed to be consistent within a given test. You don't need to annotated it onto the command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, got it. These pytest fixtures are still enigmatic to me.
"app/first-app/app.py", | ||
"app/first-app/resources/__init__.py", | ||
"app_packages/clr.py", | ||
"app_packages/toga_winforms/command.py", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this evidently works on Windows, it would be preferable to use Pathlib objects and make the path separator unambiguous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to let it stand as it is here; this list is also used to check the content of the ZIP file, and the namelist of the ZIP file consists of strings in this format.
However, the entries will be converted to Pathlib objects when creating the file with create_file()
, as you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's not what this part of the test is for. It's creating fixtures. It's essential that they are created with the correct, platform appropriate path separators. The test fixture should not also be providing the test validation data - the test should be explicitly specifying what it expects, not relying on the input from a fixture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is especially important when what is being validated isn't in the same format as what is being put into the system in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, changed it.
for path in paths: | ||
(src_path / path).mkdir(parents=True) | ||
for file in package_command.files: | ||
open(f"{src_path}/{file}", "x").close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've got a create_file()
utility method in tests/utils
for this purpose - it creates a file with known content, and ensures the directory exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used it now.
Again, the unrelated issue with |
c110454
to
abcd175
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is almost ready to land - the only outstanding issue I can see is the "package signing" issue. Indent that block so it only applies to MSI packaging, and we should be good.
It would also be desirable to have a test of that case, confirming that when a ZIP package is signed, only the app binary is signed. It doesn't need to actually do the signing; mocking
sign_app` and confirming the calls made should be sufficient.
app=app, | ||
filepath=self.distribution_path(app), | ||
**sign_options, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like the only outstanding item.
@freakboy3742 Just to be sure that I got it right: At the moment, when a zip has been packaged, it will not go on for signing, because I put a |
My apologies - I completely missed the return in my scan of the code. As a general rule, we prefer to avoid using |
If I may, a small nitpick while we're here: the |
OK, CI / Unit tests on macOS (for now: 3.8 and 3.9) throw a number of DeprecationWarnings. This is not the well known |
Oh wow - you're having the most remarkable luck hitting CI problems that aren't your fault at all :-) This one is because there's been a recent update to setuptools that is causing dmgbuild, the tool that we use to build macOS DMG installers, to raise a warning that pytest is considering a failure. I know how to fix it, but I'll need to push out a new dmgbuild release and then update briefcase to use that new version. |
...and the hits keep coming. I've fixed the dmgbuild issue; that's revealed an issue with the Window winstore CI environment. |
dd27da0
to
b14b233
Compare
It's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome - Thanks for the contribution! The issue with the failing macOS test is unrelated to this patch, and CI is passing, so I'm happy to merge this, and deal with the intermittent test failure independently.
Great! Thank you for your guidance with this PR. |
I'm not overly concerned that the specific files in the test case include As for whether there's other stuff that can be removed - I honestly don't know. I haven't done a full audit; the exes are the obvious ones, but there might be some additional files that aren't needed. In terms of how to learn about it... I'm not sure there's much by way of documentation. The best approach will be to try and work out what role each file in the archive actually serves, and build up a list of candidates for elimination. |
As discussed in issue #457 and elsewhere on the discord channel, a Windows app packed as zip file could be a nice alternative for shipping and will sometimes be preferable to an MSI installer.
I also got the impression that @freakboy3742 wouldn't block such a PR per se, so I'll give it a try ;-)
Fixes #457
PR Checklist: