Update to Sphinx 3.3.x branch and fix test mock #597
Conversation
|
@utzig I am guessing the PR you submitted upstream sphinx-doc/sphinx#8364 is what is needed to fix this? Or are there other problems too? |
utzig
force-pushed
the
disable-broken-sphinx
branch
2 times, most recently
from
5dde120
to
1791f2f
Compare
Dropping the requirements of <3.3 made it fail again, I will add it back; yes that PR is required. There might be other problems but I think with that one merged it already passes here, and will re-test locally. |
utzig
force-pushed
the
disable-broken-sphinx
branch
from
1791f2f
to
b8f07ef
Compare
|
I tested locally both with Sphinx 3.3.x and sphinx-doc/sphinx#8364 and both fail the tests. I think my suggested fix is not complete, it has fixed my issues building docs here but there's probably other things missing. |
| @@ -11,7 +11,7 @@ jobs: | |||
| - 3.0.4 | |||
| - 3.1.2 | |||
| - 3.2.0 | |||
| - git+https://github.com/sphinx-doc/sphinx.git@3.2.x | |||
| - git+https://github.com/sphinx-doc/sphinx.git@3.3.x | |||
There was a problem hiding this comment.
@utzig Seems like 3.3.x is really broken in that case. With the current setup with requirements jobs run for 3.3.x, install 3.3.x from source, then uninstall it and fetch 3.2.x to satisfy the requirements. This results in a misleading test job running for 3.3.x.
I think commenting the 3.3.x branch out with a comment, like the master branch for Sphinx 4, is a good solution for now. Can you push a fixup for that? I will merge after, the rest is good.
There was a problem hiding this comment.
Done. I was testing a little bit why it is broken and wanted to be sure the issue is not Breathe related. Turns out some of the fails are due to asserts for valid lineno, which when Sphinx calls self.get_source_info()[1] it goes through docutils into get_source_and_line in the MockStateMachine. So I presume it is possible to have a lineno that is None but the C/CPP domain parsers are currently not allowing for it (after a recent change).
There was a problem hiding this comment.
For the mock I suggest just returning some arbitrary number.
There was a problem hiding this comment.
Done, it passes the tests, but I am not sure that means we should drop the other commit. TBH for doing another release I would rather have it not depending on Sphinx<3.3, because on one of the projects I work on, we already upgraded to 3.3.0! But it is failing for other projects, so maybe for a next release we just wait before 3.3.1? Is there a timeline for that @jakobandersen? Should I drop the requirements @vermeeren?
utzig
force-pushed
the
disable-broken-sphinx
branch
from
b8f07ef
to
1f3d81f
Compare
|
(I'll try to get the missing Sphinx PR merged for 3.3.1) |
utzig
force-pushed
the
disable-broken-sphinx
branch
2 times, most recently
from
d30e378
to
1da3aa1
Compare
Signed-off-by: Fabio Utzig <fabio.utzig@nordicsemi.no>
This allows tests passing with Sphinx==3.3.0 and branch 3.3.x (as of now). Signed-off-by: Fabio Utzig <fabio.utzig@nordicsemi.no>
|
@jakobandersen @vermeeren In retrospect I don't think Breathe should dictate a requirement of Sphinx<3.3 because of something broken, since the ABI/API are still compatible, so it was a stupid idea to do it. I dropped the original commit, added a fix to the test mock and updated the branch to get 3.3.x. If it is broken for someone, for any project the person should just fallback to 3.2.1 and or wait for 3.3.1! |
utzig
changed the title
There was a problem hiding this comment.
@utzig I think the discussions are resolved and I agree with your latest comment, so in my opinion this is good for merging as it solved the failing tests properly. Can I get a final ok from you too? I am not entirely sure if this is WIP or not.
It's not WIP, it should be fine for merging! |
|
@utzig Done, thanks! |
This avoids using Sphinx versions that are currently broken.