-
Notifications
You must be signed in to change notification settings - Fork 176
Initial MSI implementation, based on Briefcase #1084
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
We require contributors to sign our Contributor License Agreement and we don't have one on file for @mhsmith. In order for us to review and merge your code, please e-sign the Contributor License Agreement PDF. We then need to manually verify your signature, merge the PR (conda/infrastructure#1222), and ping the bot to refresh the PR. |
|
@conda-bot check |
constructor/briefcase.py
Outdated
| msi_paths = list(dist_dir.glob("*.msi")) | ||
| if len(msi_paths) != 1: | ||
| raise RuntimeError(f"Found {len(msi_paths)} MSI files in {dist_dir}") | ||
| shutil.copy(msi_paths[0], info["_outpath"]) |
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.
Should this be a move command instead? In heavy installers and reduced disk storage availability, it might be the difference between success and error.
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.
Done.
| "enum": [ | ||
| "all", | ||
| "exe", | ||
| "msi", |
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 change should be applied in _schema.py and then propagated.
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.
Done.
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 should also have an integration test in test_examples.py once the dependencies are ready.
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 think most integration tests have installer_type: all, which will break if features are missing. I suggest replacing them with installer_type: {{ 'exe' if sys.platform == 'win32' else 'all' for those that fail because of missing features. That will also give us a good idea about when we have feature parity.
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.
@mhsmith just for completeness, I've added (and working on finalizing it) the integration tests in test_examples.py in this branch: https://github.com/lrandersson/constructor/tree/briefcase
I branched from the branch that's in this PR.
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.
Testing instructionsCheck out the following repositories:
Run these commands: The MSI will be generated in the working directory. |
Remaining features required for production use[Moved to #967] |
marcoesters
left a comment
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.
My biggest change request is to get the dependencies right. Specifically, briefcase is missing from the list. I'm also a little concerned about how tightly this is bound to Python conventions.
constructor/briefcase.py
Outdated
| name = info["name"] | ||
| if not name: | ||
| raise ValueError("Name is empty") |
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.
| name = info["name"] | |
| if not name: | |
| raise ValueError("Name is empty") | |
| if not (name := info.get("name")): | |
| raise ValueError("Name is empty") |
If we are concerned about empty values, we should use get, too.
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 fixed it in mhsmith#1
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've copied that fix to this PR.
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 think most integration tests have installer_type: all, which will break if features are missing. I suggest replacing them with installer_type: {{ 'exe' if sys.platform == 'win32' else 'all' for those that fail because of missing features. That will also give us a good idea about when we have feature parity.
| if not name: | ||
| raise ValueError("Name is empty") | ||
|
|
||
| # Briefcase requires version numbers to be in the canonical Python format, and some |
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.
Python is not fully compatible with SemVer, so that could be a pretty significant limitation.
It will at least require a few version changes in our integration test examples:
| version: X |
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.
Fixed temporarily in mhsmith#1
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.
As far as MSI is concerned, the version number is only used for 2 purposes:
-
Display in the installed apps list. This is just for the user's information, so I think it's acceptable if some of the construct.yaml
versiongets moved into thenamefor display purposes, as long as all the information is still present. -
Blocking the installer of an old version if a new version is already installed. Since constructor itself doesn't define any version ordering rules, it's impossible to do this perfectly for all version schemes. The current code covers a reasonably large number of cases, but it could be extended if necessary.
Notice the current code only uses the last valid Python package version number it finds. This is to accommodate the Miniconda construct.yaml file, which sets version to something like py313_25.1.2-3. It's better to transform this into 25.1.2.3 rather than 313.25.1.2.3.
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.
For versions with no numbers in them at all, we could either reject them as the PR currently does, or display a warning and fall back to a default like 0.0.1 or 1.0.0. That's probably a good idea, since it would allow all existing construct.yaml files to at least build, and the integration tests wouldn't need to change so much.
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've made it use a default of 0.0.1, so that if a valid version is added in the future, it'll be treated as an upgrade.
| strip_chars = " .-_" | ||
| before = info["version"][:start].strip(strip_chars) | ||
| after = info["version"][end:].strip(strip_chars) | ||
| name = " ".join(s for s in [name, before, after] if 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.
So, for something like Miniconda3-py310_25.9.1-3-Windows-x86_64.msi, we have the following?
name = Miniconda3 py310
version = 25.9.1.3
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, those would be the name and version from Briefcase's point of view, and that's how they'd be displayed in the Windows apps list.
The current code can generate these values from a construct.yaml file where py310 is part of the version. There's an example of that in test_name_version in test_briefcase.py.
constructor/briefcase.py
Outdated
| # different versions of a product. | ||
| def get_bundle_app_name(info, name): | ||
| # If reverse_domain_identifier is provided, use it as-is, but verify that the last | ||
| # component is a valid Python package name, as Briefcase requires. |
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.
Since constructor doesn't require Python-based packages, will we run into issues with non-Python namespaces?
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.
As with the version, I guess we could make a best effort to transform it into something valid. In the case of MSI, this value is only used internally by Briefcase and is never visible to the user. So it doesn't matter if it's slightly different to the construct.yaml value, as long as it's consistent.
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.
Done.
| briefcase = Path(sysconfig.get_path("scripts")) / "briefcase.exe" | ||
| logger.info("Building installer") | ||
| run( | ||
| [briefcase, "package"] + (["-v"] if verbose else []), | ||
| cwd=tmp_dir, | ||
| check=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.
I have two questions here:
- Is it possible to use
briefcaseas an API rather than a CLI? - This looks like Windows - do we need any guards here?
Either way, we should make sure that briefcase.exe exists before calling it.
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 added 2) in mhsmith#1, the check for briefcase.exe is also 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.
- No, the CLI is the only stable interface.
- The check in mhsmith#1 looks good to me. The fact that we're running on WIndows is already guarded by
os_allowedin main.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.
I've copied the briefcase.exe check to this PR.
| @@ -0,0 +1,9 @@ | |||
| _conda constructor --prefix . --extract-conda-pkgs | |||
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.
Let's use absolute paths instead of relative paths.
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.
Done.
pyproject.toml
Outdated
| "jinja2", | ||
| "jsonschema >=4" | ||
| "jsonschema >=4", | ||
| "tomli-w >=1.2.0", |
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.
Since this is only needed for MSI installers, this should probably only be added for Windows - same in the recipe.
This is also missing briefcase as a dependency.
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.
In mhsmith#1
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've updated pyproject.toml, meta.yaml and environment.yml. A new enough version of Briefcase is now on conda-forge, so it no longer needs to be installed manually.
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.
Briefcase 0.3.26 also has a couple of new MSI features:
- Interactive selection of the install directory.
- Uninstaller options – though as it says here, these currently only appear when you choose "Modify" from the app list rather than "Uninstall", so we'll need to revisit this in the future.
|
To keep all the discussion in one place, I've merged mhsmith#1 into this PR, and sent an invite to @lrandersson to give him access to to push to the branch directly. |
|
On second thoughts, this PR is getting big enough already, and I'm probably not the best person to review the subsequent PRs. So I'll un-merge mhsmith#1 from this PR, and I suggest we proceed as follows: |
dev/environment.yml
Outdated
| - briefcase >=0.3.26 # [win] | ||
| - tomli-w >=1.2.0 # [win] |
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.
Environment YAML files do not recognize these selectors as far as I know. the pillow line doesn't make sense. I'd rather remove those Windows specific dependencies here.
Those dependencies should go into extra-requirements-windows.txt
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, I've moved the 2 new requirements to extra-requirements-windows.txt. The pillow line was already there, so I'll leave it alone to avoid conflict with any fix on the main branch.
…irements-windows.txt
* Initial prototype as shown in demo * Switch to install_launcher option * Update schema properly * Move MSI file rather than copying it * Add fallbacks for invalid versions and app names * Use absolute paths in install script * Check that briefcase.exe exists * Add briefcase to dependencies, and make it and tomli-w Windows-only * Move Windows-specific dependencies from environment.yml to extra-requirements-windows.txt --------- Co-authored-by: Marco Esters <mesters@anaconda.com>
Description
Initial work towards:
Requires Briefcase to be installed in the environment, including the following updates:
As discussed, this PR is sufficient to build a simple MSI installer (with no install-time options) from the miniconda3 recipe. After the Briefcase updates are merged, I'll post a comment here with instructions on how to do that.
@freakboy3742: FYI
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?