Fix CI documentation diff #39622

user202729 · 2025-03-03T03:37:01Z

As discussed in #39542 . Without this, the "Changes is ready!" feature will be broken.

Also remove some outdated documentation (the requirement list in update-conda was wrong (toml missing), I just refer to README.md which contains the correct list)

📝 Checklist

The title is concise and informative.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation and checked the documentation preview.

⌛ Dependencies

user202729 · 2025-03-03T03:40:37Z

tools/clean-doc-html

+    -e 's;#L[0-9]*";";' \
+    -e 's;tab-set--[0-9]*-;tab-set-;' \
+    -e 's;"tab-set--[0-9]*";"tab-set";' \
+    "$@"


Not sure if this is more suitable to be put in .ci instead. On the other hand it seems like something that is not that specific to CI?

I suggest to put it in .github/workflows. tools is for developers (I have a hard time seeing why someone would like to run this script locally) and .ci is mostly outdated.

Oops, I misreads.

On the other hand

.ci is mostly outdated.

the problem is the substitute aren't good enough yet. For example test-new takes only ≈ 15 minutes while meson test takes an hour. Meson doesn't have incremental build or documentation build or documentation diff either.

I suggest to put it in .github/workflows.

Actually… this feels wrong somehow.

Argument to support it: there are already two scripts there.

Arguments against it: we're only using GitHub actions for CI, so GitHub workflows and CI should be synonymous. It is not a good idea to have two locations for the same purpose. (if we do this we might consider moving the scripts back as well, but I guess that's out of scope here)

What do you think?

In #39642, I propose to remove the .ci directory. What do you think?

user202729 · 2025-03-03T03:56:41Z

.github/workflows/doc-build.yml

-                                                  -e 's;#L[0-9]*";";' \
-                                                  -e 's;tab-set--[0-9]*-;tab-set-;' \
-             && true)
+             find . -name "*.html" | xargs ../tools/clean-doc-html && true)


The true seems useless, but I keep it just in case.

github-actions · 2025-03-03T04:45:21Z

Documentation preview for this PR (built with commit 3b83e4b; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

user202729 · 2025-03-03T06:14:33Z

I wonder if marking it as CI Fix will work… problem is GitHub reads the yml files before applying the changes (otherwise how can it know what to run to apply the changes in the first place), right.

On the slightly-less-negative side we only waste 13 additional seconds of CPU time per pull request.

tobiasdiez · 2025-03-03T12:55:08Z

Also move some documentation closer to the source.

Not sure if I like this change or not. The rendering of code in markdown (eg here on github) is usually way better than in the docstrings in python files.

user202729 · 2025-03-03T15:16:55Z

The rendering of code in markdown (eg here on github) is usually way better than in the docstrings in python files.

That's true, but in principle that's only a technical issue rather than a practical one (e.g. normally there is Sphinx to render RST to HTML) In practice… either way is fine for me.

tobiasdiez

From my side this looks good. @kwankyu can you please have a look as well given your expertise in the doc-changeset business.

user202729 · 2025-03-16T11:09:09Z

@kwankyu Do you have any comment? (I think we show not leave "view changes" broken for too long, yes it's my fault)

For what it's worth the changes link for this PR (https://doc-pr-39622--sagemath.netlify.app/CHANGES.html) is working. (the page is empty though.)

kwankyu · 2025-03-16T23:15:24Z

Bash scripts like ".github/workflows/clean-doc-html.sh" used for CI go into the .ci directory.

General remark: The sed command has been carefully improved and battle-tested. Please test your changes to check for regressions.

user202729 · 2025-03-17T01:17:10Z

Bash scripts like ".github/workflows/clean-doc-html.sh" used for CI go into the .ci directory.

Uh, it's because of #39642 . I guess I can move it back (then maybe the other pull request if merged after this one can move it to .github/workflows again or something)

General remark: The sed command has been carefully improved and battle-tested. Please test your changes (on your fork) to check for regressions.

The command is not changed, just moved from one file to another to remove code duplication.

kwankyu · 2025-03-17T01:29:11Z

The command is not changed, just moved from one file to another to remove code duplication.

The two sed commands are not identical.

user202729 · 2025-03-17T01:34:10Z

The command is not changed, just moved from one file to another to remove code duplication.

The two sed commands are not identical.

That's the point of this pull request, because of the code duplication I accidentally change one without change the other in #39542 (it was previously identical) which breaks the feature, so I decide to remove the code duplication to avoid possible issues in the future. (and in the meanwhile make the two commands identical again, obviously)

kwankyu · 2025-03-17T01:39:50Z

... in #39542 (it was previously identical)

They were different in #39542.

user202729 · 2025-03-17T01:45:16Z

Before #39542 :

            # mathjax path in old doc (regex)
            mathjax_path_from="[-./A-Za-z_]*/tex-chtml[.]js?v=[0-9a-f]*"
            # mathjax path in new doc
            mathjax_path_to=$(docker exec -e SAGE_USE_CDNS=yes BUILD /sage/sage -python -c "from sage_docbuild.conf import mathjax_path; print(mathjax_path)")
            new_version=$(docker exec BUILD cat src/VERSION.txt)
            # Wipe out chronic diffs between old doc and new doc
            (cd doc && \
             find . -name "*.html" | xargs sed -i -e '/class="sidebar-brand-text"/ s/Sage [0-9a-z.]* /Sage '"$new_version"' /' \
                                                  -e 's;?v=[0-9a-f]*";";' \
                                                  -e 's;'"$mathjax_path_from"';'"$mathjax_path_to"';' \
                                                  -e '\;<script type="application/vnd\.jupyter\.widget-state+json">;,\;</script>; d' \
                                                  -e 's;#L[0-9]*";";' \
                                                  -e 's;tab-set--[0-9]*-;tab-set-;' \

versus

            (cd doc && git add -A && git commit --quiet -m 'new')
            # Wipe out chronic diffs of new doc against old doc before creating CHANGES.html
            (cd doc && \
             find . -name "*.html" | xargs sed -i -e '/This is documentation/ s/ built with GitHub PR .* for development/ for development/' \
                                                  -e 's;?v=[0-9a-f]*";";' \
                                                  -e '\;<script type="application/vnd\.jupyter\.widget-state+json">;,\;</script>; d' \
                                                  -e 's;#L[0-9]*";";' \
                                                  -e 's;tab-set--[0-9]*-;tab-set-;' \
             && git commit -a -m 'wipe-out')

… okay yes they're different. I can't say I fully understand what they do so I'll just try my best to preserve the old behavior while removing code duplication.

kwankyu · 2025-03-17T01:59:48Z

Before making diffs, the first sed command removes things specific to the doc for the last sage release, and the second sed command removes things specific to the doc for the PR branch. They cannot be the same.

user202729 · 2025-03-17T09:20:18Z

I decide to just keep the old situation and fix both occurrences. Refactor etc. can be left for some day in the future.

Seems like it's working now — I previously tried adding a code block and there isn't several spurious tab-set diffs shown up.

kwankyu

LGTM.

sagemathgh-39622: Fix CI documentation diff As discussed in sagemath#39542 . Without this, the "Changes is ready!" feature will be broken. Also remove some outdated documentation (the requirement list in update- conda was wrong (toml missing), I just refer to README.md which contains the correct list) ### 📝 Checklist  - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies    URL: sagemath#39622 Reported by: user202729 Reviewer(s): Kwankyu Lee, Tobias Diez, user202729

github-actions bot added the s: needs review label Mar 3, 2025

user202729 force-pushed the docdiff-ci-refactor branch from 1f10bb6 to 2b1815d Compare March 3, 2025 03:38

user202729 commented Mar 3, 2025

View reviewed changes

user202729 mentioned this pull request Mar 3, 2025

Hide more chronic diff on CI #39542

Merged

5 tasks

user202729 force-pushed the docdiff-ci-refactor branch from 2b1815d to 8c2fff5 Compare March 3, 2025 03:56

user202729 commented Mar 3, 2025

View reviewed changes

user202729 added the p: CI Fix merged before running CI tests label Mar 3, 2025

user202729 changed the title ~~Refactor some tools and fix CI documentation diff~~ Fix CI documentation diff Mar 4, 2025

tobiasdiez approved these changes Mar 7, 2025

View reviewed changes

user202729 removed the p: CI Fix merged before running CI tests label Mar 12, 2025

user202729 marked this pull request as draft March 17, 2025 01:48

github-actions bot removed the s: needs review label Mar 17, 2025

user202729 force-pushed the docdiff-ci-refactor branch from 749f874 to f93e1ef Compare March 17, 2025 09:17

user202729 marked this pull request as ready for review March 17, 2025 09:17

github-actions bot added the s: needs review label Mar 17, 2025

Refactor some tools and fix CI documentation diff

3b83e4b

user202729 force-pushed the docdiff-ci-refactor branch from f93e1ef to 3b83e4b Compare March 17, 2025 09:19

kwankyu approved these changes Mar 17, 2025

View reviewed changes

kwankyu added s: positive review and removed s: needs review labels Mar 17, 2025

vbraun merged commit 185fd5f into sagemath:develop Mar 22, 2025
23 of 24 checks passed

github-actions bot removed the s: positive review label Mar 22, 2025

user202729 mentioned this pull request Mar 25, 2025

Renamed vector subspace complement to orthogonal_complement and changed the complement function #39659

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CI documentation diff #39622

Fix CI documentation diff #39622

user202729 commented Mar 3, 2025 •

edited

Loading

user202729 Mar 3, 2025

tobiasdiez Mar 3, 2025

user202729 Mar 4, 2025

user202729 Mar 4, 2025 •

edited

Loading

tobiasdiez Mar 7, 2025

user202729 Mar 3, 2025

github-actions bot commented Mar 3, 2025 •

edited

Loading

user202729 commented Mar 3, 2025 •

edited

Loading

tobiasdiez commented Mar 3, 2025

user202729 commented Mar 3, 2025

tobiasdiez left a comment

user202729 commented Mar 16, 2025

kwankyu commented Mar 16, 2025 •

edited

Loading

user202729 commented Mar 17, 2025

kwankyu commented Mar 17, 2025

user202729 commented Mar 17, 2025

kwankyu commented Mar 17, 2025

user202729 commented Mar 17, 2025

kwankyu commented Mar 17, 2025

user202729 commented Mar 17, 2025 •

edited

Loading

kwankyu left a comment

Fix CI documentation diff #39622

Fix CI documentation diff #39622

Conversation

user202729 commented Mar 3, 2025 • edited Loading

📝 Checklist

⌛ Dependencies

user202729 Mar 3, 2025

Choose a reason for hiding this comment

tobiasdiez Mar 3, 2025

Choose a reason for hiding this comment

user202729 Mar 4, 2025

Choose a reason for hiding this comment

user202729 Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

tobiasdiez Mar 7, 2025

Choose a reason for hiding this comment

user202729 Mar 3, 2025

Choose a reason for hiding this comment

github-actions bot commented Mar 3, 2025 • edited Loading

user202729 commented Mar 3, 2025 • edited Loading

tobiasdiez commented Mar 3, 2025

user202729 commented Mar 3, 2025

tobiasdiez left a comment

Choose a reason for hiding this comment

user202729 commented Mar 16, 2025

kwankyu commented Mar 16, 2025 • edited Loading

user202729 commented Mar 17, 2025

kwankyu commented Mar 17, 2025

user202729 commented Mar 17, 2025

kwankyu commented Mar 17, 2025

user202729 commented Mar 17, 2025

kwankyu commented Mar 17, 2025

user202729 commented Mar 17, 2025 • edited Loading

kwankyu left a comment

Choose a reason for hiding this comment

user202729 commented Mar 3, 2025 •

edited

Loading

user202729 Mar 4, 2025 •

edited

Loading

github-actions bot commented Mar 3, 2025 •

edited

Loading

user202729 commented Mar 3, 2025 •

edited

Loading

kwankyu commented Mar 16, 2025 •

edited

Loading

user202729 commented Mar 17, 2025 •

edited

Loading