search

beeware / briefcase

Tools to support converting a Python project into a standalone native application.
https://briefcase.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
2.66k stars 372 forks source link

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

Open mocaccano opened 1 year ago

mocaccano commented 1 year ago

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

mhsmith commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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",
 ]
wyrd-0 commented 1 year ago

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 commented 1 year ago

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

freakboy3742 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 6 months ago

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 commented 2 months ago

Hello friends, I am creating an Android mobile application with Beeware for personal use, the application works well locally on my Windows system but when I launch the Android interface it refuses ... 'is to do speech-to-translate I use the module how googletrans pygamus. I would like to have help

rmartin16 commented 2 months ago

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

sarayourfriend commented 1 week ago

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 :sweat_smile:.

freakboy3742 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 :slightly_smiling_face:. We can bike shed the option names in the PR :grinning:

sarayourfriend commented 5 days ago

I've got the PR up as a draft here: https://github.com/beeware/briefcase/pull/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 commented 5 days ago

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 commented 5 days ago

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 commented 5 days ago

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:

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 commented 4 days ago

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 commented 4 days ago

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:

mhsmith commented 3 days ago

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:

mhsmith commented 3 days ago

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 commented 3 days ago

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 commented 1 day ago

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 commented 19 hours ago

Apologies for the tardy response, been a busy weekend.

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

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](https://pip.pypa.io/en/stable/development/release-process/#use-feature) 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 commented 16 hours ago

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 commented 16 hours ago

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

freakboy3742 commented 15 hours ago

(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.