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

Fix Via's requirements files #946

Merged
merged 1 commit into from
May 30, 2023
Merged

Fix Via's requirements files #946

merged 1 commit into from
May 30, 2023

Conversation

seanh
Copy link
Contributor

@seanh seanh commented May 30, 2023

This is the result of running make --always-make requirements on Linux. This fixes a couple of issues that were added to the requirements files by #905:

  1. Add frontend infrastructure for video player app #905 added a macOS-specific dependency (appnope) to Via's dev.txt.

    This happened because the requirements files had been compiled on macOS for Add frontend infrastructure for video player app #905. Requirements files have to be compiled on Linux!

  2. Add frontend infrastructure for video player app #905 removed the --allow-unsafe argument from the pip-compile command. This option should be used, see: https://github.com/jazzband/pip-tools#deprecations

This is the result of running `make --always-make requirements` on
Linux. This fixes a couple of issues that were added to the requirements
files by #905:

1. #905 added a macOS-specific
   dependency ([appnope](https://pypi.org/project/appnope/)) to Via's
   `dev.txt`.

   This happened because the requirements files had been compiled on
   macOS for #905. Requirements
   files have to be compiled on Linux!

2. #905 removed the
   `--allow-unsafe` argument from the `pip-compile` command. This option
   should be used, see: https://github.com/jazzband/pip-tools#deprecations
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

LGTM. I thought I ran make requirements in 9eb4527 to update the requirements files, but indeed it seems that --allow-unsafe was not applied. If I run the following locally I can reproduce the change in this PR, except for the macOS-specific issue:

touch requirements/*.in
make requirements

@robertknight
Copy link
Member

Requirements files have to be compiled on Linux!

This is really unfortunate. We ought to have a documented way for developers using macOS systems to avoid this hazard. A solution that uses Docker would be OK if we can't find an easier alternative.

@seanh
Copy link
Contributor Author

seanh commented May 30, 2023

Requirements files have to be compiled on Linux!

This is really unfortunate. We ought to have a documented way for developers using macOS systems to avoid this hazard. A solution that uses Docker would be OK if we can't find an easier alternative.

I think on macOS you could use a VM or maybe Docker but I haven't tried it. Maybe we could rewrite our make requirements script so that it always runs in Docker.

There are non-standard universal requirements file formats in the Python world such as Pipenv's format and Poetry's but these are non-standard formats and tools like Pipenv and Poetry don't just compile requirements files (so they wouldn't just replace pip-compile) but instead want to take over your whole project management, switching to them would be a long story with plenty of drawbacks as well as the benefits and overall likely not a good idea. I think we're waiting for the Python ecosystem to catch up to us here.

The most practical solution for the time being might actually be to try to avoid dependencies with macOS/Linux differences. It'll always be technically true that compiling Python requirements files can produce different results on macOS versus Linux: that's part of the ecosystem and not likely to change any time soon, nothing we can do about it. But it doesn't have to be practically true that our requirements files compile differently on macOS and Linux.

Currently I think the only such dependency we have is ipython (which depends on appnope on macOS but not on Linux). ipython is very handy but it's a dev-only dependency that we don't really need.

I think it could be difficult to keep dependencies with macOS differences out. The only way to know whether compiling a potential new dependency produces different results on macOS is to actually compile it on macOS and Linux which requires access to both systems and most developers only have access to one or the other. If you know how to read Python requirements you could look at a potential new dependency's requirements and look for OS-specific qualifiers but the question is not whether the dependency has any such qualifiers, it's whether the dependency or any of its dependencies do. And even if a dependency is really clean today that could change underneath our feet tomorrow after we've added the dependency to our requirements.

Practically speaking though these macOS or Linux differences may be rare so it may not be too hard to keep them out.

@seanh seanh merged commit e40ad31 into main May 30, 2023
@seanh seanh deleted the fix-requirements-files branch May 30, 2023 12:57
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.

2 participants