Ensure no missing pip dependencies in build_image.py#218
Conversation
|
@whyitfor could we please land this one and use the new option in our CI? As I am working on #416 and #417 I am went to go back to this manually to make sure all the dependencies across ofrak components are consistent (only remembered to do it because I saw the docker build output for #417 and noticed it's uninstalling/reinstalling some packages in weird way in |
|
@ANogin, can you complete the PR descripion for this? |
whyitfor
left a comment
whyitfor
left a comment
There was a problem hiding this comment.
I've reflected on this one a bit, and discussed with @rbs-jacob.
Let's make the following changes.
- Remove the
check-python-reqsargument frombuild-image.py build-image.pyshould add aninspecttarget to the Makefile it generates that runspython3 -m pip check. This will enable users who want to check for this consistency to do so.- Does
--network="none"work for all use cases? If so, let's make it default. If not, we should remove it.
ANogin
changed the title
--check_python_reqs option to build_image.pybuild_image.py
Done, should be good to go once #419 is fixed. |
| # Legacy-edinable is needed to allow mypy to find packages. | ||
| # See https://github.com/python/mypy/issues/13392 | ||
| SETUPTOOLS_ENABLE_FEATURES="legacy-editable" $(PIP) install -e .[test] |
There was a problem hiding this comment.
yes this change is no longer needed (the command line flag is preferred over the environment variable)
There was a problem hiding this comment.
No, with command line flag it insists on reaching out to pypi over network, but with the environment variable it does not.
There was a problem hiding this comment.
I've looked into this a bit and reflected.
If I understand correctly,SETUPTOOLS_ENABLE_FEATURES results in using a pre-PEP 517 backend. editable_mode=compat uses a PEP660 backend that provides some backwards compatibility. Switching back to SETUPTOOLS_ENABLE_FEATURES does not make sense to me: is moving the repo in the wrong direction, away from PEP660.
It looks like there are ways to get pip to not reach out to the network (a combination of --no-index and --find-links) which we might want to eventually look into.
For now though, I'd like to prioritize getting the pip check functionality this MR adds in, without the --network="none" changes.
Co-authored-by: Wyatt <53830972+whyitfor@users.noreply.github.com>
At the start of `finish.Dockerfile, download setuptools and wheel to /pip-wheels for pip's PEP 517 build isolation. Then use PIP_NO_INDEX=1 PIP_FIND_LINKS=/pip-wheels for all pip install commands. If any runtime dependency was not properly installed by `base.Dockerfile` and is missing, pip fails with "No matching distribution found" plus a custom error message explaining the issue. Changes to `base.Dockerfile` generation: - Install `requirements-pip.txt` first (pins pip/setuptools versions) - Remove redundant `pip install --upgrade pip` (version now pinned) - Install `requirements-dev.txt` for DEVELOP builds Changes to finish.Dockerfile generation: - Download setuptools/wheel to /pip-wheels for build isolation - Remove redundant `pip install` of `requirements-dev.txt` (now in base) - Use `PIP_NO_INDEX=1 PIP_FIND_LINKS=/pip-wheels` for all pip installs - Add custom error message when pip install fails - Add `pip check` after installation to verify dependency consistency - Add `inspect` target to generated Makefile with `pip check` - Make `test` target depend on `inspect` Supersedes #218
|
Closing in favor of #694 |
4 participants
One sentence summary of this PR (This should go in the CHANGELOG!)
This changes the
make install/make developstep of thefinish.Dockerfileto run with network disabled, which means that any missing dependencies that were not properly installed inbase.Dockerfileresult in a build failure and flagged.Link to Related Issue(s)
Would enable detecting things like #419 would be detected by CI. This depends on #571 fixing #419 (until that is fixed, this change would break the build - will keep it in draft state until then).
Please describe the changes in your request.
See summary.
Anyone you think should look at this, specifically?
@whyitfor