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

Drop dh-virtualenv #6538

Closed
legoktm opened this issue Sep 7, 2022 · 7 comments · Fixed by #6884
Closed

Drop dh-virtualenv #6538

legoktm opened this issue Sep 7, 2022 · 7 comments · Fixed by #6884
Labels
needs/discussion queued up for discussion at future team meeting. Use judiciously.

Comments

@legoktm
Copy link
Member

legoktm commented Sep 7, 2022

Description

We currently use dh-virtualenv to create a virtualenv that we install our dependencies in. It has some nice features, and some downsides. I believe that we've reached the point that the downsides outweigh the features, which are simple to reimplement ourselves. This is a proposal for all the SDW components too, I just didn't want to file two identical tickets and split discussion.

Features

  • Creates virtualenv in /opt/venvs/..., installs dependencies and our code
  • Edits the virtualenv so it can properly be relocated (artifact of Debian packaging, it's built in ./debian/securedrop-app-code and then moved to opt/venvs)
  • Automatically rebuild the virtualenv when the underlying Python version changes with triggers (this "feature" caused issues for us, postinst error in securedrop-app-code package install #6230)

Downsides

Proposal

  • Create the virtualenv ourselves, with python3 -m venv ...
  • Install the dependencies ourselves, with pip install ...
  • Fix up the virtualenv ourselves, with a one-line sed command (n.b. in testing this, I discovered their sed command isn't fully comprehensive as mine was, so this will help with reproducibility)

I think this will also demystify exactly what is happening at build time since we can see the underlying pip/virtualenv commands.

@legoktm
Copy link
Member Author

legoktm commented Sep 7, 2022

Here's my demo of this change against securedrop-client: freedomofpress/securedrop-builder@a580cf0 - I think it's significantly clearer exactly what is happening during the install step (the sed step probably needs a comment).

The diffoscope report: https://gist.github.com/legoktm/61d28e6f045bbcb10500114b9ee8f1f4 - the useless interpreter hook is removed, and insignificant quoting changes plus one spot VIRTUAL_ENV wasn't updated properly that the new sed command fixes.

@legoktm legoktm added the needs/discussion queued up for discussion at future team meeting. Use judiciously. label Sep 7, 2022
@gonzalo-bulnes
Copy link
Contributor

💡 I find the diffoscope report very compelling! The changes, given your explanation in this PR's description, seem very reasonable to me 👍

@eaon
Copy link
Contributor

eaon commented Sep 8, 2022

Yeah this seems all around reasonable. Added bonus, if I'm not mistaken we could probably use the same approach for the workstation/launcher/updater RPM package (which would greatly benefit from standardisation)

@kushaldas
Copy link
Contributor

Yeah this seems all around reasonable. Added bonus, if I'm not mistaken we could probably use the same approach for the workstation/launcher/updater RPM package (which would greatly benefit from standardisation)

RPM by default strips away libraries and that can cause trouble in similar case. Otherwise I also wrote https://github.com/kushaldas/rpm-macros-virtualenv :)

@legoktm
Copy link
Member Author

legoktm commented Jan 31, 2023

inactive Debian maintainer, I've had to fix it twice already so it stays in testing

I just filed https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1030170 about how it's incompatible with bookworm's Python 3.11. It was fixed upstream last month (see spotify/dh-virtualenv#354) and merged by the maintainer very quickly, but no new release nor an update in Debian. It also doesn't appear to be running the standard Debian CI autopkgtests tests that would've flagged this automatically rather than needing human reports.

I'll wait a day or two before kicking off the NMU process to fix it again.

@legoktm
Copy link
Member Author

legoktm commented Feb 2, 2023

I'll wait a day or two before kicking off the NMU process to fix it again.

Done, it'll take ~10 days to migrate into bookworm, for now I've proposed just patching it via sed when we install deps: freedomofpress/securedrop-builder@dcbe842

@legoktm
Copy link
Member Author

legoktm commented Feb 15, 2023

The failure at freedomofpress/securedrop-builder#414 has actually been emitting deprecation warnings in bullseye, except dh_virtualenv has been hiding them.

Using the demo from #6538 (comment), here's the output you get from building securedrop-client on bullseye without dh_virtualenv:

...
Building wheels for collected packages: securedrop-client
  Building wheel for securedrop-client (setup.py) ... done
  Created wheel for securedrop-client: filename=securedrop_client-0.8.1_dev_20230214_235645-py3-none-any.whl size=3892555 sha256=129cc435e404be6bc45cd2bd27c5355f88d4de378c999cdbfdd55e88060ce6a7
  Stored in directory: /tmp/pip-ephem-wheel-cache-7qt8yiqa/wheels/54/e0/66/6113ad76fe39fda5cbc19bb93d30641c66954e4d5e58cb9db1
  WARNING: Built wheel for securedrop-client is invalid: Metadata 1.2 mandates PEP 440 version, but '0.8.1-dev-20230214-235645' is not
Failed to build securedrop-client
Installing collected packages: securedrop-client
    Running setup.py install for securedrop-client ... done
  DEPRECATION: securedrop-client was installed using the legacy 'setup.py install' method, because a wheel could not be built for it. pip 21.0 will remove support for this functionality. A possible replacement is to fix the wheel build issue reported above. You can find discussion regarding this at https://github.com/pypa/pip/issues/8368.
Successfully installed securedrop-client-0.8.1-dev-20230214-235645

Unfortunately dh_virtualenv hides those messages so we didn't notice when we switched to bullseye in the first place.

On bookworm with dh_virtualenv, the error you get is:

Processing /root/project/build/debbuild/packaging/securedrop-client
  error: subprocess-exited-with-error
  
  × python setup.py egg_info did not run successfully.
  │ exit code: 1
  ╰─> See above for output.
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
  Preparing metadata (setup.py) ... error
error: metadata-generation-failed

× Encountered error while generating package metadata.
╰─> See above for output.

note: This is an issue with the package mentioned above, not pip.
hint: See above for details.

Contrast that with the bookworm output without dh_virtualenv:

Processing /src/build/debbuild/packaging/securedrop-client
  Preparing metadata (setup.py) ... error
  error: subprocess-exited-with-error
  
  × python setup.py egg_info did not run successfully.
  │ exit code: 1
  ╰─> [30 lines of output]
      /src/build/debbuild/packaging/securedrop-client/debian/securedrop-client/opt/venvs/securedrop-client/lib/python3.11/site-packages/setuptools/dist.py:548: UserWarning: The version specified ('0.8.1-dev-20230214-235645') is an invalid version, this may not work as expected with newer versions of setuptools, pip, and PyPI. Please see PEP 440 for more details.
        warnings.warn(
      running egg_info
      Traceback (most recent call last):
        File "<string>", line 2, in <module>
        File "<pip-setuptools-caller>", line 34, in <module>
        File "/src/build/debbuild/packaging/securedrop-client/setup.py", line 15, in <module>
          setuptools.setup(
        File "/src/build/debbuild/packaging/securedrop-client/debian/securedrop-client/opt/venvs/securedrop-client/lib/python3.11/site-packages/setuptools/__init__.py", line 108, in setup
          return distutils.core.setup(**attrs)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        File "/src/build/debbuild/packaging/securedrop-client/debian/securedrop-client/opt/venvs/securedrop-client/lib/python3.11/site-packages/setuptools/_distutils/core.py", line 185, in setup
          return run_commands(dist)
                 ^^^^^^^^^^^^^^^^^^
        File "/src/build/debbuild/packaging/securedrop-client/debian/securedrop-client/opt/venvs/securedrop-client/lib/python3.11/site-packages/setuptools/_distutils/core.py", line 201, in run_commands
          dist.run_commands()
        File "/src/build/debbuild/packaging/securedrop-client/debian/securedrop-client/opt/venvs/securedrop-client/lib/python3.11/site-packages/setuptools/_distutils/dist.py", line 969, in run_commands
          self.run_command(cmd)
        File "/src/build/debbuild/packaging/securedrop-client/debian/securedrop-client/opt/venvs/securedrop-client/lib/python3.11/site-packages/setuptools/dist.py", line 1213, in run_command
          super().run_command(command)
        File "/src/build/debbuild/packaging/securedrop-client/debian/securedrop-client/opt/venvs/securedrop-client/lib/python3.11/site-packages/setuptools/_distutils/dist.py", line 987, in run_command
          cmd_obj.ensure_finalized()
        File "/src/build/debbuild/packaging/securedrop-client/debian/securedrop-client/opt/venvs/securedrop-client/lib/python3.11/site-packages/setuptools/_distutils/cmd.py", line 111, in ensure_finalized
          self.finalize_options()
        File "/src/build/debbuild/packaging/securedrop-client/debian/securedrop-client/opt/venvs/securedrop-client/lib/python3.11/site-packages/setuptools/command/egg_info.py", line 219, in finalize_options
          parsed_version = parse_version(self.egg_version)
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        File "/src/build/debbuild/packaging/securedrop-client/debian/securedrop-client/opt/venvs/securedrop-client/lib/python3.11/site-packages/pkg_resources/_vendor/packaging/version.py", line 266, in __init__
          raise InvalidVersion(f"Invalid version: '{version}'")
      pkg_resources.extern.packaging.version.InvalidVersion: Invalid version: '0.8.1-dev-20230214-235645'
      [end of output]
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
error: metadata-generation-failed

× Encountered error while generating package metadata.
╰─> See above for output.

note: This is an issue with the package mentioned above, not pip.
hint: See above for details.

legoktm added a commit that referenced this issue Jun 27, 2023
dh-virtualenv is responsible for creating the virtualenv, installing
dependencies from requirements.txt, and then munging the virtualenv to
use the path that it will be installed to.

There are a number of issues with it however:
* largely unmaintained in Debian (missed focal)
* incompatible with debhelper compat 12
* cannot use --require-hashes
* hides deprecation warnings and error messages

It is now getting in the way of Rust packaging since we want to install
a wheel into the virtualenv, however it needs to happen before the
munging step, which dh-virtualenv doesn't really support.

Since we already want to get rid of it, let's just do it now.

Fixes #6538.
legoktm added a commit that referenced this issue Jun 29, 2023
dh-virtualenv is responsible for creating the virtualenv, installing
dependencies from requirements.txt, and then munging the virtualenv to
use the path that it will be installed to.

There are a number of issues with it however:
* largely unmaintained in Debian (missed focal)
* incompatible with debhelper compat 12
* cannot use --require-hashes
* hides deprecation warnings and error messages

It is now getting in the way of Rust packaging since we want to install
a wheel into the virtualenv, however it needs to happen before the
munging step, which dh-virtualenv doesn't really support.

Since we already want to get rid of it, let's just do it now.

Fixes #6538.
legoktm added a commit that referenced this issue Jul 20, 2023
dh-virtualenv is responsible for creating the virtualenv, installing
dependencies from requirements.txt, and then munging the virtualenv to
use the path that it will be installed to.

There are a number of issues with it however:
* largely unmaintained in Debian (missed focal)
* incompatible with debhelper compat 12
* cannot use --require-hashes
* hides deprecation warnings and error messages

It is now getting in the way of Rust packaging since we want to install
a wheel into the virtualenv, however it needs to happen before the
munging step, which dh-virtualenv doesn't really support.

Since we already want to get rid of it, let's just do it now.

Fixes #6538.
legoktm added a commit that referenced this issue Jul 21, 2023
dh-virtualenv is responsible for creating the virtualenv, installing
dependencies from requirements.txt, and then munging the virtualenv to
use the path that it will be installed to.

There are a number of issues with it however:
* largely unmaintained in Debian (missed focal)
* incompatible with debhelper compat 12
* cannot use --require-hashes
* hides deprecation warnings and error messages

It is now getting in the way of Rust packaging since we want to install
a wheel into the virtualenv, however it needs to happen before the
munging step, which dh-virtualenv doesn't really support.

Since we already want to get rid of it, let's just do it now.

Fixes #6538.
legoktm added a commit that referenced this issue Aug 2, 2023
dh-virtualenv is responsible for creating the virtualenv, installing
dependencies from requirements.txt, and then munging the virtualenv to
use the path that it will be installed to.

There are a number of issues with it however:
* largely unmaintained in Debian (missed focal)
* incompatible with debhelper compat 12
* cannot use --require-hashes
* hides deprecation warnings and error messages

It is now getting in the way of Rust packaging since we want to install
a wheel into the virtualenv, however it needs to happen before the
munging step, which dh-virtualenv doesn't really support.

Since we already want to get rid of it, let's just do it now.

Fixes #6538.
legoktm added a commit that referenced this issue Aug 2, 2023
dh-virtualenv is responsible for creating the virtualenv, installing
dependencies from requirements.txt, and then munging the virtualenv to
use the path that it will be installed to.

There are a number of issues with it however:
* largely unmaintained in Debian (missed focal)
* incompatible with debhelper compat 12
* cannot use --require-hashes
* hides deprecation warnings and error messages

It is now getting in the way of Rust packaging since we want to install
a wheel into the virtualenv, however it needs to happen before the
munging step, which dh-virtualenv doesn't really support.

Since we already want to get rid of it, let's just do it now.

Fixes #6538.
@cfm cfm closed this as completed in #6884 Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/discussion queued up for discussion at future team meeting. Use judiciously.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants