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

moshez · 2023-04-24T14:55:34Z

In that case, instead of merging, we override

Fixes: #476

PR Checklist:

All new features have been tested
All new features have been documented
I have read the CONTRIBUTING.md file
I will abide by the code of conduct

… that case, instead of merging, we override Signed-off-by: Moshe Zadka <moshez@zadka.club>

freakboy3742

So - there's a start of a locking implementation here, but there's still a lot of work needed.

I've flagged a bunch of stuff inline that stood out on a first pass - the most notable of which is that the code doesn't run as provided. The invocation of piptools straight up crashes in my testing... and I can't see how it has ever worked.

There's three "bigger picture" issues that need some work as well.

It seems like there's a bit of a missed opportunity for a simplification. This PR is using pistols to produce a requirements.txt file, which is then being post-processed back into a list of requirements. If we're going to the trouble of building a requirements file for the purposes of locking, it seems foolish to not to just use it as an input to pip.
It misses the overlap with the existing install_app_requirements() invocation that is already requirements file based. Some platforms (notably, Android, plus the AppImage and System Linux backends) don't use pip to install requirements directly - they write a requirements file based on the requires list provided.
Continuing that theme - there is some non-trivial backend-specific handling of requirements in the Linux, Android and web backends. It's not immediately clear to me that the changes made in this PR will be compatible with that backend handling (and, because the PR doesn't run as provided, I can't test those theories).
Lastly - the general workflow wasn't obvious to me on first pass. I would have expected a workflow where create generated a lock file when requirements were installed (possibly requiring a --lock argument); the user could then choose to adopt that lock file by adding it to their pyproject.toml. I'm not absolutely wedded to that as a UX - but requiring the user to modify the pyproject.toml to reference a requires_lock file that doesn't yet exist in order to generate a lock file with that name was definitely contrary to what I would have expected.

freakboy3742 · 2023-05-05T05:44:58Z

src/briefcase/commands/create.py

+ text=True,
+ capture_output=True,
+ check=True,
+ )


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)?

freakboy3742 · 2023-05-05T05:47:50Z

tests/commands/build/test_call.py

+ if cmd not in {"run", "build", "create", "update"}:
+ continue
+ rest[-1].pop("relock", None)
+ return actions


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.

freakboy3742 · 2023-05-05T05:49:23Z

docs/reference/configuration.rst

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


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

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

freakboy3742 · 2023-05-05T06:10:13Z

tests/commands/update/conftest.py

@@ -46,7 +46,7 @@ def verify_app_tools(self, app):

 # Override all the body methods of a UpdateCommand
 # with versions that we can use to track actions performed.
- 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))


This should also be tracking the provided argument in the list of actions.

freakboy3742 · 2023-05-05T06:10:19Z

tests/commands/create/conftest.py

@@ -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))


This should also be tracking the provided argument in the list of actions.

freakboy3742 · 2023-05-05T06:10:51Z

src/briefcase/commands/create.py

@@ -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?


There's a missing docstring here.

freakboy3742 · 2023-05-05T06:11:18Z

src/briefcase/commands/create.py

+ return requires
+
+ def install_app_requirements(
+ self, app: BaseConfig, test_mode: bool, relock: bool = False


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

mhsmith · 2023-05-09T21:31:36Z

src/briefcase/commands/base.py

+ parser.add_argument(
+ "--relock",
+ action="store_true",
+ help=f"Recalculate locked requirements for the app{context_label}",
+ )


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

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.

mhsmith · 2023-05-16T13:44:07Z

I would have expected a workflow where create generated a lock file when requirements were installed (possibly requiring a --lock argument); the user could then choose to adopt that lock file by adding it to their pyproject.toml.

That seems a bit awkward. Instead of requiring the user to pick a filename, why not just auto-generate the filename based on the platform and format names? Then pyproject.toml doesn't need to be edited; instead, we have the following scenarios:

Every command which can install requirements has a --lock option which generates or re-generates the lock file.
If the lock file exists, it's used automatically in any subsequent commands.
Maybe we also have an --unlock option to remove the lock file. In that case, the the user would never need to touch the file directly, and its name, location and format would become internal implementation details which we'd be free to change in the future.

I don't have a clear idea of the file's name or location, but maybe we could store it under a briefcase or .briefcase subdirectory of the project. This could be used to store other things which are internal to Briefcase, but should still be checked into version control, unlike build.

freakboy3742 · 2023-05-17T08:15:17Z

I would have expected a workflow where create generated a lock file when requirements were installed (possibly requiring a --lock argument); the user could then choose to adopt that lock file by adding it to their pyproject.toml.

That seems a bit awkward. Instead of requiring the user to pick a filename, why not just auto-generate the filename based on the platform and format names? Then pyproject.toml doesn't need to be edited; instead, we have the following scenarios:

So, one reason is that people can be... unreasonably opinionated about the names they use for their lock files :-)

Every command which can install requirements has a --lock option which generates or re-generates the lock file.

If the lock file exists, it's used automatically in any subsequent commands.

Maybe we also have an --unlock option to remove the lock file. In that case, the the user would never need to touch the file directly, and its name, location and format would become internal implementation details which we'd be free to change in the future.

I do like this as a workflow. My hesitation is around the slightly opaque user experience - "I've added foobar==1.2.3 to my requirements, but it's not getting installed" becomes even harder to diagnose. This is always going to be the case when lock files are involved, but when the lock file is completely implied, there's no evidence in the config file that directs the user's attention to the fact that locking is being performed.

I don't have a clear idea of the file's name or location, but maybe we could store it under a briefcase or .briefcase subdirectory of the project. This could be used to store other things which are internal to Briefcase, but should still be checked into version control, unlike build.

This is where Opinions (tm) get involved :-) The only conventions I've seen around requirements are either (1) in the project root, or (2) in a requirements directory - both are workable here.

mhsmith · 2023-05-18T09:33:40Z

My hesitation is around the slightly opaque user experience - "I've added foobar==1.2.3 to my requirements, but it's not getting installed" becomes even harder to diagnose.

I think changing the requirements list should always invalidate an existing lock file. We could store the original (merged but unlocked) requirements file next to the lock file, and compare it with the current configuration.

This also touches on the automatic update idea discussed at #807 (comment).

The only conventions I've seen around requirements are either (1) in the project root, or (2) in a requirements directory - both are workable here.

If we do decide that the lock file is an internal implementation detail, I think it should be stored in a directory whose name makes that clear.

freakboy3742 · 2023-05-18T21:55:17Z

My hesitation is around the slightly opaque user experience - "I've added foobar==1.2.3 to my requirements, but it's not getting installed" becomes even harder to diagnose.

I think changing the requirements list should always invalidate an existing lock file. We could store the original (merged but unlocked) requirements file next to the lock file, and compare it with the current configuration.

This also touches on the automatic update idea discussed at #807 (comment).

Ah - the overlap with #807 changes things significantly. I was considering the lock file as something you could (concievably, although you're not likely to) write by hand, in which case manual specification and customisation options make more sense. Having the lock autogenerated on any change to requirements is another matter entirely.

The only conventions I've seen around requirements are either (1) in the project root, or (2) in a requirements directory - both are workable here.

If we do decide that the lock file is an internal implementation detail, I think it should be stored in a directory whose name makes that clear.

Agreed; although the lock file is something that would be checked into version control, so we need to be careful not to make it too hidden; and we need to make sure that it any "#807 state" information that feeds the lock file invalidation process is also storable in version control.

moshez added 4 commits April 24, 2023 08:54

Override requirements (at a given level) from a requirements file. In…

9a6e968

… that case, instead of merging, we override Signed-off-by: Moshe Zadka <moshez@zadka.club>

lock file at point of reading

d8b0c09

support relocking

96c6018

+stuff

68bcfe6

moshez force-pushed the require-file branch from 7f4eb04 to 68bcfe6 Compare April 24, 2023 16:17

moshez added 10 commits April 24, 2023 10:24

docs

23f201d

fix some tests

72c48f8

more test fixes

7f578ad

fix stuff

3f42195

frop

6e31779

blackify

eb6c7b9

stuff

7f921bb

stuff

e6664c8

stuff

9cc1f65

test prelocking

1c9555b

moshez marked this pull request as ready for review April 24, 2023 18:43

moshez changed the title ~~(Draft) Override requirements (at a given level) from a requirements file.~~ Override requirements (at a given level) from a requirements file. Apr 24, 2023

freakboy3742 requested changes May 5, 2023

View reviewed changes

freakboy3742 added the not quite right The idea or PR has been reviewed, but more work is needed. label May 8, 2023

mhsmith reviewed May 9, 2023

View reviewed changes

This was referenced May 11, 2023

Install a python requirement with an --extra-index-url argument #1270

Open

Allow pyproject.toml to reference a requirements file #1275

Open

beeware deleted a comment from freakboy3742 May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

moshez commented Apr 24, 2023 •

edited

Loading

freakboy3742 left a comment

freakboy3742 May 5, 2023

freakboy3742 May 5, 2023

freakboy3742 May 5, 2023

mhsmith May 11, 2023 •

edited

Loading

freakboy3742 May 5, 2023

freakboy3742 May 5, 2023

freakboy3742 May 5, 2023

freakboy3742 May 5, 2023

mhsmith May 9, 2023

freakboy3742 May 9, 2023

mhsmith commented May 16, 2023

freakboy3742 commented May 17, 2023

mhsmith commented May 18, 2023

freakboy3742 commented May 18, 2023

		@@ -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?

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

Are you sure you want to change the base?

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

Conversation

moshez commented Apr 24, 2023 • edited Loading

PR Checklist:

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsmith May 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsmith commented May 16, 2023

freakboy3742 commented May 17, 2023

mhsmith commented May 18, 2023

freakboy3742 commented May 18, 2023

moshez commented Apr 24, 2023 •

edited

Loading

mhsmith May 11, 2023 •

edited

Loading