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

python3-ipython: update to 8.14.0. #44268

Closed
wants to merge 2 commits into from
Closed

Conversation

tornaria
Copy link
Contributor

@tornaria tornaria commented Jun 4, 2023

  • switch to pep517 build style: needs using a venv
  • update depends and checkdepends
  • break unnecessary checkdepends cycle with ipython_ipykernel

Testing the changes

  • I tested the changes in this PR: briefly

I will run sagemath testsuite with this update and report back.

@ahesford here's one package where testing with PYTHONPATH breaks and testing in a venv works. I wrote a custom do_check() function but you'll see it's quite generic and could be moved to build-style/python3-pep517.sh.

Maybe you can figure out a different way to make the generic testing with PYTHONPATH to work.

@tornaria
Copy link
Contributor Author

tornaria commented Jun 4, 2023

There was a test failure on x86_64-musl:

FAILED IPython/utils/tests/test_process.py::SubProcessTestCase::test_system_interrupt

But in my local test this passed.

 - switch to pep517 build style: needs using a venv
 - update depends and checkdepends
 - break unnecessary checkdepends cycle with ipython_ipykernel
 - fix a race (ipython/ipython#12164)
@tornaria
Copy link
Contributor Author

tornaria commented Jun 5, 2023

I rerun CI without changes and got the same failure. Locally I can reproduce if I change the sleep time from 0.5 to 0.0.

I added a reasonably looking patch that I submitted upstream at ipython/ipython#12164. Now all tests pass (even with sleep time 0.0).

I also run the sage testsuite with this update without any issue so this is ready to merge.

I would appreciate any comments on what to do about do_check().

srcpkgs/python3-ipython/template Show resolved Hide resolved
srcpkgs/python3-ipython/template Show resolved Hide resolved
Comment on lines 22 to 31
do_check() {
# Running via PYTHONPATH breaks... venv works
# This could be moved to build-style/python3-pep517.sh
local testdir="${wrksrc}/.xbps-testdir"
rm -rf "${testdir}"
python3 -m venv --system-site-packages --without-pip "${testdir}"
local -x PATH="${testdir}/bin:${PATH}"
python3 -m installer dist/*.whl
python3 -m pytest
}
Copy link
Member

Choose a reason for hiding this comment

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

The problem isn't that this works in a venv, it's how you're calling pytest. The default do_build runs pytest3 but you are running python3 -m pytest, which means you add . to the Python path by default. If you were to run

ln -Tsf /usr/bin/pytest3 "${testdir}/bin/pytest3"
"${testdir}/bin/pytest3"

instead of python3 -m pytest, the tests would again fail with an inability to find IPython.

This suggests that pytest is not actually testing against the contents of the wheel, but is instead testing the contents of the source tree. (Although, in the single test test_ipython_embed, it expects to find an ipython entrypoint and fails.) In fact, it is sufficient to do

do_check() {
    python3 -m pytest -k 'not(test_ipython_embed)'
}

for a successful test run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem isn't that this works in a venv, it's how you're calling pytest. The default do_build runs pytest3 but you are running python3 -m pytest, which means you add . to the Python path by default.

I see, it's really misleading that the python path changes depending on this.

If you were to run

ln -Tsf /usr/bin/pytest3 "${testdir}/bin/pytest3"
"${testdir}/bin/pytest3"

instead of python3 -m pytest, the tests would again fail with an inability to find IPython.

I don' t think that will run pytest in the venv, since the shebang for /usr/bin/pytest3 is /usr/bin/python3. It does what you claim if I do something like

"${testdir}/bin/python3" /usr/bin/pytest3

so TIL that python3 -m pytest is not the same as python3 /usr/bin/pytest.

This suggests that pytest is not actually testing against the contents of the wheel, but is instead testing the contents of the source tree.

Sure and since all the test files are actually installed in the wheel, we get a conflict between the files pytest is trying to load (from .) and the modules that are installed in $testdir.

So an easy way to make . into the python path would be

make_check_pre='env PYTHONPATH=.'

With this in place and the standard do_check(), all tests pass (even test_ipython_embed). What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's the best option, because it requires a single line without overriding the default functions.

@ahesford ahesford closed this in bfe4edd Jun 5, 2023
@tornaria tornaria deleted the ipython branch June 6, 2023 00:07
sirkhancision pushed a commit to sirkhancision/void-packages that referenced this pull request Jun 10, 2023
 - switch to pep517 build style
 - update depends and checkdepends
 - break unnecessary checkdepends cycle with ipython_ipykernel
 - fix a race (ipython/ipython#12164)

Closes: void-linux#44268 [via git-merge-pr]
sirkhancision pushed a commit to sirkhancision/void-packages that referenced this pull request Jun 12, 2023
 - switch to pep517 build style
 - update depends and checkdepends
 - break unnecessary checkdepends cycle with ipython_ipykernel
 - fix a race (ipython/ipython#12164)

Closes: void-linux#44268 [via git-merge-pr]
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