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

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

Open
mocaccano opened this issue May 11, 2023 · 40 comments · May be fixed by #2059
Open

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

mocaccano opened this issue May 11, 2023 · 40 comments · May be fixed by #2059
Labels
enhancement New features, or improvements to existing features. good first issue Is this your first time contributing? This could be a good place to start!

Comments

@mocaccano
Copy link

What is the problem or limitation you are having?

Hi, I have a project which requires PyTorch CPU version, not the default GPU version. Normally, it's possible to install this in one of two ways using pip:

pip install torch torchvision --extra-index-url https://download.pytorch.org/whl/cpu

or

pip install -r requirements.txt

where the requirements look like:

--extra-index-url https://download.pytorch.org/whl/cpu
torch==2.0.1
torchvision==0.15.2

Describe the solution you'd like

How do I translate this to the Briefcase environment? This is a question rather than a solution, but I'm new to Beeware and don't see any obvious way of including --extra-index-url in the requires section of the pyproject.toml file.

Describe alternatives you've considered

Thanks!

Additional context

No response

@mocaccano mocaccano added the enhancement New features, or improvements to existing features. label May 11, 2023
@mhsmith
Copy link
Member

mhsmith commented May 11, 2023

It would be useful to allow the pyproject.toml file to reference a requirements file, but we don't currently support that (#1275). Meanwhile, you should be able to set the option using an environment variable PIP_EXTRA_INDEX_URL, as shown here.

@freakboy3742
Copy link
Member

Agreed that this is something we should formally support.

In addition to the PIP_EXTRA_INDEX_URL workaround, you could download the wheel manually, and add a direct reference to the wheel in your requires list.

Another workaround - although one that is a lot more fragile, and I wouldn't really want to rely on long term - would be to add ["--extra-index-url", "https:///...." to your requires list. By a quirk of implementation, it should work on macOS, iOS and Cocoa. On Android and Linux, adding "--extra-index-url https:///...." might work. This is obviously very fragile, but as a workaround, it might do the job.

@mocaccano
Copy link
Author

mocaccano commented May 12, 2023

Thank you for your responses. I tried adding "--extra-index-url https://...." to requires but it didn't work. I am on Ubuntu. On the other hand PIP_EXTRA_INDEX_URL works very well, so I'm using this for now. Manual wheel could work, but haven't tried it so far.

@rmartin16
Copy link
Member

I tried adding "--extra-index-url https://...." to requires but it didn't work. I am on Ubuntu.

To add to the fragility of this workaround, you can't have the space separating the option and its value...an equal sign must be used. This should be true for any pip option you want to use:

 requires = [
     "--extra-index-url=https://download.pytorch.org/whl/cpu",
     "torch==2.0.1",
     "toga-gtk~=0.3.1",
 ]

@freakboy3742 freakboy3742 added the good first issue Is this your first time contributing? This could be a good place to start! label Jul 30, 2023
@wyrd-0
Copy link

wyrd-0 commented Aug 14, 2023

Agreed that this is something we should formally support.

In addition to the PIP_EXTRA_INDEX_URL workaround, you could download the wheel manually, and add a direct reference to the wheel in your requires list.

Another workaround - although one that is a lot more fragile, and I wouldn't really want to rely on long term - would be to add ["--extra-index-url", "https:///...." to your requires list. By a quirk of implementation, it should work on macOS, iOS and Cocoa. On Android and Linux, adding "--extra-index-url https:///...." might work. This is obviously very fragile, but as a workaround, it might do the job.

New here, but I just got through the tutorial and I'm looking to contribute. Wanted some clarity about what the most desirable solution would be:

  1. extend briefcase to enable passing in a requirements.txt file OR
  2. extend briefcase to enable passing in multiple --extra-index-url parameters and passing them along to pip OR
  3. add [extra-index-urls] to pyproject.toml and incorporate that into updates

@mhsmith
Copy link
Member

mhsmith commented Aug 14, 2023

The most flexible way would be to allow arbitrary pip options, like Chaquopy does. @freakboy3742 @rmartin16: any thoughts?

@freakboy3742
Copy link
Member

That's definitely an option; my primary hesitation is the possibility that at some point in the future, we might provide an install option other than pip (the most likely alternative being Conda; but in theory, there could be others), as well as (potentially) lock file formats that are bound to specific tools (e.g., poetry).

Baking pip options directly into the config could back us into a corner, whereas "repository url" and/or "extra repository URL" is less likely to be a problem.

@wyrd-0
Copy link

wyrd-0 commented Aug 16, 2023

That's definitely an option; my primary hesitation is the possibility that at some point in the future, we might provide an install option other than pip (the most likely alternative being Conda; but in theory, there could be others), as well as (potentially) lock file formats that are bound to specific tools (e.g., poetry).

Baking pip options directly into the config could back us into a corner, whereas "repository url" and/or "extra repository URL" is less likely to be a problem.

@freakboy3742, how about I then add repository_url and extra_repository_url under
[tool.briefcase.app.{project_name}], which will be ignored if left empty, and otherwise passed to the package installer?

@freakboy3742
Copy link
Member

Sounds like a good approach to me; with the caveat that the extra needs to be a list, not just a single URL (since you can specify multiple extra URLs)

@rfletchr
Copy link

rfletchr commented Aug 21, 2023

It would be fantastic if we could simply opt to disable briefcases package resolution and use an existing system like setup tools (boring tech FTW).

from setuptools import setup

setup(
    name='somepackage',
    install_requires=[
        'somedep'
    ],
    dependency_links=[
        'https://pypi.example.org/pypi/somedep/'
    ]
    # ...
)
pip install .
briefcase dev

I mean why re-invent the whl;)

@freakboy3742
Copy link
Member

@rfletchr Briefcase will honor PEP621 metadata (now formally documented here). That means dependencies will be honoured as a [project] key; but dependency_links isn't a PEP621 key.

Beyond that, setuptools doesn't have sufficient flexibility to define the packaging needs of a bundled Python app, so it's not possible to fully specify a Briefcase configuration purely with setuptools.

@freakboy3742
Copy link
Member

Adding to the feature list - we probably also want to support --find-links with a local_wheel_path setting so that we can point at a directory of local wheels.

@Koffilo

This comment was marked as off-topic.

@rmartin16
Copy link
Member

@Koffilo, please open a discussion if you have a question about BeeWare. Also please stop posting other questions in existing issues.

@sarayourfriend
Copy link
Contributor

I've got most of a PR ready for this change, however, I wanted to clarify something: pip's --extra-index-url and --index-url option supports local repository paths as well. I'm not sure how to test this well, and I'm concerned about how builds would work if that's supported by Briefcase, especially remote builds (like linux builds in Docker containers). However, I'm not confident in how that works to know whether an absolute reference wouldn't be sufficient. Anything in requires that is a local dependency gets resolved to an absolute path, but I haven't looked deeply to see if there is any additional handling for that required.

Additionally, to add support for a local_wheel_path setting (to support --find-links), would this problem need to be solved anyway? I've never used --find-links personally, so not familiar with the use case to understand it well enough.

Solving this for remote URLs was relatively straightforward. The local paths are tripping me up a bit 😅.

@freakboy3742
Copy link
Member

I'm not sure excessive testing of --extra-index-url is necessary - if we can confirm that the value provided as configuration item meets basic validation (i.e., it's a URL), and it is passed in the right way to the underlying pip command, then I'm happy to leave it as an issue for pip to report whether that URL is valid or not.

As for --find-links - that's one that BeeWare uses - see this configuration that is used to provide binary wheels that haven't been published to verify that the BeeWare support packages are working as expected. The value is any local file system location; any wheels in that location are evaluated for a match.

IIRC, the difference between --find-links and --(extra-)index-url is that the former exposes a single flat directory, whereas the latter requires a specific directory structure where each project is in a separate subfolder. I haven't tried to see if a file:// URL can be used for this... all the online references I've seen suggest running python -m http.server to expose a local repository.

Something that I hadn't considered before as a possibility that would avoid the need for an extra local configuration items - we could use the differentiation between URL and non-URL to determine whether --find-links or --extra-index-url is used. If you use a URL (be that https:// or file://), the value is passed as --extra-index-url; anything else is evaluated as a filesystem location, and passed to --find-links (possibly after ensuring that the directory actually exists). I assume that the base index URL would then fail if the value provided isn't a URL.

@sarayourfriend
Copy link
Contributor

sarayourfriend commented Nov 3, 2024

TL;DR: There are problems with using local directory references for index-url due to build context changes with Flatpak and Docker-based builds (explained below). Are Briefcase's options meant to cover all usages of pip's options? If so, then my branch needs more work to support that, and especially the Docker-based functionality of it should be tested. If not (i.e., if only live HTTP URLs are meant to be supported), then the branch is probably near feature complete for repository_url and extra..., just lacking documentation and full unit test coverage. More work would be needed either way if the PR must support find-links as well (could push that off into a separate PR to make review easier, happy to work on it regardless).


Interesting to know about the find-links usage. For what it's worth, pip's documentation explicitly says it will accept a local directory reference for --(extra-)index-url:

--index-url: [...] This should point to a repository compliant with PEP 503 (the simple repository API) or a local directory laid out in the same format.

https://pip.pypa.io/en/stable/cli/pip_install/#cmdoption-i (emphasis mine)

--extra-index-url says it takes the same option format as --index-url. You don't need to run an HTTP server for this either, as long as you provide index.html files that conform to PEP 503 (the linked example repository below demonstrates this). --find-links explicitly supports a file:// URL and doesn't need the HTML files (I assume, based on your description of how BeeWare uses it).

The fact that pip supports these formats are, I agree, outside of Briefcase's concerns, and Briefcase shouldn't test pip, just that it's giving pip the right options.

To clarify my concern, it is that if the intention behind repository_url and extra_repository_url is to support the same exact option choices as pip, then local package directory references become a problem for certain builds. Here is an example repository that depends on the branch I'm working on which demonstrates this working. I'm on Fedora, where briefcase build linux works perfectly. It sources the myhttpx package from the packages directory (referenced as --extra-index-url=./packages in the resultant pip arguments) and installs everything perfectly. However, briefcase build linux flatpak fails, because ./packages is not in the flatpak builder's scope. If I change ./packages to an absolute path, flatpak builds successfully and finds myhttpx (regardless of whether the file:// scheme is specified). On the other hand, briefcase build linux --target ubuntu:latest fails regardless (with relative or absolute path) because the path isn't mapped into the container.

So, the question is this: is the intention for Briefcase to support local path references in these options the way pip does? Or, should Briefcase explicitly only support URLs? In other words, in the spirit of pip being an implementation detail, can Briefcase take a more conservative approach, and thereby avoid the complexity of needing to manage local path references for each of these contexts?

As things currently stand, because this isn't an officially supported option in Briefcase, it makes sense that the developer experience of trying to use a local directory reference is broken and that users would need to come up with their own workarounds. It doesn't seem to me that Briefcase is necessarily bound to support those edge cases, and by using repository instead of index, a certain distance is already established. In effect, Briefcase's naming already asserts, this is "kind of like [index-url/extra-index-url]" but not necessarily "exactly those". Such distinctions between Briefcase's option and pip's would need to be absolutely clear in the documentation.

On the other hand, if the intention is to support all potential uses of those two pip options, then Briefcase needs to ensure local directory references work in all these cases. My point about testing was that I wasn't sure how to verify if the local directory references would be an issue (I'd never used these options with Briefcase before), but it's clear that they would be.

I assume all of this all holds for an officially supported version of find-links, and that the only reason the specific usage you linked does not present this problem is that macOS builds necessarily happen on a local macOS host (i.e., no Docker), and the create command converts the local requirement reference to an absolute path (see CreateCommand._write_requirements_file), so even if the build directory is not the base_path, the reference still resolves for --find-links.

@sarayourfriend
Copy link
Contributor

Quick clarification: handling the relative vs absolute path references is trivial, and would just need to follow the same logic as is used for local references in requires. Doing so would cover every case except Docker. The potentially non-trivial problem is making sure these are all appropriately mapped in the Docker container. The further philosophical question of whether Briefcase intends to support exactly the same usage as pip for these options can be considered separately from whether it's possible to sensibly implement (it is possible).

@freakboy3742
Copy link
Member

TL;DR: There are problems with using local directory references for index-url due to build context changes with Flatpak and Docker-based builds (explained below). Are Briefcase's options meant to cover all usages of pip's options?

In short, no.

In long... the longer term goal is that the "environment management" aspect of Briefcase will become pluggable. Although Briefcase currently uses venv and pip, you could equally use uv or conda; or we could add support for using poetry, pdm, etc (or, at least, provide a hook so that people could provide an implementation supporting those tools). Providing those hooks/options is something that is very much on my todo list (for conda in particular, for reasons that shouldn't be surprising if you know who my employer is 😄)

This also means that shouldn't be aiming to expose every possible feature of pip literally. As with Toga, the goal should be to abstract the high level ideas that transcend any specific environment management implementation; longer term, if we need to add support for a specific pip option, it would be in a pip-specific configuration block (similar to how we have platform-generic permission management options, but iOS- and Android-specific mechanisms to output platform-specific permissions). In this context we need to be able to specify "where do you get your packages from" in a way that isn't a hack dependent on a specific interpretation of pip arguments.

That said - if you provide a list of package repositories, it's then up to the installer how those sources are interpreted. I wouldn't expect a conda installer to be able to make sense of a pip-layout repository. But conda might need to be pointed at a different root conda repository, or supplemental repositories, or a local filesystem with pre-build conda packages.

As for how to handle the limitations of local directory references - I'd be entirely comfortable if v1 of this feature didn't allow local filesystem references for Flatpak or Docker builds. It should be possible (if somewhat complex) to expose a file location for Docker by mapping the filesystem mount - but that could be added as a future enhancement. It might even be possible to resolve this for Flatpak by linking filesystems; but again, that can be a future enhancement. In the short term, I'm entirely comfortable documenting the limitation as a platform-specific quirk, and the platforms that have the limitation validate and report the configuration issue in a helpful way.

If so, then my branch needs more work to support that, and especially the Docker-based functionality of it should be tested. If not (i.e., if only live HTTP URLs are meant to be supported), then the branch is probably near feature complete for repository_url and extra..., just lacking documentation and full unit test coverage. More work would be needed either way if the PR must support find-links as well (could push that off into a separate PR to make review easier, happy to work on it regardless).

Interesting to know about the find-links usage. For what it's worth, pip's documentation explicitly says it will accept a local directory reference for --(extra-)index-url:
...
--extra-index-url says it takes the same option format as --index-url. You don't need to run an HTTP server for this either, as long as you provide index.html files that conform to PEP 503 (the linked example repository below demonstrates this). --find-links explicitly supports a file:// URL and doesn't need the HTML files (I assume, based on your description of how BeeWare uses it).

Interesting... I guess that upgrades the test to "filesystem location exists and contains an index.html" over whether a path is used as an --(extra-)index-url or a --find-links source. (And, to explicitly join the dots with the above - that becomes a "pip backend" interpretation of the setting; conda might interpret this a different way)

To clarify my concern, it is that if the intention behind repository_url and extra_repository_url is to support the same exact option choices as pip, then local package directory references become a problem for certain builds.
...
So, the question is this: is the intention for Briefcase to support local path references in these options the way pip does? Or, should Briefcase explicitly only support URLs? In other words, in the spirit of pip being an implementation detail, can Briefcase take a more conservative approach, and thereby avoid the complexity of needing to manage local path references for each of these contexts?

I'd say the guiding principle here is "does the underlying idea of the configuration option being exposed make sense regardless of the specific implementation?". That doesn't mean the feature is implemented everywhere - but there needs to at least be a potential interpretation that would make sense. Beyond that, we take a "best effort" approach. A local filesystem path will work in a lot of situations; there are time that it won't, and those edge cases we can document (and treat as future enhancements). Does that make sense?

(Also - in the spirit of late-design bike sheds - I'm also wondering whether the name of the option should be repository_url - explicitly suggesting a URL - or something more generic like package_repository. By analogy, the option is template, not template_url, specifically because it doesn't have to be a URL - a URL is just a very common input format).

@sarayourfriend
Copy link
Contributor

Okay, thanks for the thorough response. This makes perfect sense, and is in line with what I was thinking as well. I'll get that branch cleaned up and ready for review by sometime tomorrow afternoon 🙂. We can bike shed the option names in the PR 😀

@sarayourfriend
Copy link
Contributor

sarayourfriend commented Nov 5, 2024

I've got the PR up as a draft here: #2057

However, in finalising it with documentation, and thinking further about how to support --find-links, it struck me that trying to find generic options that could in the future map to a variety of different package installer backends may be misguided. --find-links in particular, I could not think of a generic way to describe it that didn't ultimately rely on pip's implementation details. The difference between it and --extra-index-url are really pip implementation details, and might be completely irrelevant for other package repository formats.

So: rather than trying to abstract these package installer options into a few generic names, and considering that support for different installers is on the roadmap, what if briefcase instead added support for explicit configuration of those installers' options? For now, only pip would have it, but future installers, however they are implemented, could rely on a loose API with a uniform entrypoint for retrieving configuration options.

For example, could something like this work better to separate these concerns?

[tool.briefcase.app.helloworld]
# This would be the default and for now the only option
requirement_installer = "pip"

[tool.briefcase.pip_options]
index_url = ""  # maps explicitly to the option of the same name in pip, no ambiguity, no abstraction
extra_index_urls = []  # ditto!
find_links = ""  # ditto!

Then, turn pip into another Tool implementation. CreateCommand.install_app_requirements then does a rough equivalent of getattr(self.tools[app], app.requirement_installer).install_app_requirements(). The pip Tool handles the implementation details of transforming the AppConfig (and GlobalConfig.pip_options) into the arguments required to install the app's requirements.

This is, admittedly, a huge change, as the methods used on CreateCommand to construct the pip arguments are overridden by various subclasses. But, I think it's necessary to consider here as a more viable and simpler long-term option compared to trying to abstract options specific to the implementation details of particular package installers.

A similar approach could be taken to selecting the environment manager, if relevant.

@freakboy3742 what do you think?

@sarayourfriend
Copy link
Contributor

Addendum: it strikes me that the installer options might be best suited on AppConfig rather than on the global config so that they could be extended/overridden by platform if needed. Nesting them in a table named explicitly for the installer being used would still be necessary to avoid the problematic abstraction of options.

@freakboy3742
Copy link
Member

So: rather than trying to abstract these package installer options into a few generic names, and considering that support for different installers is on the roadmap, what if briefcase instead added support for explicit configuration of those installers' options? For now, only pip would have it, but future installers, however they are implemented, could rely on a loose API with a uniform entrypoint for retrieving configuration options.

It's taken me a while to convince myself, but I think I'm inclined to agree. As much as I'd like to keep generic options to "keep it simple", every abstraction layer inevitably leads to gaps; and in this case, we're already talking about niche customisation, so the gaps are where we're going to get bitten.

The real tipping point for me was digging into the full list of values that --find-links and --index-url can accept. Both accept URLs, and both accept filesystem locations; and while we could make some guesses about the likely content that will almost certainly serve 90% (or more) of use cases, it seems shortsighted to require significantly smarter logic to determine whether a URL/filesystem location is an index or a find-links target, purely so we can keep a couple of settings generic. Any "smarts" will need to be implemented, tested, and maintained... or we can just expose "--find-links" verbatim in a pip_options block (although bikeshedding, I would be inclined to keep the option simple as tools.briefcase.pip)

The only edge case I can think of is backends (or backend-specific app configuration) that need to provide specific overrides. For example, the iOS backend needs to inject the https://pypi.anaconda.org/beeware/simple repo to ensure that the stop-gap iOS binary wheels are added; likewise, an end-user might want to add their own custom repos to contain iOS wheels.

However, my inclination is that we're likely OK on both those counts. The iOS backend already defines the extra arguments as part of extra_pip_args. If/when we add a conda backend, then I imagine we'd add a extra_conda_args (or abstract the API so that an analogous API entry point existed). From the perspective of app configuration, the general trend should be towards PEP 621 settings, and away from backend-specific configuration for anything "generic" - and I think pip install flags fall in that bucket. It's ultimately not a problem if you include an "iOS wheel repository" as a candidate source when you're installing for Linux - pip just won't find any wheel matches there.

@mhsmith
Copy link
Member

mhsmith commented Nov 6, 2024

rather than trying to abstract these package installer options into a few generic names, and considering that support for different installers is on the roadmap, what if briefcase instead added support for explicit configuration of those installers' options?

I agree with this approach, but I do have one suggestion:

[tool.briefcase.pip_options]
index_url = ""  # maps explicitly to the option of the same name in pip, no ambiguity, no abstraction
extra_index_urls = []  # ditto!
find_links = ""  # ditto!

I think mapping the options to a structured TOML representation is more trouble than it's worth, and probably can't even be done unambiguously:

  • The example suggests that extra_index_urls would map to multiple --extra-index-url options, so there would have to be some magic around the letter "s". But what if there was an option which took multiple arguments, e.g. --option arg1 arg2?
  • Some options are flags with no arguments, which would presumably represented in TOML as a boolean. But if the user passes False, does that mean to omit the argument, or to pass a --no-whatever argument?

I suggest we avoid all this complexity by taking the options as a flat list of strings, as Chaquopy does. For example:

requirement_installer = "pip"
requirement_options = ["--extra-index-url", "https://whatever", "--no-deps"]

it strikes me that the installer options might be best suited on AppConfig rather than on the global config so that they could be extended/overridden by platform if needed.

Yes, this will definitely be needed. If the options appear in multiple levels of the config, I suggest concatenating them with the less-specific sections coming first, as we already do with the requires and sources settings.

@freakboy3742
Copy link
Member

I think mapping the options to a structured TOML representation is more trouble than it's worth, and probably can't even be done unambiguously:
...
I suggest we avoid all this complexity by taking the options as a flat list of strings, as Chaquopy does. For example:

requirement_installer = "pip"
requirement_options = ["--extra-index-url", "https://whatever", "--no-deps"]

Those are all reasonable points. Passing through arguments as literals has the added benefit of being the least possible implementation effort (it's a single new list-of-strings setting), and the least prone to error on our part. It also means that if pip ever adds a new option, end-users can add support for that option without any interaction from us. Exposing flags like --use-pep517, --use-feature and --use-deprecated by default could be especially useful.

My one concern/downside is that it doesn't provide any protection to the end user - so if a user gets creative and decides to add ["--plat", "manylinux_2_28_aarch64"] to their iOS builds (or any other non-sensical/contradictory setting), they're not going to have a good time, and Briefcase will be limited in the assistance it can provide. However, pip_options is definitely getting into expert mode territory, so there's an aspect of "Doctor, it hurts when I hold my arm like this", "well don't hold your arm like that" that I'm willing to live with if you're extensively tweaking pip settings.

it strikes me that the installer options might be best suited on AppConfig rather than on the global config so that they could be extended/overridden by platform if needed.

Yes, this will definitely be needed. If the options appear in multiple levels of the config, I suggest concatenating them with the less-specific sections coming first, as we already do with the requires and sources settings.

Agreed. I think the use case is fairly narrow - there aren't many options that you would need to enable on a per-platform basis. The best I can think of would be enabling a specific index URL - but those should be able co-exist globally based purely on platform specifiers. However, I can't rule out that eccentricities will exist, so it might as well be cumulative. The good news is that by making it a single requirement_options list setting means it will fit directly into the existing cumulative settings handling without needing any special handling.

@sarayourfriend
Copy link
Contributor

Point taken regarding a set list of options being ambiguous viz booleans, and potentially limited depending on the precise level of flexibility users wish to have moving forward. I agree that an option for users to pass whatever arguments they like seems like a good idea.

However, there are still some edge cases to sort out. First, the path references issue I remarked on before potentially becomes a bit hairier if explicit options aren't defined (even if support for them is pushed off), but I think I have a solution that is better than anything we've discussed so far, would require minimal code changes to support, and would work for all build targets. Second, pip install arguments are used both as arguments to the actual command, and as content for a requirements.txt file, so they need to be easily turned into either... but, requirements files only support a single argument per line, so the precise format of the configuration option could be a bit tricky.

Path references

Explicitly named options (as opposed to only a list of ambiguous argument strings) makes it possible for Briefcase to transform values as needed to match the requirements of a given build target's context. For example, local paths used for --find-links or --extra-index-url need to at minimum be transformed into absolute paths so they can be referenced from whatever context pip install runs. For example, Flatpak builder runs pip install in the flatpak-builder context. It has host filesystem access, so an absolute path works fine (in other words, it isn't necessary to copy the files). Docker-based builds run pip install inside the container, where even an absolute path isn't sufficient, and yet more specific handling would be needed to make those local files available. By whatever means Docker-based builds end up supporting this is mostly immaterial, but the fact that Briefcase would need to be able to identify relevant paths can already be seen.

Relying solely on a simple list of string arguments to pass to pip install will make any attempt to support easy to understand path references for arguments that accept them overly complex. Briefcase would need to inspect the pip arguments list and try to identify which ones need transforming based on the name of the argument and the value. Inspecting the value is required either way, because only values that are paths need transforming in any case.

Keep in mind that BeeWare already relies on relative paths being transformed to absolute ones in this configuration. Support for it is absolutely required for this to work (unless macOS builds happen at the project's root directory, I don't have a macOS device to test it, but based on my reading of the code, that is not the case, and an absolute path transformation is what makes this work).

A sort of workaround exists using sources (and cleanup_paths), which involves configuring the pip install arguments with paths to the location based on the app_path for each build target. For Docker, it looks something like this:

pip_install_arguments = ["--extra-index-url=helloworld-0.0.1/usr/lib/helloworld/app/packages"]

It's a workaround, and maybe it falls under the arm-hurting analogy, but it's pretty ugly. The path just changes to match whatever app_path is defined as for the build target. And it only works if sources are copied into the bundle before pip install is called, which isn't true for Flatpak (and which may be a bug in the Flatpak template?).

Because if you use sources, it will always be based on app_path, one option is to expose an app_path template variable of the sort that cleanup_paths has access to. In that case, the pip arguments can be unified for all targets like this:

pip_install_arguments = ["--extra-index-url={app_path}/packages"]

As far I as I can tell, that would work for everything except Flatpak, but should be fixable in that target's template.

Other options I can think of that I like a lot less involve defining entirely new configuration options to facilitate easily identifying directories to copy to locations relative to where pip install is invoked.

The most flexible and least complicated solution to me is adding the template variable for the new pip install arguments option and explicitly documenting the approach using sources and cleanup_paths (combined with updating any build target templates that pip install requirements before sources are available).

Use of pip install arguments in requirements file

Rather than running pip install immediately, sometimes install_app_requirements just writes a requirements.txt file to be used in a pip install invocation later in the build. Flatpak does this, for example, as the actual pip install command is defined in manifest.yml. The reason this workaround works in any build context including Flatpak and why it requires using the = separator rather than a space is because everything in requires gets written to the requirements.txt that is ultimately installed passed to pip install -r.

In my original PR, I stuck with this approach and wrote the new arguments into the requirements file. Requirements files support pip install arguments, but strictly only one per line, and only if the name of the argument and any values are on the same line. That presents something of a developer experience issue if the pip install arguments configuration value is used in this way. The documentation would need to carefully explain that arguments must be provided as a single string of the name and value in such a way that works both as a line in a requirements file and when passed to the pip install command directly. So "--use-feature=XYZ", "-rfile.txt" but not "--use-feature XYZ", "-r file.txt" or "--use-feature", "XYZ", "-r", "file.txt". Only the first version works both as command arguments and as individual lines of a requirements file.

As I've thought about this more and more (I've been writing and re-writing and deleting and re-writing this comment for about 3 hours as I test things out and try different ideas), I think it's a mistake to keep using the requirements.txt in this way. It presents far too much complexity. However, I'm not confident of the feasibility of changing the way that Flatpak (for example) executes pip install so that it could accept arbitrary arguments based on the app configuration at build time. I don't think the templates quite work that way, but I could be wrong.

If it's easy to avoid putting those extra arguments into the requirements file when it is written, I'd really prefer an approach that used that, because it creates better ergonomics for the generic list of string arguments.


Okay, whew.

Would love to know what y'all think on these two counts:

  • Is the approach using template variables along with documenting sources and cleanup_paths along with one or more bug fixes to build target templates acceptable for solving the relative paths issue in pip install arguments?
  • Can y'all think of a way that works to avoid pushing extra pip install arguments into a requirements file so that users do not need to worry about fiddly details of how to define the arguments? The end result must instead be that the pip install argument is itself directly configurable for a build target even if that target defers running pip install and if an external tool needs to write the invocation (i.e., it can't use app_context.run).

@sarayourfriend sarayourfriend linked a pull request Nov 7, 2024 that will close this issue
4 tasks
@mhsmith
Copy link
Member

mhsmith commented Nov 7, 2024

Path references

Have we established exactly what forms of local paths are accepted by --find-links and --extra-index-url? The pip documentation about this has always been vague. Specifically, do they accept bare paths, or only file URLs, and in the case of URLs, do they need to be absolute?

A sort of workaround exists using sources (and cleanup_paths)

That doesn't seem very intuitive, to use sources for things which aren't actually sources and which you have no intention of keeping at runtime.

As you've noticed, we already allow requires entries which point at arbitrary local paths, by transforming them to absolute paths when writing the requirements file. The code for this is in _write_requirements_file in commands/create.py, and it detects local paths by looking for items that contain a slash but are not URLs. Could we do the same with custom pip arguments which look like local paths, maybe with an extra check that the path actually exists?

If it's easy to avoid putting those extra arguments into the requirements file when it is written, I'd really prefer an approach that used that, because it creates better ergonomics for the generic list of string arguments.

I agree. I think requirements files are only used by two platforms:

  • Android – Chaquopy has a pip options setting which I linked in my previous comment. This would need to be added to the template here.
  • Flatpak – Although we run pip within a container, we still have direct control over its command line, so we can add whatever we want.

@mhsmith
Copy link
Member

mhsmith commented Nov 7, 2024

As for the name of the option, the reason I suggested requirement_options was to indicate its connection with requires and requirement_installer, but I haven't thought through all the alternatives.

@freakboy3742
Copy link
Member

However, there are still some edge cases to sort out.

Good catch on both of these. We definitely need to be careful in how we modify/transform any arguments to make sure we don't introduce a new class of problems - and we definitely don't want to get into the game of trying to parse and re-interpret pip's own arguments so we can work out how to "pair" them.

However, I don't think it's necessarily as complex as you might think.

Keep in mind that BeeWare already relies on relative paths being transformed to absolute ones in this configuration. Support for it is absolutely required for this to work (unless macOS builds happen at the project's root directory, I don't have a macOS device to test it, but based on my reading of the code, that is not the case, and an absolute path transformation is what makes this work).

This isn't quite right. When Briefcase is invoking pip directly, it's always doing so in a working directory that is the project directory (i.e., the folder containing pyproject.toml), so relative references in pyproject.toml resolve to the same location in practice (See the app_context.run call in create._pip_install(); or the -vv logged output for a create call).

The three exceptions to this are:

  1. Android builds. Briefcase writes a requirements.txt file, but converts any file reference in the list of requirements into an absolute path. Chaquopy then manages the actual install, but it's running on the local machine, so any absolute paths resolve without a problem.
  2. Flatpak builds. Again, Briefcase writes a requirements.txt file with absolute file paths; the running Flatpak container has access to the host filesystem, so in practice, any "small scale" relative path that the user is likely to be using can be resolved at build time.
  3. Docker builds. This is a literal pip execution, but inside the container. The Docker container mounts the project directory (build/myapp/ubuntu/22.04) and the Briefcase cache folder. Any file being referenced needs to be in one of those locations, so any concept of "relative to the project directory" is going to be a bit wonky.

Case 1 and 2 aren't a problem for relative paths - we know that any value is either a package name, a path, or a URL. There's a _is_local_requirement() check that manages this for us, on the basis that / and \ aren't legal in package names, so if they exist in a requirement, they must be either a local file or a URL.

The complication for arbitrary arguments is that it's a little harder to say that any given argument is a filesystem path - but if we say "contains a / and the location is an existent file or directory", that should be safe enough (I think?)

Case 3 is a lot harder to manage, because it requires adding a new filesystem mount and rewriting any filesystem path. We currently manage this by looking for any filesystem location in the requirements, and building an sdist locally, putting that into the Docker container, and then installing the sdist. That approach obviously won't work for --index-url or --find-links; however, as noted previously, (1) I'd be ok with that being a known limitation for v1, and (2) there's a future world where we add a Docker filesystem mount for any file system location that is referenced and re-write file references relative to that mount.

Would love to know what y'all think on these two counts:

  • Is the approach using template variables along with documenting sources and cleanup_paths along with one or more bug fixes to build target templates acceptable for solving the relative paths issue in pip install arguments?

I don't have any fundamental objection to making these arguments templated... but I'm not sure I understand the use case. What case are you envisaging where a user will need to reference a location inside the transient build folder as a source of packages?

Also - the cleanup_paths use of templating is documented AFAICT; but I don't believe sources is templated - nor should it be... as with --index-url and --find-links, I'm not sure I see what the use case is for this. Can you elaborate on what you mean about the bugs in these two?

  • Can y'all think of a way that works to avoid pushing extra pip install arguments into a requirements file so that users do not need to worry about fiddly details of how to define the arguments? The end result must instead be that the pip install argument is itself directly configurable for a build target even if that target defers running pip install and if an external tool needs to write the invocation (i.e., it can't use app_context.run).

Is there any reason that we can't pass them in as arguments to pip install - even in the requirements.txt case? pip -r requirements.txt where the requirements file contains --extra-index-url https://example.com should be functionally the same as pip --extra-index-url https://example.com -r requirements.txt.

In the case for Flatpak, the invocation of pip is part of the flatpak manifest, which is templated - so we should be able to include any extra pip arguments in that template. The one downside is that the templated value is only generated during the create call - but we could address that by writing a pip invocation into a script every time requirements are updated, and then invoking that script as part of the flatpak manifest. That might even make the transition to other installers easier, because "install-requirements.sh" becomes something that is generated externally to the base template.

The same is true of Android - Chaquopy allows you to pass in the pip invocation. The "write a script" approach won't work here, but once pip supports android platform tags natively, maybe we can look to moving the pip install out of the chaquopy build system so we can do a more conventional Briefcase-managed pip install.

@mhsmith
Copy link
Member

mhsmith commented Nov 10, 2024

Flatpak builds. Again, Briefcase writes a requirements.txt file with absolute file paths; the running Flatpak container has access to the host filesystem,

Specifically, it has access to /home, which we can reasonably assume most of the relevant local paths are within.

The complication for arbitrary arguments is that it's a little harder to say that any given argument is a filesystem path - but if we say "contains a / and the location is an existent file or directory", that should be safe enough (I think?)

I think so. We would need to document that local paths in pip arguments need to contain a slash (e.g. ./subdir rather than subdir) , but that's not too much of a burden.

What case are you envisaging where a user will need to reference a location inside the transient build folder as a source of packages?

If I understand correctly, it was just a workaround for the difficulty of using relative paths in the pip arguments, but it looks like converting them to absolute paths automatically is a better solution, because it doesn't require the user to do anything special.

The same is true of Android - Chaquopy allows you to pass in the pip invocation

The -r syntax in that link is a special case – -r is the only flag accepted by the install method. Apart from requirements and requirement filenames, all other pip install arguments must be passed to the options method I linked above.

The reason for this is that Chaquopy runs pip once for each ABI. So it needs to distinguish which arguments are options, which get passed to every pip invocation, and which ones are requirements sources. For the first ABI, it installs all requirements, and then for the remaining ABIs it installs native requirements only.

@sarayourfriend
Copy link
Contributor

Apologies for the tardy response, been a busy weekend.

@freakboy3742 @rmartin16, please confirm if this summary matches your desired path forward:

  • Add a list-of-string option to app, which is cumulative (so different targets can add to it)
  • The option should have a generic name which we can bike shed the specifics of in the PR, but something indicating it contains arguments for the requirement installer
  • Any string in the option that starts with ./ and exists on the filesystem when evaluated relative to base_path should be converted to an absolute path
    • Outstanding question: should Briefcase support transformations for these arguments in all the forms "--find-links", "./local-path", "--find-links=./local-path", and "-f./local-path"? If so, what mechanism should be used to avoid needing to recreate pip install's argument parser (at minimum tracking all pip install arguments that could possibly have a path argument)? = is not that problematic, arg.split("=", maxsplit=1)[-1] could work in that case to get the value, but the short-form is more troublesome, especially if it's
  • To support an easy API for the option for users and the Briefcase code, do not write options to requirements.txt (thereby avoiding issues of arguments on a single line); instead, update the Flatpak and Android templates to render the pip invocation options into manifest.yml or build.gradle respectively
    • Outstanding question: should template re-processing happen whenever -r briefcase would have updated requirements as part of briefcase build? To clarify, my understanding based on @rmartin16's clarification about Android is that something like an install-requirements.sh script would not work. Briefcase needs to put the pip options as an options invocation in the pip block of build.gradle. My original suggestion was based on working around the fact that template processing happens only once. If Briefcase starts doing it every time requirements would update, that would make this possible? I don't know how that affects successfully setting up future installer backends (maybe it would require adding logic in the templates for each backend supported by the platform, it might end up being target specific anyway?).

If that is accurate, then I think the right order of PRs to support this would be:

  1. Add template re-processing whenever requirements would update as part of briefcase build
  2. Update the Flatpak and Android templates to support pip arguments.
  3. Add the new option to Briefcase

This order ensures the option isn't available until the templates can actually use it. I'm making an assumption that this is important, particularly for Android which I suspect is a popular target, but it's just an assumption. If it doesn't need to be in this order, then I can group the first and last change into a single changeset if desired... though separate PRs might be easier to review and work with. Let me know.


Just want to clarify a few things which shouldn't have any bearing on the above, but I feel were misunderstood in my previous comment. I've put them in a details/summary block to prevent distracting too much, but I did want to make sure I wasn't being misunderstood or causing confusion.

Regarding automatic transformation of paths in the arguments string to absolute paths: my concern here is solved if Briefcase only transforms arguments that start with ./ and are therefore “unambiguously” (maybe arguable) paths. Otherwise, even if checking to make sure the value resolves to a real local path, you could end up transforming a coincidentally named pip feature flag into an absolute path, likely leading to very confusing behaviour. It seems a rare edge case, but not even that far-fetched.

Regarding template variables in cleanup_paths: it is documented, I did not mean to imply it isn't documented. The idea to use templating for the requirement installer arguments came from reading the configuration documentation and seeing that cleanup_paths had it (i.e., a precedence exists).

That doesn't seem very intuitive, to use sources for things which aren't actually sources and which you have no intention of keeping at runtime.

The existence of cleanup_paths seems to imply that some things copied into the bundle as part of sources or potentially generated as a side effect of the build (?) won't be needed. It's rather common to have build-only dependencies from which the final build output are derived. You could argue the fact that Python source code files are runtime dependencies is a side effect of CPython implementation details (generating pyc on the fly). C source code files used to build a runtime dependency the Python code links to are certainly "sources" but not strictly needed in the final bundle. I imagine that's also the case for Java files from which JVM bytecode or IR is derived during the build. They are sources though. A vendored or in-house dependency might be too, in that case. Wouldn't that be true for sources the build ultimately outputs to WASM or even pyc files (if startup optimisation was needed)?

This is an extremely minor point, but I fear I've misunderstood something about the intention of sources + cleanup_paths if it isn't for this kind of thing.

Can you elaborate on what you mean about the bugs in these two?

I suggested there may be a bug in the Flatpak template, because it does not copy sources before running pip install, whereas sources are available before pip install for other targets. Bug wasn't the right word, I should have said, "difference" or "(overall) inconsistency". I don't know what the intended behaviour is, or even if there is one, so I'm wrong to call it a bug. Apologies for the confusion.

Is there any reason that we can't pass them in as arguments to pip install - even in the requirements.txt case? pip -r requirements.txt where the requirements file contains --extra-index-url https://example.com should be functionally the same as pip --extra-index-url https://example.com -r requirements.txt.

Yes, there is. I had a detailed explanation of this in my comment originally, but I seem to have left it out when I re-wrote it after further exploration. I apologise that it was unclear. The requirements file only allows a single pip install option per line. If multiple options are present on a line then it will only interpret the last one.

Given this file, --extra-index-url will be completely ignored.

--extra-index-url=./packages -f wheels
myhttpx

It is fixed by splitting the first line into two, one for each argument.

If the option is supplied as a flat list of strings, it becomes difficult to know for sure how to interpret the input such that the requirements file can be written with only a single argument and value per line.

Given the following example I cannot think of an easy way to split it into arguments per-line:

args = ["-f", "wheels", "--no-clean", "--extra-index-url=https://example.com", "--ignore-installed", "--index-url", "https://pypi.org/simple"]

This is a valid list of pip install arguments, but splitting it into the lines of a requirements file is non-trivial. The file must have the following:

-f wheels
--no-clean
--extra-index-url=https://example.com
--ignore-installed
--index-url https://pypi.org/simple

You cannot merely group every 2 items, or else --index-url gets split across two lines, and --no-clean and --extra-index-url get put on the same line. You could try to implement some heuristics to guess correctly (put a new line before anything that starts with -), but it will ultimately be flaky and non-resilient if the intention is to support arbitrary user input. Worse, the failure is obscure, pip install emits no warning that it is ignoring an argument from the requirements file.

Furthermore, taking this specific example, --ignore-installed and --no-clean are not supported in the requirements file, i.e., pip does not support any arbitrary arguments in the requirements file. That may also be obscure, but it's at the very least inconsistent behaviour between targets (those that do and do not use a requirements file rather than directly modifying the pip install arguments), currently with no clear indication given to the user of that difference.

@freakboy3742
Copy link
Member

freakboy3742 commented Nov 11, 2024

Apologies for the tardy response, been a busy weekend.

No problems at all - we'll take whatever contributions we can get, on whatever timeline your schedule allows.

@freakboy3742 @rmartin16,

I think you probably meant @mhsmith here...

please confirm if this summary matches your desired path forward:

  • Add a list-of-string option to app, which is cumulative (so different targets can add to it)

👍

  • The option should have a generic name which we can bike shed the specifics of in the PR, but something indicating it contains arguments for the requirement installer
  • Any string in the option that starts with ./ and exists on the filesystem when evaluated relative to base_path should be converted to an absolute path

I don't think we can be as specific as "starts with ./" - after all ../ would also be a reasonable value (albeit one that requires a very specific directory structure that is potentially outside the scope of a single GitHub checkout).

I'd be more inclined to use a 3 part check:

  1. contains a / or \ character
  2. isn't a URL (i.e., doesn't match r.*?://)
  3. Path(value).resolve().exists()

Part (1) and (2) of that check are already implemented as part of is_local_requirement().

  • Outstanding question: should Briefcase support transformations for these arguments in all the forms "--find-links", "./local-path", "--find-links=./local-path", and "-f./local-path"? If so, what mechanism should be used to avoid needing to recreate pip install's argument parser (at minimum tracking all pip install arguments that could possibly have a path argument)? = is not that problematic, arg.split("=", maxsplit=1)[-1] could work in that case to get the value, but the short-form is more troublesome, especially if it's

That's a good catch... I hadn't thought of that one.

I can think of 2 possible approaches:

  1. We add an "argument parsing" parsing component to the 3-part test I mentioned above (looking for -f<path> and the "split on =" format). This will find paths however they're defined, but requires more complex (and potentially error prone) arg-parsing logic; but it would be consistent everywhere.
  2. We use this discrepancy to our advantage. The 3-part check I mentioned earlier is inherently prone to edge cases, because we can't know with absolute certainty if an argument that "looks file like" is, in fact, a file or directory. So - we document this as the behavior of the arg parsing: If the argument "looks like a file" (using the test above) and it is passed in as a standalone argument (i.e., (["--find-links", "../wheels"]), it will be treated as a file, and resolved. However, if it's passed as part of a single argument (i.e., ["-f../wheels"] or ["--find-links=../wheels"]), it won't be processed. This might require adding an additional check of "argument doesn't start with -" to avoid -f../wheels being treated as a file name itself (which... technically, it is).

Given the inherent ambiguity associated with interpreting arguments, I'd argue keeping an escape hatch available to allow for a "file like" argument to not be treated as a file is probably worthwhile - but I'm open to be convinced I'm wrong on this one.

  • To support an easy API for the option for users and the Briefcase code, do not write options to requirements.txt (thereby avoiding issues of arguments on a single line); instead, update the Flatpak and Android templates to render the pip invocation options into manifest.yml or build.gradle respectively

👍

  • Outstanding question: should template re-processing happen whenever -r briefcase would have updated requirements as part of briefcase build?

This is a general problem that Briefcase has at present - see #472. There are a range of features that we can't currently update after initial project generation.

In this case, it might be worth exploring whether we can factor the pip arguments out into a standalone gradle file and "import" that file into the top-level one. I'm not especially familiar with Gradle, but a quick search seems to indicate there are a few options for re-using Gradle logic in a way that will be compatible with Briefcase regenerating the part of the file that is imported, without regenerating the "host" file that does the importing.

If that is accurate, then I think the right order of PRs to support this would be:

  1. Add template re-processing whenever requirements would update as part of briefcase build
  2. Update the Flatpak and Android templates to support pip arguments.
  3. Add the new option to Briefcase

That sounds like a good path forward to me.

@freakboy3742
Copy link
Member

(apologies - I somehow managed to hit a magic key combination that submitted my comment and closed the issue... currently editing my response)...

@freakboy3742
Copy link
Member

(Responding to the rest in a standalone comment because of the "hit close accidentally" debacle..)

Regarding automatic transformation of paths in the arguments string to absolute paths: my concern here is solved if Briefcase only transforms arguments that start with ./ and are therefore “unambiguously” (maybe arguable) paths.

Completely agreed there's an ambiguity here. However, even "starts with ./" doesn't completely avoids the ambiguity. It definitely reduces the potential error surface, but it is still technically possible to provide an argument that does match a local filename but shouldn't be treated as a file.

However, as I noted in my previous comment - as long as we've got a way to explicitly say "don't interpret as a file", I think we can call that a win - and the ambiguity over -f./file and -f ./file possibly gives us a way to do that.

That doesn't seem very intuitive, to use sources for things which aren't actually sources and which you have no intention of keeping at runtime.

This is an extremely minor point, but I fear I've misunderstood something about the intention of sources + cleanup_paths if it isn't for this kind of thing.

I think you've correctly understood the general relationship between sources and cleanup_paths - but you're pushing the interpretation a little further than intended with the "workaround" you proposed.

Although the name of the option is sources, it's meant to be for Python sources to be used at runtime, not a generic mechanism for any "source code inputs" to the build process. Everything that is copied as part of sources ends up in a location that is on the running app's PYTHONPATH, and the general expectation is that every file that is added by sources will be imported (or loaded as data) at runtime. There will be some exceptions (such as cleaning up .pyc and other build artefact files), but as a general rule - if you copy it as sources, you want it at runtime.

Your usage of sources to copy in a wheel file so it is in a location that can be seen by an in-container pip invocation and then deleted by cleanup_paths is definitely creative - but it's definitely pushing the intention of sources much further than intended; and I agree with @mhsmith that even if it is technically possible, it's not something that we really want to be relying on.

Can you elaborate on what you mean about the bugs in these two?

I suggested there may be a bug in the Flatpak template, because it does not copy sources before running pip install, whereas sources are available before pip install for other targets. Bug wasn't the right word, I should have said, "difference" or "(overall) inconsistency". I don't know what the intended behaviour is, or even if there is one, so I'm wrong to call it a bug. Apologies for the confusion.

Right - that makes sense. FWIW, I think that's in the territory of "not exactly a bug, but also not an intended use case either". As with my previous comment, the intention isn't that pip should be able to reference anything described in sources; the two are dealing with two very separate sets of inputs.

Is there any reason that we can't pass them in as arguments to pip install - even in the requirements.txt case? pip -r requirements.txt where the requirements file contains --extra-index-url https://example.com should be functionally the same as pip --extra-index-url https://example.com -r requirements.txt.

Yes, there is. I had a detailed explanation of this in my comment originally, but I seem to have left it out when I re-wrote it after further exploration. I apologise that it was unclear. The requirements file only allows a single pip install option per line. If multiple options are present on a line then it will only interpret the last one.

My apologies - I think we're talking at crossed purposes here. I completely agree with what you've written here - I was advocating for not putting these arguments into requirements.txt at all, and modifying the flatpak manifest script so that the invocation of pip install uses the arguments as part of the command line, rather picking them up as part of the requirements.txt file. That way we avoid the "how to split the arguments" problem, and the question of which arguments are legal in requirements.txt.

@mhsmith
Copy link
Member

mhsmith commented Nov 11, 2024

even if checking to make sure the value resolves to a real local path, you could end up transforming a coincidentally named pip feature flag into an absolute path, likely leading to very confusing behaviour

It seems unlikely that a feature flag would contain a slash, and if it did, I think @freakboy3742's comments above provide a good way of stopping it (or any other argument) from being interpreted as a filename.

it might be worth exploring whether we can factor the pip arguments out into a standalone gradle file and "import" that file into the top-level one

Gradle files can contain arbitrary Groovy code, so we could make Briefcase write the pip options to a simple text file, one option per line, and put some code in the template's build.gradle to read the options from the file and pass them to the pip { options } method. This options file would be generated at the same time as the requirements.txt, but since it would be read directly by the build.gradle file on every build, it wouldn't need a timestamp comment header to trigger a rerun of pip when it changes.

Can we deal with Flatpak in a similar way? If so, we can avoid doing any "template re-processing whenever requirements would update".

@freakboy3742
Copy link
Member

Can we deal with Flatpak in a similar way? If so, we can avoid doing any "template re-processing whenever requirements would update".

Yes, that's the core of the install_requirements.sh idea mentioned previously above. Instead of having pip install -r requirements.txt literally in the flatpak manifest file, we make the install step install_requirements.sh as a script to be executed, and re-write install_requirements.sh to include the command line invocation (with all the current arguments we need). There's likely some other ways to slice the problem using $(cat installer_arguments.txt) or similar command line expansions.

@sarayourfriend
Copy link
Contributor

Awesome, thanks for the clarifications all around, and apologies for the mixed up ping @mhsmith.

For writing a file like install_requirements.sh or pipOptions.groovy, the logic for writing that file would be part of Briefcase, but then applying/using that file would be in the template. Is that correct? Would some version awareness need to be implemented to make sure the template is capable of using the file, and if not, alert the user of that fact? I found this issue about template versioning, but it looks like the resolution was to wait and see. Is that still the case? In this situation, it's additive. Is it safe to always write the new file, and leave it up to the template itself whether that file is used?

@freakboy3742
Copy link
Member

For writing a file like install_requirements.sh or pipOptions.groovy, the logic for writing that file would be part of Briefcase, but then applying/using that file would be in the template. Is that correct?

Correct - although the template might output an "empty" or "default" version of the file that Briefcase will overwrite so that the "empty template" will continue to work as expected. This will be particular important during the transition period where the template supports the new feature, but Briefcase doesn't (yet).

Would some version awareness need to be implemented to make sure the template is capable of using the file, and if not, alert the user of that fact? I found this issue about template versioning, but it looks like the resolution was to wait and see. Is that still the case? In this situation, it's additive. Is it safe to always write the new file, and leave it up to the template itself whether that file is used?

AFAICT, it should be safe to write the file. If you've got a project generated with Briefcase 0.3.20, and you use Briefcase 0.3.21 to build, then it will write the file, but the project won't pick up the new option. That's not ideal, but it's not entirely surprising as an outcome.

FWIW, we do have the ability to define a template epoch - for example, Briefcase 0.3.15 introduced a backwards incompatible change for Android templates, so the template contains this marker, and the Android backend contains the corresponding marker. If you had a project generated with 0.3.14 or earlier, it would raise an error if you tried to build the project with 0.3.15. However, we only really use this capability when we make a major change that is fundamentally incompatible, rather than something like this where and existing project will still be compatible, it just won't be able to use features from the new release.

@mhsmith
Copy link
Member

mhsmith commented Nov 12, 2024

I found this issue about template versioning, but it looks like the resolution was to wait and see. Is that still the case?

No, that issue was implemented pretty much as discussed. For example. you can see that the Android template repository has one branch for each Briefcase version released in the last 2 years.

So there's no risk of an old Briefcase version using a new template. And the template main branch is only used by the Briefcase development version, not any released version.

But if the user runs briefcase create and then upgrades Briefcase, the new Briefcase version will see the output of an old template. If we know this combination will be incompatible, we can use the "epoch" mechanism Russell mentioned, which will cause Briefcase to give an error and instruct the user to run briefcase create again.

@sarayourfriend
Copy link
Contributor

Alright, thanks for clarifying that. Sounds like it should be fine to not worry about it in this case 🙂

To amend my earlier list of tasks for this, template re-processing isn't necessary, so I'll work on PRs to the Flatpak and Android templates to add the new files. I'll probably start with the Flatpak one.

sarayourfriend added a commit to sarayourfriend/briefcase-linux-flatpak-template that referenced this issue Nov 14, 2024
re: beeware/briefcase#1270

Facilitates configuration of the pip install command arguments by Briefcase or the use of a different package installer altogether without needing to re-process the template or add complex logic to the template (as would be required if the command was templated directly into the manifest).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features, or improvements to existing features. good first issue Is this your first time contributing? This could be a good place to start!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants