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

Override requirements (at a given level) from a requirements file. #1209

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions changes/476.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Support for pinned requirements files has been added
30 changes: 30 additions & 0 deletions docs/reference/configuration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,21 @@ application level, *and* platform level, the final set of requirements will be
the *concatenation* of requirements from all levels, starting from least to
most specific.

``requires_lock``
~~~~~~~~~~~~~~~~~

The name of a lock file.
If the file exists,
its contents will be used instead of the
value of the
``requires``
field.

Use
``briefcase update -r --relock``
to recompute the lock file.
Copy link
Member

Choose a reason for hiding this comment

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

This is... certainly a formatting choice. Is there a reason for the free verse style here? :-)

Copy link
Member

@mhsmith mhsmith May 11, 2023

Choose a reason for hiding this comment

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

[Conversation about general requirements file support moved to #1275]



``revision``
~~~~~~~~~~~~

Expand Down Expand Up @@ -415,6 +430,21 @@ level, application level, *and* platform level, the final set of requirements
will be the *concatenation* of requirements from all levels, starting from least
to most specific.


``test_requires_lock``
~~~~~~~~~~~~~~~~~~~~~~

The name of a lock file.
If the file exists,
its contents will be used instead of the
value of the
``test_requires``
field.

Use
``briefcase update -r --test --relock``
to update the lock file.

``test_sources``
~~~~~~~~~~~~~~~~

Expand Down
1 change: 1 addition & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ install_requires =
cookiecutter >= 2.1, < 3.0
dmgbuild >= 1.6, < 2.0; sys_platform == "darwin"
GitPython >= 3.1, < 4.0
pip-tools >= 6.13, < 7.0
platformdirs >= 2.6, < 4.0
psutil >= 5.9, < 6.0
requests >= 2.28, < 3.0
Expand Down
6 changes: 6 additions & 0 deletions src/briefcase/commands/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,12 @@ def _add_update_options(
help=f"Update requirements for the app{context_label}",
)

parser.add_argument(
"--relock",
action="store_true",
help=f"Recalculate locked requirements for the app{context_label}",
)
Comment on lines +676 to +680
Copy link
Member

Choose a reason for hiding this comment

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

Why "relock" rather than simply "lock"? "Relock" implies that it was locked before, which isn't necessarily the case.

Copy link
Member

Choose a reason for hiding this comment

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

In the way it's defined in the PR at present, it is a relock, because the initial create call always passes relock=True, automatically creating the lock file if the setting exists. This is part of my issue with this PR, as you need to specify a lock file that doesn't exist to get the initial invocation to work as you'd expect. So, "re" part of the relocking is present, but it won't always be clear how you get to the relock in the first place.

If the setting is also exposed to the create command, then it would definitely make sense for the option to be --lock. It might even make sense regardless, describing "locking" as an imperative action that is part of a build.


parser.add_argument(
"--update-resources",
action="store_true",
Expand Down
50 changes: 45 additions & 5 deletions src/briefcase/commands/create.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import shutil
import subprocess
import sys
import tempfile
from datetime import date
from pathlib import Path
from typing import List, Optional
Expand Down Expand Up @@ -487,7 +488,48 @@ def _install_app_requirements(
else:
self.logger.info("No application requirements.")

def install_app_requirements(self, app: BaseConfig, test_mode: bool):
def _calc_app_requirements(self, app: BaseConfig, test_mode: bool, relock: bool):
requires = app.requires.copy() if app.requires else []
if test_mode and app.test_requires:
requires.extend(app.test_requires)

lock_attr = "requires_lock" if not test_mode else "test_requires_lock"
try:
lock_value = getattr(app, lock_attr)
except AttributeError:
return requires
else:
lock_file = self.base_path / lock_value

if relock:
with tempfile.NamedTemporaryFile(mode="w+") as fpout:
fpout.writelines(line + "\n" for line in requires)
fpout.flush()
res = self.tools[app].app_context.run(
[
sys.executable,
"-u",
"-m",
"piptools",
"compile",
fpout.name,
"--output-file=-",
],
text=True,
capture_output=True,
check=True,
)
Copy link
Member

Choose a reason for hiding this comment

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

So... did you run this? Briefcase's run wrapper doesn't support capture_output, and doesn't need text, and this crashes for me on a briefcase build -r --relock.

If all we're looking to do is capture all of the output, capture_output() would be a better match; or, is there a reason to do the whole dance through stdout at all? Why not just output directly to the lock file (possibly with a -U flag to force an upgrade of any existing locked requirements)?

lock_file.write_text(res.stdout)

requires = [
line.split("#", 1)[0].strip() for line in lock_file.read_text().splitlines()
]
requires = [potential for potential in requires if potential != ""]
return requires

def install_app_requirements(
self, app: BaseConfig, test_mode: bool, relock: bool = False
Copy link
Member

Choose a reason for hiding this comment

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

relock shouldn't have a default value here; we're in a position to be explicit whenever it's used.

):
"""Handle requirements for the app.

This will result in either (in preferential order):
Expand All @@ -504,9 +546,7 @@ def install_app_requirements(self, app: BaseConfig, test_mode: bool):
:param app: The config object for the app
:param test_mode: Should the test requirements be installed?
Copy link
Member

Choose a reason for hiding this comment

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

There's a missing docstring here.

"""
requires = app.requires.copy() if app.requires else []
if test_mode and app.test_requires:
requires.extend(app.test_requires)
requires = self._calc_app_requirements(app, test_mode, relock)

try:
requirements_path = self.app_requirements_path(app)
Expand Down Expand Up @@ -766,7 +806,7 @@ def create_app(self, app: BaseConfig, test_mode: bool = False, **options):
self.install_app_code(app=app, test_mode=test_mode)

self.logger.info("Installing requirements...", prefix=app.app_name)
self.install_app_requirements(app=app, test_mode=test_mode)
self.install_app_requirements(app=app, test_mode=test_mode, relock=True)

self.logger.info("Installing application resources...", prefix=app.app_name)
self.install_app_resources(app=app)
Expand Down
3 changes: 2 additions & 1 deletion src/briefcase/commands/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ def update_app(
update_requirements: bool,
update_resources: bool,
test_mode: bool,
relock: bool,
**options,
):
"""Update an existing application bundle.
Expand All @@ -47,7 +48,7 @@ def update_app(

if update_requirements:
self.logger.info("Updating requirements...", prefix=app.app_name)
self.install_app_requirements(app=app, test_mode=test_mode)
self.install_app_requirements(app=app, test_mode=test_mode, relock=relock)

if update_resources:
self.logger.info("Updating application resources...", prefix=app.app_name)
Expand Down
38 changes: 23 additions & 15 deletions tests/commands/build/test_call.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,14 @@
from briefcase.exceptions import BriefcaseCommandError


def _clean_relock(actions):
for cmd, *rest in actions:
if cmd not in {"run", "build", "create", "update"}:
continue
rest[-1].pop("relock", None)
return actions
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this is in aid of... the point of these tests is to confirm that the right arguments are being passed in to each stage. Post-processing the results to remove one of the arguments... doesn't make any sense to me.



def test_specific_app(build_command, first_app, second_app):
"""If a specific app is requested, build it."""
# Add two apps
Expand All @@ -18,7 +26,7 @@ def test_specific_app(build_command, first_app, second_app):
build_command(first_app, **options)

# The right sequence of things will be done
assert build_command.actions == [
assert _clean_relock(build_command.actions) == [
# Host OS is verified
("verify-host",),
# Tools are verified
Expand Down Expand Up @@ -47,7 +55,7 @@ def test_multiple_apps(build_command, first_app, second_app):
build_command(**options)

# The right sequence of things will be done
assert build_command.actions == [
assert _clean_relock(build_command.actions) == [
# Host OS is verified
("verify-host",),
# Tools are verified
Expand Down Expand Up @@ -81,7 +89,7 @@ def test_non_existent(build_command, first_app_config, second_app):
build_command(**options)

# The right sequence of things will be done
assert build_command.actions == [
assert _clean_relock(build_command.actions) == [
# Host OS is verified
("verify-host",),
# Tools are verified
Expand Down Expand Up @@ -121,7 +129,7 @@ def test_unbuilt(build_command, first_app_unbuilt, second_app):
build_command(**options)

# The right sequence of things will be done
assert build_command.actions == [
assert _clean_relock(build_command.actions) == [
# Host OS is verified
("verify-host",),
# Tools are verified
Expand Down Expand Up @@ -155,7 +163,7 @@ def test_update_app(build_command, first_app, second_app):
build_command(**options)

# The right sequence of things will be done
assert build_command.actions == [
assert _clean_relock(build_command.actions) == [
# Host OS is verified
("verify-host",),
# Tools are verified
Expand Down Expand Up @@ -213,7 +221,7 @@ def test_update_app_requirements(build_command, first_app, second_app):
build_command(**options)

# The right sequence of things will be done
assert build_command.actions == [
assert _clean_relock(build_command.actions) == [
# Host OS is verified
("verify-host",),
# Tools are verified
Expand Down Expand Up @@ -271,7 +279,7 @@ def test_update_app_resources(build_command, first_app, second_app):
build_command(**options)

# The right sequence of things will be done
assert build_command.actions == [
assert _clean_relock(build_command.actions) == [
# Host OS is verified
("verify-host",),
# Tools are verified
Expand Down Expand Up @@ -329,7 +337,7 @@ def test_update_non_existent(build_command, first_app_config, second_app):
build_command(**options)

# The right sequence of things will be done
assert build_command.actions == [
assert _clean_relock(build_command.actions) == [
# Host OS is verified
("verify-host",),
# Tools are verified
Expand Down Expand Up @@ -384,7 +392,7 @@ def test_update_unbuilt(build_command, first_app_unbuilt, second_app):
build_command(**options)

# The right sequence of things will be done
assert build_command.actions == [
assert _clean_relock(build_command.actions) == [
# Host OS is verified
("verify-host",),
# Tools are verified
Expand Down Expand Up @@ -442,7 +450,7 @@ def test_build_test(build_command, first_app, second_app):
build_command(**options)

# The right sequence of things will be done
assert build_command.actions == [
assert _clean_relock(build_command.actions) == [
# Host OS is verified
("verify-host",),
# Tools are verified
Expand Down Expand Up @@ -501,7 +509,7 @@ def test_build_test_no_update(build_command, first_app, second_app):
build_command(**options)

# The right sequence of things will be done
assert build_command.actions == [
assert _clean_relock(build_command.actions) == [
# Host OS is verified
("verify-host",),
# Tools are verified
Expand Down Expand Up @@ -540,7 +548,7 @@ def test_build_test_update_dependencies(build_command, first_app, second_app):
build_command(**options)

# The right sequence of things will be done
assert build_command.actions == [
assert _clean_relock(build_command.actions) == [
# Host OS is verified
("verify-host",),
# Tools are verified
Expand Down Expand Up @@ -599,7 +607,7 @@ def test_build_test_update_resources(build_command, first_app, second_app):
build_command(**options)

# The right sequence of things will be done
assert build_command.actions == [
assert _clean_relock(build_command.actions) == [
# Host OS is verified
("verify-host",),
# Tools are verified
Expand Down Expand Up @@ -716,7 +724,7 @@ def test_test_app_non_existent(build_command, first_app_config, second_app):
build_command(**options)

# The right sequence of things will be done
assert build_command.actions == [
assert _clean_relock(build_command.actions) == [
# Host OS is verified
("verify-host",),
# Tools are verified
Expand Down Expand Up @@ -772,7 +780,7 @@ def test_test_app_unbuilt(build_command, first_app_unbuilt, second_app):
build_command(**options)

# The right sequence of things will be done
assert build_command.actions == [
assert _clean_relock(build_command.actions) == [
# Host OS is verified
("verify-host",),
# Tools are verified
Expand Down
2 changes: 1 addition & 1 deletion tests/commands/create/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ def generate_app_template(self, app):
def install_app_support_package(self, app):
self.actions.append(("support", app.app_name))

def install_app_requirements(self, app, test_mode):
def install_app_requirements(self, app, test_mode, relock=False):
self.actions.append(("requirements", app.app_name, test_mode))
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 also be tracking the provided argument in the list of actions.


def install_app_code(self, app, test_mode):
Expand Down
Loading