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

fix changed code #5579

Merged
merged 1 commit into from Jul 30, 2023
Merged

fix changed code #5579

merged 1 commit into from Jul 30, 2023

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Jul 30, 2023

PR #5556 changed the code to use __Pyx_PySequence_ITEM, but changed a bit too much on this line. This was causing PyPy to fail the fused_def test, after the fix the test passes.

This is the change in the context of the PR

@da-woods da-woods merged commit f082052 into cython:master Jul 30, 2023
73 of 75 checks passed
@da-woods da-woods added this to the 3.0.1 milestone Jul 30, 2023
@da-woods
Copy link
Contributor

da-woods commented Jul 30, 2023

Thanks. I wonder why we didn't see it in PyPy in the CI? I'd thought we didn't allow failures on PyPy...

@mattip
Copy link
Contributor Author

mattip commented Jul 30, 2023

Dunno. I think this is the one. It seems to pass here, but failed on my weekly PyPy + Cython build

@da-woods
Copy link
Contributor

The only thing you're obviously doing differently is -DCYTHON_USE_TYPE_SPECS, although (based on a quick look) I don't see why that would affect this code.

Do you think we should be setting -DCYTHON_USE_TYPE_SPECS by default for PyPy?

@mattip
Copy link
Contributor Author

mattip commented Jul 30, 2023

It seems that is redefined to 0 anyway, so that flag is superfluous in CI:

#undef CYTHON_USE_TYPE_SPECS
#define CYTHON_USE_TYPE_SPECS 0

@mattip
Copy link
Contributor Author

mattip commented Jul 30, 2023

The PyPy CI run is skipping the "Setup python" step since the condition is wrong

if: startsWith(matrix.python-version, '3.')

@mattip
Copy link
Contributor Author

mattip commented Jul 30, 2023

While we are at it, I am not sure what the cache step in CI is for. Is something else skipped if the cache is found? It is emitting warnings. A newer workflow is https://github.com/actions/cache/tree/main, but I am not sure how the caching is meant to work and how to check that a cache is being used.

@da-woods
Copy link
Contributor

While we are at it, I am not sure what the cache step in CI is for. Is something else skipped if the cache is found? It is emitting warnings. A newer workflow is https://github.com/actions/cache/tree/main, but I am not sure how the caching is meant to work and how to check that a cache is being used.

ccache skips compilation of a file if the file is identical to one it's seen before (and all the defines/command line flags are the same). From the point of view of the tests it should be the same - it just accelerates the C compilation step by using a result from earlier

@da-woods
Copy link
Contributor

The PyPy CI run is skipping the "Setup python" step since the condition is wrong

if: startsWith(matrix.python-version, '3.')

That's likely it. So possibly we're not actually setting up PyPy and just running everything in CPython... I'll investigate and fix if needed (but likely tomorrow).

Thanks for spotting that!

@mattip
Copy link
Contributor Author

mattip commented Jul 31, 2023

I submitted #5582

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants