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

Add per-app pip install argument configuration #2059

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sarayourfriend
Copy link
Contributor

@sarayourfriend sarayourfriend commented Nov 7, 2024

Fixes #1270

This PR adds a new app config option requirement_installer_args, which is a list of strings, as discussed in the issue.

The goal is to allow users to configure pip install with any options they want. As discussed in the issue, arguments that appear to be local paths in format and resolve to an existing local path relative to the command's base path are transformed to absolute paths when calling pip install.

For most build targets, the arguments are added directly to the pip install invocation, alongside the requirements. Users have full control over the options they choose to pass.

For some build targets (Flatpak and Android), Briefcase hands off pip install invocation to a specialised build tool (Flatpak builder and Chaquo respectively). In these cases, Briefcase must supply the user-defined installer arguments in a way the build tool can reference. For both these cases, I've updated the templates (beeware/briefcase-linux-flatpak-template#53, beeware/briefcase-android-gradle-template#95) to process a pip-options.txt file defined in the path index as app_requirement_installer_args_path. When this path is defined, Briefcase will write a new-line separated list of the requirement installer args. It is up to the template to decide how to utilise this file. If no such path is defined, Briefcase will not write the installer args, but will still write the requirements.txt as it does today. This ensures seamless backwards compatibility with templates that do not define the new key in the path index.

The PR also changes the generated requirements.txt file to always write the timestamp comment, even if the file would end up empty otherwise.

PR Checklist:

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

@sarayourfriend sarayourfriend force-pushed the add/requirement-installer-config branch 2 times, most recently from e14375f to a737935 Compare November 14, 2024 05:37

pip_install_command = " ".join(
[
"/app/bin/python3",
Copy link
Member

Choose a reason for hiding this comment

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

Don't want to interject myself too much here....but potentially something to consider.

I've tried to be leery of coupling Briefcase too much to its first-party templates. For instance, here, we're assuming where Python will be available in the Flatpak; this will limit where a custom template can put Python without also re-implementing this packaging format for Briefcase. So (without having considered any potential complexity for this...), perhaps Python could be added to PATH in the Flatpak (or a similar mechanism) to prevent this coupling. freakboy3742, I'll ofc defer to you for any prioritization of this concern :)

p.s. - definitely appreciating your contributions :)

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that we should be avoiding coupling Briefcase to an "official" template wherever possible; but in this case, I think we're reasonable safe. /app/bin is where binaries will almost certainly be installed for virtual all practical Flatpak purposes; if that turns out to not be the case, we could add an extension point to the template that defines flatpak_python_exe = "/app/bin/python3" (similar to how we configure the location of the support package etc). However, that seems like a fairly niche requirement; and it's a fairly simple thing to add if and when the requirement emerges.

Copy link
Member

Choose a reason for hiding this comment

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

I've tried to be leery of coupling Briefcase too much to its first-party templates. For instance, here, we're assuming where Python will be available in the Flatpak

I suppose by this argument, we should also allow the template to control where the pip options are written, rather than assuming that install_requirements.sh is in the same directory as requirements.txt.

In fact, I have an idea which would solve both this and the Python executable path issue. In #1270 I suggested that the Android mechanism would be:

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

What if Flatpak worked in the same way?

  • Briefcase would use the same code for both Flatpak and Android.
  • This code would write only the options file, not a full shell script.
  • The location and name of the file would be controlled by a variable in the template itself, just like the requirements.txt file.
  • The details of how the file is used would be contained entirely within the template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhsmith so would the file to write have a generic name like the configuration option? I suppose I can see a version of this feature where the arguments are always written in the same format regardless of target (and therefore, it can be part of the base CreateCommand without private method overrides like I've done in this PR)... and then it's up to the template whether or how it uses those arguments.

On the other hand, keeping in mind that arbitrary requirement installer backends could be supported aside from pip (like conda, uv), writing the pip options to a file makes the requirement installer backend a template detail, rather than a Briefcase detail... but only for Flatpak and Android which run pip install themselves rather than other targets where Briefcase executes it.

Implementing another installer backend then gets split. For most targets, it would just be a change to Briefcase to support it. For targets that use the pip-options file, they would need to have logic in the template to use the correct installer backend, adding complexity to the template.

Of course for Flatpak more would need to be done to support some other installer backend in this case (i.e., providing conda/uv/whatever in the Flatpak environment) but that could be put into the install-requirements.sh script. Android seems like potentially the most restrictive because Chaquopy exposes a custom pip interface rather than allowing any old package installer command running? I'd worry that taking the approach necessary for Android for everything else potentially limits the possibilities for targets that don't have the same limitations.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that it would be desirable to use the same format for both Flatpak and Android (if only because the code can be shared); and that the other benefits you describe would also be useful.

My only hesitation is whether there's an easy way to manage this in the Flatpak template itself. I'm not certain that subshell invocations (like $(cat somefile.txt) ) are legal in a Flatpak manifest. Definitely happy to be proven wrong on this front, though.

Copy link
Contributor Author

@sarayourfriend sarayourfriend Nov 16, 2024

Choose a reason for hiding this comment

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

It's kind of ugly, but this works as a build command in Flatpak manifest to read the arguments from a newline delimited pip-options.txt:

      - readarray -t EXTRA_PIP_ARGS < pip-options.txt; CC="gcc -pthread" /app/bin/python3 -m pip install --no-cache-dir -r requirements.txt --target /app/briefcase/app_packages "${EXTRA_PIP_ARGS[@]}"

I don't really know if this is portable. $SHELL is reported as /bin/sh but on my Fedora installation that's just bash. I'm concerned this won't work if the build host doesn't use bash for sh and adding a dependency on it doesn't seem right.

I suppose this argument reading and command preparation could be written as a Python script executed with os.execve (something along those lines), removing the build host's shell environment as a factor altogether.

Copy link
Member

Choose a reason for hiding this comment

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

A python script sounds like a reasonable approach to me - we definitely don't have any guarantees that the end-user will be using bash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've opened a PR with a potential approach for using this options-file approach for Flatpak: beeware/briefcase-linux-flatpak-template#53

Copy link
Member

Choose a reason for hiding this comment

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

Implementing another installer backend then gets split. For most targets, it would just be a change to Briefcase to support it. For targets that use the pip-options file, they would need to have logic in the template to use the correct installer backend, adding complexity to the template.

On Flatpak, I think the main reason we run pip inside a container rather than directly from Briefcase is to support building binary packages from source. But that wouldn't be a factor with Conda packages, which are always binary.

So, although I haven't thought through the details, I suspect that when we add Conda support to Briefcase, we'll be running Conda directly from Briefcase on all desktop platforms including Flatpak. This will inevitably complicate the template, but I don't think anything in this PR will make it any worse.

Mobile platforms aren't currently supported by Conda at all, so we don't need to worry about that until much later, and maybe never.

docs/reference/configuration.rst Outdated Show resolved Hide resolved
src/briefcase/platforms/android/gradle.py Outdated Show resolved Hide resolved
Comment on lines 292 to 294
pip_options = "\n".join(
[f"# Generated {datetime.datetime.now()}"] + self._extra_pip_args(app)
)
Copy link
Member

Choose a reason for hiding this comment

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

Unlike the requirements file, the options file doesn't need a timestamp comment to trigger a re-run of pip when it changes, because it's read by Gradle on every build. And if we use this file format in Flatpak as I suggested above, then eliminating the comment would make it easier to process with a shell script.

Copy link
Member

Choose a reason for hiding this comment

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

It's not needed to trigger a re-run, but I'd argue it's helpful as a diagnostic tool. However, if it interferes with easy command line processing, then it's a "nice-to-have" we can definitely lose.

Comment on lines 674 to +690
try:
requirements_path = self.app_requirements_path(app)
self._write_requirements_file(app, requires, requirements_path)
except KeyError:
requirements_path = None

try:
requirement_installer_args_path = self.app_requirement_installer_args_path(
app
)
except KeyError:
requirement_installer_args_path = None

if requirements_path:
self._write_requirements_file(
app, requires, requirements_path, requirement_installer_args_path
)
else:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change in logic is to isolate the indication of whether the requirements files should be written to only if app_requirements_path is defined. To ensure backwards compatibility, app_requirement_installer_args_path needs to be optional. Existing template versions won't define it but should continue to write the requirements path.

@mhsmith
Copy link
Member

mhsmith commented Nov 18, 2024

For future reference, force pushing is fine if it's to clean up something that other people probably haven't seen yet. But please don't use a force push to alter or remove commits which have already been commented on, as it makes the conversation difficult to follow.

@sarayourfriend
Copy link
Contributor Author

sarayourfriend commented Nov 19, 2024

Sure, no problem, I will avoid it in the future. I chose to force push here because I noticed PRs are not squashed, and I wanted to avoid introducing commits on main that didn't reflect the desired implementation. Apologies. Maybe this could have been better approached as a separate PR, considering the degree of change in the approach (though it's also why I've left this as a draft for now).

In any case, I'll avoid it in this repository from now on.

@sarayourfriend sarayourfriend marked this pull request as ready for review November 20, 2024 22:33
@sarayourfriend
Copy link
Contributor Author

I've updated the PR description to reflect the current implementation. Looking forward to reviews when y'all have a chance to revisit this 🙂.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Install a python requirement with an --extra-index-url argument
4 participants