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

lib: use separate packages for pycriu and crit #2282

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

rst0git
Copy link
Member

@rst0git rst0git commented Oct 10, 2023

Newer versions of pip use an isolated virtual environment when building Python projects. However, when the source code of CRIT is copied into the isolated environment, the symlink for ../lib/py (pycriu) becomes invalid. As a workaround, we used the --no-build-isolation option for pip install. However, this functionality has issues in some versions of PIP (pypa/pip#8221, pypa/pip#8165 (comment)). To fix this problem, this patch adds separate packages for pycriu and crit, and each package is installed independently.

@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (ab73a84) 70.58% compared to head (afd2cf3) 70.54%.

❗ Current head afd2cf3 differs from pull request most recent head d020450. Consider uploading reports for the commit d020450 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##           criu-dev    #2282      +/-   ##
============================================
- Coverage     70.58%   70.54%   -0.05%     
============================================
  Files           135      132       -3     
  Lines         33522    33508      -14     
============================================
- Hits          23663    23638      -25     
- Misses         9859     9870      +11     

see 13 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rst0git rst0git marked this pull request as draft October 11, 2023 04:55
@avagin avagin requested a review from adrianreber October 11, 2023 05:18
@rst0git rst0git force-pushed the lib-pycriu-package branch 2 times, most recently from ebc3226 to 5a47bcb Compare October 11, 2023 11:09
Newer versions of pip use an isolated virtual environment when building
Python projects. However, when the source code of CRIT is copied into
the isolated environment, the symlink for `../lib/py` (pycriu) becomes
invalid. As a workaround, we used the `--no-build-isolation` option for
`pip install`. However, this functionality has issues in some versions
of PIP [1, 2]. To fix this problem, this patch adds separate packages
for pycriu and crit, and each package is installed independently.

[1] pypa/pip#8221
[2] pypa/pip#8165 (comment)

Signed-off-by: Radostin Stoyanov <[email protected]>
@rst0git rst0git force-pushed the lib-pycriu-package branch from 5a47bcb to d020450 Compare October 11, 2023 11:20
@adrianreber
Copy link
Member

The most important thing from my point of view is that our DEB and RPM packages still work. The last time we changed something with the way the Python bindings are distributed it required some work to get the packaging effort going again.

I think this needs to be tested with RPM and DEB packaging to ensure this time it works better than last time.

I think the best way to ship the Python bindings would be a separate repository, but the way CRIU uses Python during testing would mean that things get much more complicated if the Python code is another repository.

To make sure RPM packaging works there is a packit service which we could run in CI to ensure there is always a working package.

@rst0git rst0git marked this pull request as ready for review October 11, 2023 11:51
@rst0git
Copy link
Member Author

rst0git commented Oct 11, 2023

Thank you Adrian, enabling buildability checks is a good idea. This pull request aims to fix the error we currently see in CI with CentOS Stream 9 (https://github.com/checkpoint-restore/criu/runs/17505949509).

Should we open a separate pull request for testing RPM and DEB packaging?

@rst0git rst0git mentioned this pull request Oct 12, 2023
@avagin
Copy link
Member

avagin commented Oct 12, 2023

Thank you Adrian, enabling buildability checks is a good idea. This pull request aims to fix the error we currently see in CI with CentOS Stream 9 (https://github.com/checkpoint-restore/criu/runs/17505949509).

Do you understand why it fails only on CentOS 9?

@rst0git
Copy link
Member Author

rst0git commented Oct 12, 2023

Do you understand why it fails only on CentOS 9?

The problem that causes this error was introduced in pip version 20.1.11 and fixed in version 21.32. These versions of pip have a legacy behaviour that copies the entire project (crit) directory to a temporary location and break the pycriu symbolic link. This legacy behaviour has been disabled by default and removed in newer versions of pip.

It currently fails on CentOS 9 because the python3-pip package provides version 21.2.3.

Footnotes

  1. Revert building of local directories in place, restoring the pre-20.1 behaviour of copying to a temporary directory. (#7555)

  2. In-tree builds are now the default. --use-feature=in-tree-build is now ignored. --use-deprecated=out-of-tree-build may be used temporarily to ease the transition. (#10495)

@avagin avagin merged commit df24fe8 into checkpoint-restore:criu-dev Oct 17, 2023
35 of 37 checks passed
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.

4 participants