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

Do not add checksums to css files if building using the htmlhelp builder #11894

Merged
merged 4 commits into from Mar 15, 2024

Conversation

mkay-sh
Copy link
Contributor

@mkay-sh mkay-sh commented Jan 18, 2024

Subject: The windows help compiler requires that css links don't have a query component. This PR disables the checksum for css files if the htmlhelp builder is used.

Feature or Bugfix

  • Bugfix

Relates

@AA-Turner
Copy link
Member

Please could you add an entry to CHANGES?

@jayaddison
Copy link
Contributor

This looks like it affects Sphinx 7.1.0 onwards currently, and I do think that this would solve the problem.

However: it's not great to introduce builder-specific fixes in Sphinx itself, and I'd suggest that maybe adding a crc32_checksum attribute (default True, overridden as False in the sphinxcontrib-htmlhelp builder) to the base HTML builder -- since that's the top-level at which this feature is relevant -- could be an alternative. (I considered a generic checksum attribute name -- but I think that there are other checksum/integrity methods we might want to consider in future, hence trying to avoid a future naming conflict)

That said: that would require some release co-ordination (addition + release in Sphinx, then a corresponding usage + baseline Sphinx dependency bump + release of sphinxcontrib-htmlhelp) - so so it's possible that I'm suggesting more work than is practically worthwhile.

@jayaddison
Copy link
Contributor

Ok: now I understand that as of recently, many of the sphinxcontrib-* extensions no longer have Python package dependency references back to Sphinx itself. So in the short term, my alternative suggestion (implement an attribute in Sphinx, release, update the baseline dep in the extension, then release that) is not valid.

I'll try to learn a bit more about the reasoning behind the dependency versioning change (#11567); to my mind, the ability to list supported Sphinx versions from each extensions seems quite important.

@jayaddison
Copy link
Contributor

I think my previous comments here were too hasty and cautious about this - I now think it's a reasonable solution to a genuine problem, and that unless another approach arrives (that I could and should offer myself if I find time), on balance it's better to fix the existing issue. My apologies for delay caused.

@picnixz picnixz added the awaiting:response Waiting for a response from the author of this issue label Feb 25, 2024
@mkay-sh
Copy link
Contributor Author

mkay-sh commented Mar 15, 2024

Please could you add an entry to CHANGES?

I added an entry to the changes.

@picnixz picnixz removed the awaiting:response Waiting for a response from the author of this issue label Mar 15, 2024
@picnixz
Copy link
Member

picnixz commented Mar 15, 2024

Thank you! I'll merge it when the tests are done

@picnixz
Copy link
Member

picnixz commented Mar 15, 2024

Thank you!

@picnixz picnixz merged commit 7bd9c59 into sphinx-doc:master Mar 15, 2024
22 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTMLHELP Relative URL strings must not have a query component
4 participants