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.

Sign up for GitHub

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

Jump to bottom

.github/workflows/doc-build.yml: Fix generation of CHANGES.html #35808

Merged
merged 4 commits into from
Jul 1, 2023
Merged

.github/workflows/doc-build.yml: Fix generation of CHANGES.html #35808

merged 4 commits into from
Jul 1, 2023

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Jun 22, 2023 •
edited

📚 Description

This new feature from #35652 was supposed to show the changed HTML files,
but a last minute change there destroyed it.

Here we fix it.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • 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 accordingly.

⌛ Dependencies

@mkoeppe mkoeppe requested a review from kwankyu June 22, 2023 21:39
@mkoeppe mkoeppe self-assigned this Jun 22, 2023
kwankyu
kwankyu reviewed Jun 23, 2023
View reviewed changes
.github/workflows/doc-build.yml
mv /sage/local/share/doc/sage/html/en/.git /sage/.git-doc
make doc-clean doc-uninstall sagelib-clean && git clean -fx src/sage
mkdir -p /sage/local/share/doc/sage/html/en/ && mv /sage/.git-doc /sage/local/share/doc/sage/html/en/
./config.status && make doc-html
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move .git-doc to .git-doc? No .git?

Where is the code to generate CHANGES.html? No idea how this works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed now.

CHANGES.html is generated in the "Copy docs" step

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you aware that L53 is very similar to L100 in the "Copy docs" step. This seems to be a mistake to me...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, you are.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the net effect of L83 and L85. It is moving .git directory to and from .gitdoc. Why?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In particular, why mkdir -p /sage/local/share/doc/sage/html/en/. The directory already exists. Why create it again?

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'm moving the .git directory away and back to protect it from make doc-uninstall

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK.

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 24, 2023

Most of the files in changes in the documentation preview of this PR still seem spurious. There should be none?

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 24, 2023

I think they are all caused by functions whose default arguments display a hex address

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 24, 2023

I don't know if wiping out sage version from the sidebar is a good idea...

If I would change it at all and if possible, then I would augment the version number with the PR number, like Sage 10.1.beta4#12345 from Sage 10.1.beta4

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 24, 2023

By second thought, I would not change the version number at all because checking if the version number appear correct in the documentation is itself a part of the purpose of the documentation preview.

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 24, 2023

I think they are all caused by functions whose default arguments display a hex address

Yeah. I see.

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 24, 2023 •
edited

And how about providing diff separately for each changed html, instead of one giant diff HTML.diff?

If there is a free API service that show git diff nicely, then we may use that...

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 24, 2023

I would not change the version number at all because checking if the version number appear correct in the documentation is itself a part of the purpose of the documentation preview.

Without making the version number equal between both versions, every single file would show up in the diff

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 25, 2023

Both should show the same latest beta version. No? But I guess there could be a mismatch, for example, for some short time before the latest beta appears in the docker image, or when the branch of a PR is not rebased onto the latest beta. Is your making-version-number-equal a provision for these situations?

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 25, 2023 •
edited

Yes, that's right, it's intended for these situations. I don't think they are uncommon.

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 25, 2023 •
edited

I agree that they are common. But if the author recognizes the version mismatch, then he/she can just upload the branch merged with the latest beta, which I think is, anyway, a good thing to do. Also this is just optional.

On the other hand, we frequently create links to the doc built for a PR, then it is good that the doc is accompanied with the right beta version number.

I am not sure about this matter though. I would not insist for now.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 25, 2023

I'll change it so that the old copy gets updated with the version number of the PR branch.

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 25, 2023

Thanks. It seems a good compromise.

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 25, 2023 •
edited

This is of course a wishlist item.

And how about providing diff separately for each changed html, instead of one giant diff HTML.diff?

If there is a free API service that show git diff nicely, then we may use that...

There is https://diffy.org/. But it seems not work with a url.

@github-actions
Copy link

Documentation preview for this PR (built with commit ebde730; changes) is ready! 🎉

kwankyu
kwankyu approved these changes Jun 25, 2023
View reviewed changes
Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

It works now.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 25, 2023

Thanks for the review!

@tobiasdiez
Copy link
Contributor

This is of course a wishlist item.

And how about providing diff separately for each changed html, instead of one giant diff HTML.diff?
If there is a free API service that show git diff nicely, then we may use that...

There is https://diffy.org/. But it seems not work with a url.

It wouldn't be too hard to use https://diff2html.xyz/

@vbraun vbraun merged commit 404d9e5 into sagemath:develop Jul 1, 2023
16 of 17 checks passed
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jul 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwankyu kwankyu kwankyu approved these changes

Assignees

@mkoeppe mkoeppe

Labels
p: critical / 2
Projects
None yet
Milestone
sage-10.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mkoeppe @kwankyu @tobiasdiez @vbraun