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 intersphinx cache handling #12087

Merged

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Mar 14, 2024

This one is based on top of #12083.

This is the actual implementation (and bugfix) of #11466. I open it so that I can track more easily (and don't forget about it) but don't review it until the aforementioned PR is merged (and this one is rebased).

picnixz added 30 commits October 4, 2023 16:19
…com:picnixz/sphinx into fix/11466-intersphinx-inventory-consistency
@picnixz picnixz requested a review from AA-Turner July 22, 2024 15:08
Co-authored-by: James Addison <55152140+jayaddison@users.noreply.github.com>
@AA-Turner
Copy link
Member

AA-Turner commented Jul 22, 2024

@picnixz on the tests, please could you move everything new to a new test_ext_intersphinx_cache.py file? That would allow you to define the constants and classes as module globals and avoid nested class/function definitions.

If you're able to add more comments / explanation of what's being tested and which invariants we expect to hold etc, that would be great. (though optional)

A

@picnixz
Copy link
Member Author

picnixz commented Jul 22, 2024

on the tests, please could you move everything new to a new test_ext_intersphinx_cache.py file? That would allow you to define the constants and classes as module globals and avoid nested class/function definitions.

Yes I can (I thought about it because I was like "mmmh this file becomes huge...")

@AA-Turner AA-Turner changed the title [8.x] fix intersphinx cache Fix intersphinx cache handling Jul 22, 2024
@picnixz
Copy link
Member Author

picnixz commented Jul 22, 2024

Do you want me to fix the tests now? or are you working on it now?

@picnixz
Copy link
Member Author

picnixz commented Jul 22, 2024

I'll take care of fixing the CI/CD

@AA-Turner
Copy link
Member

Do you want me to fix the tests now?

That'd be great, thank you! I've no more changes for now.

@jayaddison
Copy link
Contributor

Do you want me to fix the tests now? or are you working on it now?

@AA-Turner @picnixz one more observation: during those test failures, tests on Windows succeeded. And on all the others, an Intersphinx cache key eviction failure occurred. Could the reason that only Windows passed -- and also perhaps the reason why it tends to take longer than other CI pipelines -- be that it is rebuilding from fresh every time?

Copy link
Contributor

@jayaddison jayaddison left a comment

Choose a reason for hiding this comment

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

I would personally still prefer to see the itemgetter usage removed and replaced with named attributes.

However, I'm comfortable enough with the state of the changeset to approve it 👍

@picnixz
Copy link
Member Author

picnixz commented Jul 22, 2024

I split the test into 3 actually:

  • One test for checking that it works correctly the first time.
  • One test for checking 'build -> update -> build'.
  • One test for checking 'build -> update -> build -> revert -> build'

I'm going offline now.

@AA-Turner AA-Turner merged commit 61daf1c into sphinx-doc:master Jul 22, 2024
19 checks passed
@AA-Turner
Copy link
Member

Thanks for the mammoth effort @picnixz

@picnixz
Copy link
Member Author

picnixz commented Jul 23, 2024

You're welcome! (the effort wasn't that much and it was fun). I plan, at some point, to refactor the tests in general so that we can have helper classes but I cannot find time for that...

@picnixz picnixz deleted the fix/11466-intersphinx-inventory-consistency branch July 23, 2024 06:19
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[intersphinx] '<' not supported between instances of 'dict' and 'dict' in incremental builds
3 participants