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

LaTeX: remove some internal \dimen registers #11105

Merged
merged 2 commits into from Jan 8, 2023

Conversation

jfbu
Copy link
Contributor

@jfbu jfbu commented Jan 7, 2023

internal refactoring, but as these internal registers have semi-public names, delayed to 7.0.0

EDITED: finally it was decided to merge immediately in time for 6.2.0. This has never been public API.

they are either unused or used with aliased names since 5.1.0

\sphinxverbatimborder
\sphinxverbatimsep
\sphinxshadowrule
\sphinxshadowsep
\sphinxshadowsize

@jfbu
Copy link
Contributor Author

jfbu commented Jan 7, 2023

Linting with Ruff fails for unrelated reasons (this patch only modifies latex style files).

@jfbu
Copy link
Contributor Author

jfbu commented Jan 7, 2023

I am currently quite confused regarding our modified branch model how we are supposed to make PR introducing breaking changes not to be included in the (currently) 6.x series (which is master branch now).

So I created a branch 7.x and created this PR for merge into it. But, on second thoughts, this branch could probably be renamed to pre-7 or develop or breaking or whatever (this is the branch which used to be called master formerly) but not yet 7.x, except if we agree that 7.x is both for all stuff which will become 7.0.0 (we should regularly merge the 6.x into 7.x), and once 7.0.0 is released it becomes branch for the 7.x series (as it names indicates) and we create then a 8.x branch for breaking changes, and so on.

clarification needed... ping Adam @AA-Turner

@AA-Turner
Copy link
Member

There should be no 7.x branch. When a breaking change is merged to master, we simply make the next release X.0.

A

@jfbu
Copy link
Contributor Author

jfbu commented Jan 7, 2023

There should be no 7.x branch. When a breaking change is merged to master, we simply make the next release X.0.

So this means delaying "breaking merges" to the time of X.0 release. Regarding LaTeX this makes it quite complex because one really can not merge things in arbitrary order. And once a breaking PR exists, any further work in this area will have to build on top of it, so either making the PR even more extensive or requiring further PRs to have been forked from the previous one, so if the previous one is updated, then a need arises to update all subsequent PRs. Also it means a time of year is well identified and well announced in advance for breaking merges, because I am currently sole maintainer which can handle, if they happen, extensive LaTeX changes.

Another remark is that what "breaking" means may need clarification. When I initially started contributing to this project I would have deemed breaking a LaTeX change which can impact the pdf page layout by only one millimeter, because a single millimeter can easily shift some lines to next page and end up causing distant changes. But, I feel "breaking" is here more in terms of API changes, or dependencies changes, so it would mean almost nothing in LaTeX support is "breaking".

If we agree on this, then it would mean I can change PDF renderings pretty much anytime ?

@jfbu
Copy link
Contributor Author

jfbu commented Jan 7, 2023

I will rebase this on current 6.x. It does not contain a modification to CHANGES but what the patch does is already announced in CHANGES.

Then I will delete the 7.x branch which I added earlier.

@jfbu jfbu changed the base branch from 7.x to master January 7, 2023 18:15
@AA-Turner
Copy link
Member

AA-Turner commented Jan 7, 2023

So this means delaying "breaking merges" to the time of X.0 release. Regarding LaTeX this makes it quite complex because one really can not merge things in arbitrary order. And once a breaking PR exists, any further work in this area will have to build on top of it, so either making the PR even more extensive or requiring further PRs to have been forked from the previous one, so if the previous one is updated, then a need arises to update all subsequent PRs. Also it means a time of year is well identified and well announced in advance for breaking merges, because I am currently sole maintainer which can handle, if they happen, extensive LaTeX changes.

I would phrase it as "breaking merges" cause an X.0 release, and we as maintainers co-ordinate on how many such breaking merges go into a release. On the extreme end of the scale, python-hypothesis has release automation such that every PR merged creates a release -- we probably don't want that, but could e.g. say that if you have a set of LaTeX changes you would like to merge, we merge the first and then wait e.g. a week or two such that you've enough time to be confident in your changes.

I do have a goal of making each release smaller in terms of the commits it contains (likely meaning more frequent X.Y.0 releases) as this makes it easier to locate when a bug was introduced from users' reports.

Another remark is that what "breaking" means may need clarification. When I initially started contributing to this project I would have deemed breaking a LaTeX change which can impact the pdf page layout by only one millimeter, because a single millimeter can easily shift some lines to next page and end up causing distant changes.

I would take the view of if on an update a user's documentation fails to compile it is a breaking change. We mitigate these via 2 releases worth of deprecation warnings though.

For layour changes, it is slightly trickier -- in the HTML builder, major layout shifts with CSS (e.g. the recent footnotes issues with Docutils 0.19) I think would be considered breaking, but e.g. a one milimeter shift for spacing I would not consider breaking -- it is a judgement call on the part of the maintainer.

(Sorry not to have a clear cut answer!)

A

@jfbu
Copy link
Contributor Author

jfbu commented Jan 7, 2023

Thanks @AA-Turner for taking the time to explain. I need to think about it. A real-life case can be seen with issue #11079. In order to avoid some images to simply disappear from PDF, perhaps I will need to add to the output latex mark-up a dummy paragraph via a \mbox{} (as I am not sure I master enough of DocUtils nodes and Sphinx to identify if what comes after the figure inclusion is or not a text paragraph, and not to mention potential interactions with extensions). Perhaps I will undo the extra induced vertical space via some negative \vskip but experience tells me I may not be able to maintain exact same layout as in current PDF produced via make latexpdf. If this possible layout change is not a breaking one, then I can work on a PR in time for the 6.2.0 milestone which I have assigned to this issue. Else if it is a change triggering an X.0 release, then it makes it a bit more blurry when I will need to work on this, as I am not able to supervise myself the PyPi release process, due to a too limited mastery of Python, particularly regarding linting, but not only that, and as it would feel very pretentious to say "hey guys, I made this somewhat breaking LaTeX change, time for you to devote urgently some time and make a major release".

@benjaoming
Copy link
Contributor

@AA-Turner just noticed that this was scheduled for 6.2.0 -- these are just suggestions if it makes life easier...

image

if this is a deprecation warning that's only by means of documentation, then it's probably better to put the deprecation warning in the documentation of these features. If the features do not have documentation and/or documentation warnings cannot be issued during builds, then I think it might be wise to have a list "Upcoming deprecations" at the very top of the CHANGES.rst or as a separate list referenced clearly in CHANGES.rst.

if you want to save the minor version bump, I would say that it's perfectly fine to introduce deprecations in patch releases. In fact, the earlier the better.

@jfbu
Copy link
Contributor Author

jfbu commented Jan 8, 2023

@benjaoming,

if this is a deprecation warning that's only by means of documentation, then it's probably better to put the deprecation warning in the documentation of these features. If the features do not have documentation and/or documentation warnings cannot be issued during builds, then I think it might be wise to have a list "Upcoming deprecations" at the very top of the CHANGES.rst or as a separate list referenced clearly in CHANGES.rst.

In the case at hand it is not so much the features but some underlying LaTeX code to implement them. TBH I feel I should not have made such a big fuss of it and avoid this prominent notice to start with, and silently remove the LaTeX things (already at 5.1.0). It can only break people who would have read the LaTeX code and decided to use directly such or such thing. It is a clash of culture here, because in world of LaTeX packages these things happen all the time and here I had made the mistake some years back to use names with no scary character such as @ in their names, so although documented only in the source code, they would have been deemed semi-public in LaTeX world.

if you want to save the minor version bump, I would say that it's perfectly fine to introduce deprecations in patch releases. In fact, the earlier the better.

At that time I did not know yet that a 6.1.2 would indeed be released, and I did not feel like adding such a notice to the patch release. But yes, you are right that once the patch release is known to happen we can take advantage to announce deprecations.

I feel I would have saved lots of noise if I had simply merged this PR immediately and not added the announcement from CHANGES that you quoted. It has never been said that people could use any piece of the LaTeX code and hope for no changes to break it. And 99,999% Sphinx users are not going to hack into the LaTeX code. The LaTeX code is not part of the Sphinx API. But there are a few notable extensions which do things spatialaudio/nbsphinx with LaTeX and in the past we have taken some steps to maintain old interface for compatibility.

If nobody objects for me merging this immediately and remove the prominent notice you quoted, I am all in favour of it. Besides those who read the LaTeX code have seen the deprecation mentioned in it since 5.1.0.

@jfbu
Copy link
Contributor Author

jfbu commented Jan 8, 2023

@AA-Turner I am thinking of merging this immediately in 6.x (with some changes so that the LaTeX code comments refer to 6.2.0 not 7.0.0) and reformulate the 6.2.0 CHANGES entry to be much shorter and simply say "hey folks, we have removed internal LaTeX stuff that you were not really allowed to use directly anyhow, so don't come complain".

@AA-Turner
Copy link
Member

Sounds good!

A

@jfbu
Copy link
Contributor Author

jfbu commented Jan 8, 2023

Sounds good!

great, I will merge after testing completes. I hope it is ok if I do a merge commit... we did that formerly and it helps keep my git with less dangling shas

@jfbu jfbu merged commit e5bdb93 into sphinx-doc:master Jan 8, 2023
@jfbu jfbu deleted the latex_remove_dimens branch January 8, 2023 19:04
@jfbu
Copy link
Contributor Author

jfbu commented Jan 8, 2023

@AA-Turner this is very strange I could swear the github interface told me all tests were ok and only later did I see that Ruff (now at 0.0.215) had complained...(I must be delusional) I merged without seeing the red cross...

@jfbu
Copy link
Contributor Author

jfbu commented Jan 8, 2023

@AA-Turner this is very strange I could swear the github interface told me all tests were ok and only later did I see that Ruff (now at 0.0.215) had complained...(I must be delusional) I merged without seeing the red cross...

The red cross had become a green cross, I am dreaming awake ! Time for a break !

@AA-Turner
Copy link
Member

The interface is recording Ruff as having passed for me...

A

@AA-Turner
Copy link
Member

The red cross had become a green cross, I am dreaming awake ! Time for a break !

No worries :) Thank you!

A

@jfbu jfbu modified the milestones: 7.0.0, 6.2.0 Jan 8, 2023
@jfbu
Copy link
Contributor Author

jfbu commented Jan 8, 2023

@AA-Turner only for the record,

  • the github interface did report failed CI from the branch at my fork: https://github.com/jfbu/sphinx/actions/runs/3868499141 (not sure if you can see it; I have included below a partial screenshot done after a re-launch of the failed job)
  • the github interface turned to all green after CI completed and I merged
  • a few seconds later I saw that my commit had a red cross and that Ruff had failed (I suspect this is because when it is run on my fork it does not have the exceptions from this repo).
  • then again a few seconds later and the red cross had transmogrified into a green checkmark here!
  • so I take a break, watch a few Star Trek movies, and now I have asked github to run the test again at my fork and it fails again at "Attempt #2" (same link as above)
    Capture d’écran 2023-01-08 à 22 31 31
  • now I thought, maybe I should push master and 6.1.x to my fork, it might have to do with this mystery. And lo and behold, I receive in my mailbox that linting has failed for 6.1.x and here is a screenshot:
    Capture d’écran 2023-01-08 à 22 34 37
    Compare with screenshot on this repo:
    Capture d’écran 2023-01-08 à 22 34 26
  • third attempt at my fork behaves the same (Ruff 0.0.215):
    Capture d’écran 2023-01-08 à 22 41 59

Maybe because I forked sphinx-doc/sphinx so long ago something is ill-configured, but it does looks as if CI proceeds on it differently. The most curious thing is when I saw a red cross here on this page, and seconds later it had become a green checkmark! It is as if Github was running two separate CIs at same time, the first one in an environment from my fork, the other one here...

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2023
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.

None yet

3 participants