-
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
Changes from 1 commit
c6c4134
f719293
201a3ed
a12eb59
52b3d34
0101c62
8e79869
93ce7aa
2a99674
2cd9d19
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,150 @@ | ||||
| """ | ||||
| Logic to build installers using Briefcase. | ||||
| """ | ||||
|
|
||||
| import logging | ||||
| import re | ||||
| import shutil | ||||
| import sysconfig | ||||
| import tempfile | ||||
| from pathlib import Path | ||||
| from subprocess import run | ||||
|
|
||||
| import tomli_w | ||||
|
|
||||
| from . import preconda | ||||
| from .utils import DEFAULT_REVERSE_DOMAIN_ID, copy_conda_exe, filename_dist | ||||
|
|
||||
| BRIEFCASE_DIR = Path(__file__).parent / "briefcase" | ||||
| EXTERNAL_PACKAGE_PATH = "external" | ||||
|
|
||||
| logger = logging.getLogger(__name__) | ||||
|
|
||||
|
|
||||
| def get_name_version(info): | ||||
| name = info["name"] | ||||
| if not name: | ||||
| raise ValueError("Name is empty") | ||||
|
|
||||
| # Briefcase requires version numbers to be in the canonical Python format, and some | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed temporarily in mhsmith#1
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||
| # installer types use the version to distinguish between upgrades, downgrades and | ||||
| # reinstalls. So try to produce a consistent ordering by extracting the last valid | ||||
| # version from the Constructor version string. | ||||
| # | ||||
| # Hyphens aren't allowed in this format, but for compatibility with Miniconda's | ||||
| # version format, we treat them as dots. | ||||
| matches = list( | ||||
| re.finditer( | ||||
| r"(\d+!)?\d+(\.\d+)*((a|b|rc)\d+)?(\.post\d+)?(\.dev\d+)?", | ||||
| info["version"].lower().replace("-", "."), | ||||
| ) | ||||
| ) | ||||
| if not matches: | ||||
| raise ValueError( | ||||
| f"Version {info['version']!r} contains no valid version numbers: see " | ||||
| f"https://packaging.python.org/en/latest/specifications/version-specifiers/" | ||||
| ) | ||||
| match = matches[-1] | ||||
| version = match.group() | ||||
|
|
||||
| # Treat anything else in the version string as part of the name. | ||||
| start, end = match.span() | ||||
| 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) | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, for something like
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||
|
|
||||
| return name, version | ||||
|
|
||||
|
|
||||
| # Some installer types use the reverse domain ID to detect when the product is already | ||||
| # installed, so it should be both unique between different products, and stable between | ||||
| # 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. | ||||
|
||||
| if (rdi := info.get("reverse_domain_identifier")) is not None: | ||||
| if "." not in rdi: | ||||
| raise ValueError(f"reverse_domain_identifier {rdi!r} contains no dots") | ||||
| bundle, app_name = rdi.rsplit(".", 1) | ||||
|
|
||||
| if not re.fullmatch( | ||||
| r"[A-Z0-9]|[A-Z0-9][A-Z0-9._-]*[A-Z0-9]", app_name, flags=re.IGNORECASE | ||||
| ): | ||||
| raise ValueError( | ||||
| f"reverse_domain_identifier {rdi!r} doesn't end with a valid package " | ||||
| f"name: see " | ||||
| f"https://packaging.python.org/en/latest/specifications/name-normalization/" | ||||
| ) | ||||
|
|
||||
| # If reverse_domain_identifier isn't provided, generate it from the name. | ||||
| else: | ||||
| bundle = DEFAULT_REVERSE_DOMAIN_ID | ||||
| app_name = re.sub(r"[^a-z0-9]+", "-", name.lower()).strip("-") | ||||
| if not app_name: | ||||
| raise ValueError(f"Name {name!r} contains no alphanumeric characters") | ||||
|
|
||||
| return bundle, app_name | ||||
|
|
||||
|
|
||||
| # Create a Briefcase configuration file. Using a full TOML writer rather than a Jinja | ||||
| # template allows us to avoid escaping strings everywhere. | ||||
| def write_pyproject_toml(tmp_dir, info): | ||||
| name, version = get_name_version(info) | ||||
| bundle, app_name = get_bundle_app_name(info, name) | ||||
|
|
||||
| config = { | ||||
| "project_name": name, | ||||
| "bundle": bundle, | ||||
| "version": version, | ||||
| "license": ({"file": info["license_file"]} if "license_file" in info else {"text": ""}), | ||||
| "app": { | ||||
| app_name: { | ||||
| "formal_name": f"{info['name']} {info['version']}", | ||||
| "description": "", # Required, but not used in the installer. | ||||
| "external_package_path": EXTERNAL_PACKAGE_PATH, | ||||
| "external_package_executable_path": "", | ||||
| "use_full_install_path": False, | ||||
| "post_install_script": str(BRIEFCASE_DIR / "run_installation.bat"), | ||||
| } | ||||
| }, | ||||
| } | ||||
|
|
||||
| if "company" in info: | ||||
| config["author"] = info["company"] | ||||
|
|
||||
| (tmp_dir / "pyproject.toml").write_text(tomli_w.dumps({"tool": {"briefcase": config}})) | ||||
|
|
||||
|
|
||||
| def create(info, verbose=False): | ||||
| tmp_dir = Path(tempfile.mkdtemp()) | ||||
| write_pyproject_toml(tmp_dir, info) | ||||
|
|
||||
| external_dir = tmp_dir / EXTERNAL_PACKAGE_PATH | ||||
| external_dir.mkdir() | ||||
| preconda.write_files(info, external_dir) | ||||
| preconda.copy_extra_files(info.get("extra_files", []), external_dir) | ||||
|
|
||||
| download_dir = Path(info["_download_dir"]) | ||||
| pkgs_dir = external_dir / "pkgs" | ||||
| for dist in info["_dists"]: | ||||
| shutil.copy(download_dir / filename_dist(dist), pkgs_dir) | ||||
|
|
||||
| copy_conda_exe(external_dir, "_conda.exe", info["_conda_exe"]) | ||||
|
|
||||
| briefcase = Path(sysconfig.get_path("scripts")) / "briefcase.exe" | ||||
| logger.info("Building installer") | ||||
| run( | ||||
| [briefcase, "package"] + (["-v"] if verbose else []), | ||||
| cwd=tmp_dir, | ||||
| check=True, | ||||
| ) | ||||
|
Comment on lines
148
to
159
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have two questions here:
Either way, we should make sure that
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added 2) in mhsmith#1, the check for
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've copied the |
||||
|
|
||||
| dist_dir = tmp_dir / "dist" | ||||
| 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"]) | ||||
|
||||
|
|
||||
| if not info.get("_debug"): | ||||
| shutil.rmtree(tmp_dir) | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| _conda constructor --prefix . --extract-conda-pkgs | ||
|
||
|
|
||
| set CONDA_PROTECT_FROZEN_ENVS=0 | ||
| set CONDA_ROOT_PREFIX=%cd% | ||
| set CONDA_SAFETY_CHECKS=disabled | ||
| set CONDA_EXTRA_SAFETY_CHECKS=no | ||
| set CONDA_PKGS_DIRS=%cd%\pkgs | ||
|
|
||
| _conda install --offline --file conda-meta\initial-state.explicit.txt -yp . | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -224,6 +224,7 @@ | |
| "enum": [ | ||
| "all", | ||
| "exe", | ||
| "msi", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change should be applied in
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| "pkg", | ||
| "sh" | ||
| ], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,3 +10,4 @@ dependencies: | |
| - jinja2 | ||
| - jsonschema >=4 | ||
| - pydantic 2.11.* | ||
| - tomli-w >=1.2.0 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,8 @@ dependencies = [ | |
| "ruamel.yaml >=0.11.14,<0.19", | ||
| "pillow >=3.1 ; platform_system=='Windows' or platform_system=='Darwin'", | ||
| "jinja2", | ||
| "jsonschema >=4" | ||
| "jsonschema >=4", | ||
| "tomli-w >=1.2.0", | ||
|
||
| ] | ||
|
|
||
| [project.optional-dependencies] | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also have an integration test in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think most integration tests have
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| import pytest | ||
|
|
||
| from constructor.briefcase import get_bundle_app_name, get_name_version | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "name_in, version_in, name_expected, version_expected", | ||
| [ | ||
| # Valid versions | ||
| ("Miniconda", "1", "Miniconda", "1"), | ||
| ("Miniconda", "1.2", "Miniconda", "1.2"), | ||
| ("Miniconda", "1.2.3", "Miniconda", "1.2.3"), | ||
| ("Miniconda", "1.2a1", "Miniconda", "1.2a1"), | ||
| ("Miniconda", "1.2b2", "Miniconda", "1.2b2"), | ||
| ("Miniconda", "1.2rc3", "Miniconda", "1.2rc3"), | ||
| ("Miniconda", "1.2.post4", "Miniconda", "1.2.post4"), | ||
| ("Miniconda", "1.2.dev5", "Miniconda", "1.2.dev5"), | ||
| ("Miniconda", "1.2rc3.post4.dev5", "Miniconda", "1.2rc3.post4.dev5"), | ||
| # Hyphens are treated as dots | ||
| ("Miniconda", "1.2-3", "Miniconda", "1.2.3"), | ||
| ("Miniconda", "1.2-3.4-5.6", "Miniconda", "1.2.3.4.5.6"), | ||
| # Additional text before and after the last valid version should be treated as | ||
| # part of the name. | ||
| ("Miniconda", "1.2 3.4 5.6", "Miniconda 1.2 3.4", "5.6"), | ||
| ("Miniconda", "1.2_3.4_5.6", "Miniconda 1.2_3.4", "5.6"), | ||
| ("Miniconda", "1.2c3", "Miniconda 1.2c", "3"), | ||
| ("Miniconda", "1.2rc3.dev5.post4", "Miniconda 1.2rc3.dev5.post", "4"), | ||
| ("Miniconda", "py313", "Miniconda py", "313"), | ||
| ("Miniconda", "py.313", "Miniconda py", "313"), | ||
| ("Miniconda", "py3.13", "Miniconda py", "3.13"), | ||
| ("Miniconda", "py313_1.2", "Miniconda py313", "1.2"), | ||
| ("Miniconda", "1.2 and more", "Miniconda and more", "1.2"), | ||
| ("Miniconda", "1.2! and more", "Miniconda ! and more", "1.2"), | ||
| ("Miniconda", "py313 1.2 and more", "Miniconda py313 and more", "1.2"), | ||
| # Numbers in the name are not added to the version. | ||
| ("Miniconda3", "1", "Miniconda3", "1"), | ||
| ], | ||
| ) | ||
| def test_name_version(name_in, version_in, name_expected, version_expected): | ||
| name_actual, version_actual = get_name_version( | ||
| {"name": name_in, "version": version_in}, | ||
| ) | ||
| assert (name_actual, version_actual) == (name_expected, version_expected) | ||
|
|
||
|
|
||
| def test_name_empty(): | ||
| with pytest.raises(ValueError, match="Name is empty"): | ||
| get_name_version({"name": ""}) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("version_in", ["", ".", "hello"]) | ||
| def test_version_invalid(version_in): | ||
| with pytest.raises( | ||
| ValueError, match=f"Version {version_in!r} contains no valid version numbers" | ||
| ): | ||
| get_name_version( | ||
| {"name": "Miniconda3", "version": version_in}, | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "rdi, name, bundle_expected, app_name_expected", | ||
| [ | ||
| ("org.conda", "ignored", "org", "conda"), | ||
| ("org.Conda", "ignored", "org", "Conda"), | ||
| ("org.conda-miniconda", "ignored", "org", "conda-miniconda"), | ||
| ("org.conda_miniconda", "ignored", "org", "conda_miniconda"), | ||
| ("org-conda.miniconda", "ignored", "org-conda", "miniconda"), | ||
| ("org.conda.miniconda", "ignored", "org.conda", "miniconda"), | ||
| (None, "x", "io.continuum", "x"), | ||
| (None, "X", "io.continuum", "x"), | ||
| (None, "Miniconda", "io.continuum", "miniconda"), | ||
| (None, "Miniconda3", "io.continuum", "miniconda3"), | ||
| (None, "Miniconda3 py313", "io.continuum", "miniconda3-py313"), | ||
| (None, "Hello, world!", "io.continuum", "hello-world"), | ||
| ], | ||
| ) | ||
| def test_bundle_app_name(rdi, name, bundle_expected, app_name_expected): | ||
| bundle_actual, app_name_actual = get_bundle_app_name({"reverse_domain_identifier": rdi}, name) | ||
| assert (bundle_actual, app_name_actual) == (bundle_expected, app_name_expected) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("rdi", ["", "org"]) | ||
| def test_rdi_no_dots(rdi): | ||
| with pytest.raises(ValueError, match=f"reverse_domain_identifier '{rdi}' contains no dots"): | ||
| get_bundle_app_name({"reverse_domain_identifier": rdi}, "ignored") | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("rdi", ["org.hello-", "org.-hello", "org.hello world", "org.hello!world"]) | ||
| def test_rdi_invalid_package(rdi): | ||
| with pytest.raises( | ||
| ValueError, | ||
| match=f"reverse_domain_identifier '{rdi}' doesn't end with a valid package name", | ||
| ): | ||
| get_bundle_app_name({"reverse_domain_identifier": rdi}, "ignored") | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("name", ["", " ", "!", "-", "---"]) | ||
| def test_name_no_alphanumeric(name): | ||
| with pytest.raises(ValueError, match=f"Name '{name}' contains no alphanumeric characters"): | ||
| get_bundle_app_name({}, name) |
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.
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.