Skip to content
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

Bump JDK 8u242-b08 to 17.0.7+7 #1271

Merged
merged 15 commits into from Jun 5, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changes/1065.feature.rst
@@ -0,0 +1 @@
The version of OpenJDK for Java was updated from 8 to 17.
115 changes: 55 additions & 60 deletions src/briefcase/commands/base.py
@@ -1,3 +1,5 @@
from __future__ import annotations

import argparse
import importlib
import inspect
Expand All @@ -9,7 +11,6 @@
from abc import ABC, abstractmethod
from argparse import RawDescriptionHelpFormatter
from pathlib import Path
from typing import Optional

from cookiecutter import exceptions as cookiecutter_exceptions
from cookiecutter.repository import is_repo_url
Expand All @@ -26,7 +27,7 @@
import tomli as tomllib

from briefcase import __version__
from briefcase.config import AppConfig, BaseConfig, GlobalConfig, parse_config
from briefcase.config import AppConfig, GlobalConfig, parse_config
from briefcase.console import Console, Log
from briefcase.exceptions import (
BriefcaseCommandError,
Expand Down Expand Up @@ -108,6 +109,9 @@ class BaseCommand(ABC):
GLOBAL_CONFIG_CLASS = GlobalConfig
APP_CONFIG_CLASS = AppConfig
allows_passthrough = False
# if specified for a platform, then any template for that
# platform must declare compatibility with that version or later.
oldest_compatible_briefcase: str | None = None

def __init__(
self,
Expand Down Expand Up @@ -149,7 +153,7 @@ def __init__(
Download.verify(tools=self.tools)

self.global_config = None
self._path_index = {}
self._briefcase_toml: dict[AppConfig, dict[str, ...]] = {}

@property
def logger(self):
Expand Down Expand Up @@ -324,104 +328,95 @@ def binary_path(self, app):

:param app: The app config
"""
...

def _load_path_index(self, app: BaseConfig):
"""Load the path index from the index file provided by the app template.
def briefcase_toml(self, app: AppConfig) -> dict[str, ...]:
"""Load the ``briefcase.toml`` file provided by the app template.

:param app: The config object for the app
:return: The contents of the application path index.
:return: The contents of ``briefcase.toml``
"""
try:
with (self.bundle_path(app) / "briefcase.toml").open("rb") as f:
self._path_index[app] = tomllib.load(f)["paths"]
except OSError as e:
raise BriefcaseCommandError(
f"Unable to find '{self.bundle_path(app) / 'briefcase.toml'}'"
) from e
return self._path_index[app]
return self._briefcase_toml[app]
except KeyError:
try:
with (self.bundle_path(app) / "briefcase.toml").open("rb") as f:
self._briefcase_toml[app] = tomllib.load(f)
except OSError as e:
raise BriefcaseCommandError(
f"Unable to find '{self.bundle_path(app) / 'briefcase.toml'}'"
) from e
else:
return self._briefcase_toml[app]

def support_path(self, app: BaseConfig):
"""Obtain the path into which the support package should be unpacked.
def path_index(self, app: AppConfig, path_name: str) -> str | dict | list:
"""Return a path from the path index provided by the app template.

Raises KeyError if ``path_name`` is not defined in the index.

:param app: The config object for the app
:return: The full path where the support package should be unpacked.
:param path_name: Name of the filepath to retrieve
:return: filepath for requested path
"""
# If the index file hasn't been loaded for this app, load it.
return self.briefcase_toml(app=app)["paths"][path_name]

def briefcase_target_version(self, app: AppConfig) -> str | None:
"""The list of requirements to build an app from ``briefcase.toml``."""
try:
path_index = self._path_index[app]
return self.briefcase_toml(app)["briefcase"]["target_version"]
except KeyError:
path_index = self._load_path_index(app)
return self.bundle_path(app) / path_index["support_path"]
return None

def support_path(self, app: AppConfig) -> Path:
"""Obtain the path into which the support package should be unpacked.

def support_revision(self, app: BaseConfig):
:param app: The config object for the app
:return: The full path where the support package should be unpacked.
"""
return self.bundle_path(app) / self.path_index(app, "support_path")

def support_revision(self, app: AppConfig) -> str:
"""Obtain the support package revision that the template requires.

:param app: The config object for the app
:return: The support revision required by the template.
"""
# If the index file hasn't been loaded for this app, load it.
try:
path_index = self._path_index[app]
except KeyError:
path_index = self._load_path_index(app)
return path_index["support_revision"]
return self.path_index(app, "support_revision")

def cleanup_paths(self, app: BaseConfig):
def cleanup_paths(self, app: AppConfig) -> list[str]:
"""Obtain the paths generated by the app template that should be cleaned up
prior to release.

:param app: The config object for the app
:return: The list of path globs inside the app template that should
be cleaned up.
"""
# If the index file hasn't been loaded for this app, load it.
try:
path_index = self._path_index[app]
except KeyError:
path_index = self._load_path_index(app)
return path_index["cleanup_paths"]
return self.path_index(app, "cleanup_paths")

def app_requirements_path(self, app: BaseConfig):
def app_requirements_path(self, app: AppConfig) -> Path:
"""Obtain the path into which a requirements.txt file should be written.

:param app: The config object for the app
:return: The full path where the requirements.txt file should be written
"""
# If the index file hasn't been loaded for this app, load it.
try:
path_index = self._path_index[app]
except KeyError:
path_index = self._load_path_index(app)
return self.bundle_path(app) / path_index["app_requirements_path"]
return self.bundle_path(app) / self.path_index(app, "app_requirements_path")

def app_packages_path(self, app: BaseConfig):
def app_packages_path(self, app: AppConfig) -> Path:
"""Obtain the path into which requirements should be installed.

:param app: The config object for the app
:return: The full path where application requirements should be installed.
"""
# If the index file hasn't been loaded for this app, load it.
try:
path_index = self._path_index[app]
except KeyError:
path_index = self._load_path_index(app)
return self.bundle_path(app) / path_index["app_packages_path"]
return self.bundle_path(app) / self.path_index(app, "app_packages_path")

def app_path(self, app: BaseConfig):
def app_path(self, app: AppConfig) -> Path:
"""Obtain the path into which the application should be installed.

:param app: The config object for the app
:return: The full path where application code should be installed.
"""
# If the index file hasn't been loaded for this app, load it.
try:
path_index = self._path_index[app]
except KeyError:
path_index = self._load_path_index(app)
return self.bundle_path(app) / path_index["app_path"]
return self.bundle_path(app) / self.path_index(app, "app_path")

def app_module_path(self, app):
def app_module_path(self, app: AppConfig) -> Path:
"""Find the path for the application module for an app.

:param app: The config object for the app
Expand Down Expand Up @@ -484,7 +479,7 @@ def verify_tools(self):
"""
pass

def finalize_app_config(self, app: BaseConfig):
def finalize_app_config(self, app: AppConfig):
"""Finalize the application config.

Some app configurations (notably, Linux system packages like .deb) have
Expand All @@ -502,7 +497,7 @@ def finalize_app_config(self, app: BaseConfig):
"""
pass

def finalize(self, app: Optional[BaseConfig] = None):
def finalize(self, app: AppConfig | None = None):
"""Finalize Briefcase configuration.

This will:
Expand All @@ -529,7 +524,7 @@ def finalize(self, app: Optional[BaseConfig] = None):
self.finalize_app_config(app)
delattr(app, "__draft__")

def verify_app_tools(self, app: BaseConfig):
def verify_app_tools(self, app: AppConfig):
"""Verify that tools needed to run the command for this app exist."""
pass

Expand Down
60 changes: 57 additions & 3 deletions src/briefcase/commands/build.py
@@ -1,6 +1,9 @@
from typing import Optional

from briefcase.config import BaseConfig
from packaging.version import Version

import briefcase
from briefcase.config import AppConfig, BaseConfig
from briefcase.exceptions import BriefcaseCommandError

from .base import BaseCommand, full_options
Expand All @@ -14,16 +17,66 @@ def add_options(self, parser):
self._add_update_options(parser, context_label=" before building")
self._add_test_options(parser, context_label="Build")

def build_app(self, app: BaseConfig, **options):
def build_app(self, app: AppConfig, **options):
"""Build an application.

:param app: The application to build
"""
# Default implementation; nothing to build.

def verify_template(self, app: AppConfig):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be in the base command, and called as part of a more generic check (finalize?). If you've got an app that was generated for 0.3.14, and you try to run it without building, you're going to have the same class of problem - it's not exclusively a build problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah...I initially tried to put it there but ran in to issue. I'll try to move it up again.

"""Verify the template satisfies the Command's requirements."""
self.logger.info("Checking template compatability...", prefix=app.app_name)

with self.input.wait_bar("Verifying Briefcase support..."):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure a waitbar is needed here. Verification should be near immediate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this wait bar here is mostly intended to provide better continuity with the rest of the output.

While I consider the Wait Bar to be mostly about communicating a task is ongoing, I think the consistency of [task description]... done helps users understand what exactly is taking place in a consistent format. There's several small steps like this we're already wrapping with the Wait Bar elsewhere.

I suppose the end state of the Wait bar output could be manually printed...but that feels a little awkward.

Still want to remove/change this?

Example output:

[helloworld] Generating application template...
Using app template: /home/russell/github/beeware/briefcase-android-gradle-template, branch v0.3.15

[helloworld] Installing support package...
No support package required.

[helloworld] Installing application code...
Installing src/helloworld... done

[helloworld] Installing requirements...
Writing requirements file... done

[helloworld] Installing application resources...
Unable to find src/helloworld/resources/helloworld-round-48.png for 48px round application icon; using default
Unable to find src/helloworld/resources/helloworld-round-72.png for 72px round application icon; using default
Unable to find src/helloworld/resources/helloworld-round-96.png for 96px round application icon; using default
Unable to find src/helloworld/resources/helloworld-round-144.png for 144px round application icon; using default
Unable to find src/helloworld/resources/helloworld-round-192.png for 192px round application icon; using default
Unable to find src/helloworld/resources/helloworld-square-48.png for 48px square application icon; using default
Unable to find src/helloworld/resources/helloworld-square-72.png for 72px square application icon; using default
Unable to find src/helloworld/resources/helloworld-square-96.png for 96px square application icon; using default
Unable to find src/helloworld/resources/helloworld-square-144.png for 144px square application icon; using default
Unable to find src/helloworld/resources/helloworld-square-192.png for 192px square application icon; using default

[helloworld] Removing unneeded app content...
Removing directory app/src/main/python/helloworld/__pycache__
Removing unneeded app bundle content... done

[helloworld] Created build/helloworld/android/gradle

[helloworld] Checking template compatibility...
Verifying Briefcase support... done

[helloworld] Updating app metadata...
Setting main module... done

[helloworld] Building Android APK...
Starting a Gradle Daemon, 1 incompatible Daemon could not be reused, use --status for details

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you're saying; I guess for me, "is this a valid template" is a fast validation check that should almost always pass. It's not "doing" anything - it's just verifying that everything else will work. 99% of the time, it won't be communicating anything - and the 1% of the time that it does, there will be a big "it won't work" error message.

Some of the other "short lived" tasks (e.g., updating app metadata) are actually doing something - they're part of the build process, writing/modifiying files, etc. I can't think of another task that is (a) short lived and (b) doesn't write/add/delete anything to the disk, with the exception of no-op tasks that are flagged has doing something, but then the platform backend doesn't need to actually do anything.

The closest I can think of is some of the Docker checks - but those (a) can take time, and (b) can fail in some creative ways, so flagging that you're in the middle of a Docker check can be useful debugging.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok...so, do you want this to print anything then? I feel like just printing the section header [helloworld] Checking template compatibility... with nothing else is also awkward.

That makes sense if so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry - I thought that part was implied. My intention was to say that there should be no message when verifying the template version, unless there's a problem. Apologies for the ambiguity on my part.

# check against the "release" for each Version since versions
# such as "0.3.15.dev123" are considered older than "0.3.15"

if template_target := self.briefcase_target_version(app=app):
template_target = Version(template_target).release

if platform_target := self.oldest_compatible_briefcase:
platform_target = Version(platform_target).release

if platform_target and not template_target:
raise BriefcaseCommandError(
f"""\
The app template must declare a target version of Briefcase to confirm it is
compatible with this version of Briefcase.

The template's target Briefcase version must be {'.'.join(map(str, platform_target))} or later.

Once the template is updated, run the create command:

$ briefcase create {self.platform} {self.output_format}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't verify this renders 100% correctly, but suggested text:

Suggested change
The app template must declare a target version of Briefcase to confirm it is
compatible with this version of Briefcase.
The template's target Briefcase version must be {'.'.join(map(str, platform_target))} or later.
Once the template is updated, run the create command:
$ briefcase create {self.platform} {self.output_format}
Briefcase requires that the app template explicitly declare that it is compatible with
Briefcase {'.'.join(map(str, platform_target))} or later. However, the generated app's
`briefcase.toml` has no `target_version` declaration.
If the app was generated with an earlier version of Briefcase using the default Briefcase
template, you can run:
$ briefcase create {self.platform} {self.output_format}
to re-generate your app.
If you are using a custom template, you'll need to update the template to correct any
compatibility problems, and then add the compatibility declaration.

"""
)

current_version = Version(briefcase.__version__).release
if (platform_target and platform_target > template_target) or (
template_target and template_target > current_version
):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an elif platform_target != template_target. There's no need for the actual Briefcase version to be involved, and it's not a > comparison - we're looking for a "Briefcase version as epoch" marker. For this change to Java, the template_target will be 0.3.15, and it will stay that way until we make another backwards incompatible change to the Android template format.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already discussed this here and here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has definitely been a sticking point...i guess i won't say too much more except these approaches are effectively the same...with slightly different implementations.

I like the idea of an epoch....but I think using the Briefcase version is confusing....because people will think you're literally talking about the current version of Briefcase.

I also like the idea of supporting a version inside the template that's independent of the platform version support declaration in Briefcase. This allows third party templates to specify an arbitrary level of support with Briefcase. If we limit this to Briefcase's internal platform version, then we're limiting the number of potential breaking changes third party can protect themselves from....which could be missed breaking changes or even just nuances among versions.

At this point, I'm starting to feel we may just need an executive decision....if only because I'm a bit exhausted going round and round. I'm prepared to implement the epoch versioning or even something else. (I'm still working on promoting the check to all Commands since that's pretty impactful to the tests.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case, I'm going to put on my BDFN hat and say "epoch-style numbering, using the Briefcase version as the epoch marker".

Yes, there's a source of confusion in that a 0.3.16 template will have marker saying it is for 0.3.15. However, as this discussion has revealed, any discussion of compatibility and version numbers is going to be confusing; picking an identifier that has a clear interpretation (vs a new "epoch" number that will need it's own explanation) is about as good as we're going to get, IMHO.

It's also not especially user-visible - it's a value in briefcase.toml, which isn't something that run-of-the-mill users will ever need to see. This is a "confusing detail" that will mostly affect Briefcase maintainers, not the outside world.

Using the 0.3.15 version is also implicit documentation of which release notes you need to look at for a migration guide.

I acknowledge that there could be a need for a third-party template provider to lock against a different Briefcase version. However, we don't have any particular evidence that this requirement exists - it's mostly speculation and trying to foretell the future. On that basis, I'm comfortable living with the limitation until such time as we can clearly identify a specific use-case. If such a requirement were to develop, I can imagine adding an additional template option (or command line option) to ignore template version checks, or impose a different version check; however, I think we should cross that bridge when (and if) we come to it.

minimum_version = ".".join(map(str, platform_target or current_version))
raise BriefcaseCommandError(
f"""\
The app template is not compatible with this version of Briefcase since it is
targeting version {'.'.join(map(str, template_target))}.

If you are using BeeWare's default template, then running the create command
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this grammar needs to be tweaked...i'll do it later if we're good with the rest of this.

Suggested change
If you are using BeeWare's default template, then running the create command
If you are using BeeWare's default template, then run the create command

to update the app using a compatible version of the app template.

If a custom template is being used, it must be updated to be compatible with
Briefcase version {minimum_version} before re-running the create command.

To run the create command:

$ briefcase create {self.platform} {self.output_format}
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The app template is not compatible with this version of Briefcase since it is
targeting version {'.'.join(map(str, template_target))}.
If you are using BeeWare's default template, then running the create command
to update the app using a compatible version of the app template.
If a custom template is being used, it must be updated to be compatible with
Briefcase version {minimum_version} before re-running the create command.
To run the create command:
$ briefcase create {self.platform} {self.output_format}
"""
The app templated used to generate this app is not compatible with this version of Briefcase.
Briefcase requires a template that is compatible with version {platform_target}; the template
used to generate this app is compatible with version {template_target}.
If the app was generated with an earlier version of Briefcase using the default Briefcase
template, you can run:
$ briefcase create {self.platform} {self.output_format}
to re-generate your app.
If you are using a custom template, you'll need to update the template to correct any
compatibility problems, and then add the compatibility declaration.

)

def _build_app(
self,
app: BaseConfig,
app: AppConfig,
update: bool,
update_requirements: bool,
update_resources: bool,
Expand Down Expand Up @@ -68,6 +121,7 @@ def _build_app(
else:
state = None

self.verify_template(app)
self.verify_app_tools(app)

state = self.build_app(app, test_mode=test_mode, **full_options(state, options))
Expand Down