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

Separate security scanned lockfile #159

Merged
merged 36 commits into from
Jan 13, 2025
Merged

Separate security scanned lockfile #159

merged 36 commits into from
Jan 13, 2025

Conversation

miikkako
Copy link
Contributor

@miikkako miikkako commented Jan 7, 2025

Supply a lockfile with package checksums which is scanned for vulnerabilities by aikido. No functional changes to code.

  • test_requirements_lock ci job verifies the lockfile is always up to date. Only the direct top-level deps specified in pyproject.toml are upgraded by the script.
  • tox is removed from dependencies and its envs from tox.ini are converted into simple executable files containing commands (test, format, docbuild)
  • Python 3.9 support dropped. Numpy stopped supporting it already from 2.0, and 2.2.1 is the current newest
  • uv is used in all ci and its usage is also recommended in README

@miikkako miikkako force-pushed the separate-scanned-lockfile branch from 67a3a3c to 98f25e6 Compare January 8, 2025 07:50
@miikkako miikkako force-pushed the separate-scanned-lockfile branch from 98f25e6 to a9a5464 Compare January 8, 2025 07:51
README.rst Outdated
Comment on lines 25 to 31
Supplied within the python package there is an additional `requirements.txt` file containing locked, security scanned
dependencies. The file can be used to constrain installed dependencies either directly from the repo or by
extracting it from the PyPI package.

.. code-block:: bash

$ pip install -r requirements.txt iqm-client
Copy link
Contributor Author

@miikkako miikkako Jan 8, 2025

Choose a reason for hiding this comment

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

This optional lockfile requirements.txt is merely supplied with the package and how to use it is mentioned in README.

The other option would've been an optional extra dependency like so: pip install iqm-client[lock], like this in the other PR. But it simply does not work with python -m build (invokes setuptools.build_meta via pyproject.toml) if there are checksums (--hash=sha256:...) in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example error excerpt:

packaging.requirements.InvalidRequirement: Expected end or semicolon (after version specifier)
    accessible-pygments==0.0.5--hash=sha256:40918d3e6a2b619ad424cb91e556bd3bd8865443d9f22f1dcdf79e33c8046872--hash=sha256:88ae3211e68a1d0b011504b2ffc1691feafce124b845bd072ab6f9f66f34d4b7
                       ~~~~~~~^
  × Failed to build `.../iqm-client`
  ├─▶ The build backend returned an error
  ╰─▶ Call to `setuptools.build_meta.build_sdist` failed (exit status: 1)
      hint: This usually indicates a problem with the package or the build environment.

@miikkako
Copy link
Contributor Author

miikkako commented Jan 8, 2025

requirements.txt should be used in ci, e.g. python -m pip install -e ".[dev]" -c requirements.txt but pip install doesn't support constraining with extras. uv pip install -c requirements.txt -e ".[dev]" on the other hand works.

@Aerylia
Copy link
Contributor

Aerylia commented Jan 8, 2025

This is similar to the qiskit-on-iqm lock file PR which is on hold because people were unhappy with the requirements.txt. Personally, I think the vulnerability checking should be done on the pyproject.toml as we support multiple combinations of python and library versions. Just checking if one of those is safe defeats the point. @shabeebk is looking into this.

@miikkako
Copy link
Contributor Author

miikkako commented Jan 8, 2025

This is similar to the qiskit-on-iqm lock file PR which is on hold because people were unhappy with the requirements.txt. Personally, I think the vulnerability checking should be done on the pyproject.toml as we support multiple combinations of python and library versions. Just checking if one of those is safe defeats the point. @shabeebk is looking into this.

The pyproject.toml way didn't work: see my comment #159 (comment)

@Aerylia
Copy link
Contributor

Aerylia commented Jan 8, 2025

This is similar to the qiskit-on-iqm lock file PR which is on hold because people were unhappy with the requirements.txt. Personally, I think the vulnerability checking should be done on the pyproject.toml as we support multiple combinations of python and library versions. Just checking if one of those is safe defeats the point. @shabeebk is looking into this.

The pyproject.toml way didn't work: see my comment #159 (comment)

Ok. So the vulnerability scanner only scans latest supported libraries? That sounds like a nice way to overlook vulnerabilities of old library versions.

@miikkako
Copy link
Contributor Author

miikkako commented Jan 10, 2025

Ok. So the vulnerability scanner only scans latest supported libraries? That sounds like a nice way to overlook vulnerabilities of old library versions.

I made it so now that there is a script update-requirements.py to keep requirements.txt up to date by upgrading the dependencies listed in pyproject.toml. With that, transitive dependencies aren't automatically updated which can't be controlled anyway when iqm-client is installed as a library. This best-effort security scanned requirements.txt is now used in ci jobs and I've yet to write developer instructions in README

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
Co-authored-by: Rakhim Davletkaliyev <rakhim.davletkaliyev@meetiqm.com>
@miikkako miikkako merged commit 53afeea into main Jan 13, 2025
14 checks passed
@miikkako miikkako deleted the separate-scanned-lockfile branch January 13, 2025 10:16
miikkako added a commit that referenced this pull request Jan 13, 2025
Fix failing build/publish CI from #159
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants