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 Sphinx configuration post-update #5588

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

snejus
Copy link
Member

@snejus snejus commented Jan 12, 2025

Adjust Sphinx configuration and release script

Changes:

  • Update source_suffix in docs/conf.py to use dict format as required by newer Sphinx
    versions
  • Fix typo in theme config: pygment_light_style -> pygments_light_style
  • Improve release script to handle Sphinx intersphinx output formatting:
    • Replace tabs with spaces for consistent parsing
    • Add reference to sphinx.ext.intersphinx command in docstring
    • Update line parsing to match space-indented format
  • Restructure CI workflow to ensure relevant dependencies are installed and changelog formatting is tested.

These changes ensure compatibility with newer Sphinx versions and improve the robustness
of the release script's reference parsing.

Edit:

While working on this PR GitHub decided to upgrade their linux/ubuntu runners with Ubuntu 24.04. Ubuntu upgrades are the worst: imagine not updating your system for 3 years and then suddenly upgrading everything to the most recent versions. At which point you start realizing that the final S in their LTS means Suffering instead of Support.

We use just a few system dependencies in our builds and as expected, things broke:

  • libcairo2-dev now needs to be installed for pygobject.
  • pandoc rst -> markdown conversion output has changed
  • Completion tests are unhappy about bash / bash-completion upgrade, and I could not figure out why so I'm just xfailing that test in CI.

@snejus snejus self-assigned this Jan 12, 2025
@snejus snejus force-pushed the adjust-sphinx-post-update branch from 163f651 to 8ed4430 Compare January 12, 2025 01:11
@snejus snejus force-pushed the adjust-sphinx-post-update branch from f3acf80 to 64b3481 Compare January 12, 2025 04:51
@snejus snejus marked this pull request as ready for review January 12, 2025 04:53
Copy link

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

@snejus snejus requested a review from Serene-Arc January 12, 2025 04:53
@snejus snejus force-pushed the adjust-sphinx-post-update branch from 5193dd8 to ad70aea Compare January 12, 2025 05:09
@snejus snejus force-pushed the adjust-sphinx-post-update branch from ad70aea to 5fc92c9 Compare January 12, 2025 05:12
Ubuntu version in GitHub Actions has recently been upgraded to 24.04:
  actions/runner-images#10636)

This meant that pandoc was upgraded and it changed the way markdown is
formatted by default.
Thanks GitHub for breaking workflows out of thin air.
@snejus snejus force-pushed the adjust-sphinx-post-update branch 5 times, most recently from 0a90c73 to d2eaa89 Compare January 13, 2025 22:36
@snejus snejus force-pushed the adjust-sphinx-post-update branch from d2eaa89 to 3bb8af8 Compare January 13, 2025 22:48
@JOJ0 JOJ0 self-requested a review January 15, 2025 06:35
Copy link
Member

@JOJ0 JOJ0 left a comment

Choose a reason for hiding this comment

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

I looked through it and most LGTM as you can read in inline comments. I'm wondering how I could simulate a release in my fork and see what the final outcome is. Is it as easy as pushing a tag to the fork and then do whatever is to do the trigger the workflow (need to read docs how to do the latter)

Fun fact: I copied the output of "releasy.py changelog" to a github release and the preview showed actual bug emojies for :bug: - a feature not a bug! sweet! 🍭 (and now realize that this is in place since a couple of releases already haha ;-))

Anyway I think this is already good to go and let's kick out more releases soon for the real test.

@@ -66,7 +66,7 @@
"logo": {
"text": "beets",
},
"pygment_light_style": "bw",
"pygments_light_style": "bw",
Copy link
Member

Choose a reason for hiding this comment

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

Whoops that typo was me. The funny thing is that we already seem to have the "black & white" style. If this is set to as you suggest, I get "colored" code boxes.

The pydata theme docs reveal that this was actually already set correctly and intentionally the "bw" theme was chosen (by me alone AFAIR - let's discuss if we want more color :-)). The colors we see when setting to "pygmentS..." is just the fallback to the default theme.

https://pydata-sphinx-theme.readthedocs.io/en/v0.14.4/user_guide/styling.html#configure-pygments-theme

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"pygments_light_style": "bw",
"pygment_light_style": "bw",

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I adjusted it was this warning:

$ poe docs
Poe => make -C docs html
make: Entering directory '/home/sarunas/repo/beets/docs'
sphinx-build -b html -d _build/doctrees   . _build/html
Running Sphinx v7.4.7
loading translations [en]... done
loading pickled environment... done
The parameter "pygment_light_style" was renamed to "pygments_light_style" (note the "s" on "pygments").
building [mo]: targets for 0 po files that are out of date

Have you poetry install before regenerating the docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

On the other hand I've been a bit confused about the lack of colors! 😅 Would you mind if we remove this configuration, going back to the default?

extra/release.py Show resolved Hide resolved
extra/release.py Show resolved Hide resolved
extra/release.py Show resolved Hide resolved
@pytest.mark.xfail(
os.environ.get("GITHUB_ACTIONS") == "true" and sys.platform == "linux",
reason="Completion is for some reason unhappy on Ubuntu 24.04 in CI",
)
Copy link
Member

Choose a reason for hiding this comment

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

Not that I have such a distro around but would it help to test it on a "real" Ubuntu install?

Copy link
Member

@JOJ0 JOJ0 Jan 17, 2025

Choose a reason for hiding this comment

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

@snejus I think I found the cause. Debian/Ubuntu changed the path of bash. It's now /usr/bin/bash: https://launchpadlibrarian.net/715612409/bash_5.2.21-2ubuntu1_5.2.21-2ubuntu2.diff.gz

https://launchpad.net/ubuntu/+source/bash/5.2.21-2ubuntu2

Maybe we can try working around that by using /usr/bin/env bash?

Copy link
Member

@JOJ0 JOJ0 Jan 17, 2025

Choose a reason for hiding this comment

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

....but I'm not sure anymore if my assumption is correct, let's simply trial and error is my best idea. Too much to read here in this "decision paper" (or whatever it is): https://dep-team.pages.debian.net/deps/dep17/

Copy link
Member

@JOJ0 JOJ0 Jan 17, 2025

Choose a reason for hiding this comment

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

I played around with this in my open PR. Neither changing to /usr/bin/env bash nor changing to /usr/bin/bash fixes it. I'm sorry, it's something else :-(

Screenshot 2025-01-17 at 18 09 13

And also I realize that we had a check for that in place already :-(

        if not has_program(cmd[0]):
            self.skipTest("bash not available")

Copy link
Member

Choose a reason for hiding this comment

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

After endless tinkering with getting bash-completion even to test on my laptop (macOS, old bash, incompatible completion script, yadda yadda) and wanting to debug it locally, I gave up on that idea and continued to push weird ideas to github. The first interesting clue: Permission denied !?!

Screenshot 2025-01-18 at 00 38 11

Copy link
Member Author

Choose a reason for hiding this comment

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

😞 I see a familiar struggle here to what I experienced. I didn't manage to get as far as to trigger the permission denied error - this looks interesting. How did you do it?

docs/conf.py Show resolved Hide resolved
@JOJ0
Copy link
Member

JOJ0 commented Jan 18, 2025

I invested quite some time already trying to fix/understand the bash-completion issue. I suggest we keep it disabled as you suggested in commit 3bb8af8. It's not worth further hassle in my opinion!

Am I right that we have some of the issues you mention after the runner ubuntu upgade in all open PR's? If so, that's even more a reason to live with the bash-completion issue and move on with merging this one. It's good to go in my opinion. Thanks!

@snejus
Copy link
Member Author

snejus commented Jan 18, 2025

Am I right that we have some of the issues you mention after the runner ubuntu upgade

Yes, completely. Just let me know what do you think regarding removing the "bw" colors configuration to introduce coloring to the code blocks and we go ahead with the merge!

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