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

Make doc-pdf separate from doc-html #36692

Merged
merged 5 commits into from
Nov 14, 2023
Merged

Conversation

kwankyu
Copy link
Collaborator

@kwankyu kwankyu commented Nov 10, 2023

(1) Fixes the issue in #36614 (comment) by making doc-pdf target separate from doc-html target.

(2) Support .. ONLY:: (introduced by #36495) in generating rst files for sage modules by the reference builder.

(3) Edited the pdf docs website to look consistent and tidy.

📝 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

@kwankyu kwankyu changed the title reference-top is built after sub docs Build reference_top after sub docs of reference manual Nov 10, 2023
@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 10, 2023

From #36614 (comment), it seems like the PDF docbuild is still building HTML documentation?

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 10, 2023

I think I have a fix for that in #36498 (87e40a3), please feel free to cherry-pick if you agree

@kwankyu
Copy link
Collaborator Author

kwankyu commented Nov 11, 2023

I think I have a fix for that in #36498 (87e40a3), please feel free to cherry-pick if you agree

OK. But that may be intentional. Let's see.

@kwankyu kwankyu marked this pull request as ready for review November 11, 2023 04:26
@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 11, 2023

I think sage_docbuild.ext.multidocs needs to be changed so it does not do HTML things when generating Latex/PDF

@kwankyu
Copy link
Collaborator Author

kwankyu commented Nov 11, 2023

I think sage_docbuild.ext.multidocs needs to be changed so it does not do HTML things when generating Latex/PDF

The reference pdf builder was building reference html doc to create a master index.html for generated pdf docs. This is no problem if html docs are built before pdf docs. But as the pdf workflow only builds pdf docs, this break things.

I fixed it just now. Let's see.

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 11, 2023

PDF build job succeeds, but the HTML build job now fails https://github.com/sagemath/sage/actions/runs/6834646326/job/18587631286?pr=36692#step:10:2680

  [sagemath_doc_html-none]   File "/sage/src/sage_docbuild/__main__.py", line 508, in <module>
  [sagemath_doc_html-none]     sys.exit(main())
  [sagemath_doc_html-none]              ^^^^^^
  [sagemath_doc_html-none]   File "/sage/src/sage_docbuild/__main__.py", line 504, in main
  [sagemath_doc_html-none]     builder()
  [sagemath_doc_html-none]   File "/sage/src/sage_docbuild/builders.py", line 445, in html
  [sagemath_doc_html-none]     shutil.copytree(src, dst)
  [sagemath_doc_html-none]   File "/sage/local/var/lib/sage/venv-python3.11.1/lib/python3.11/shutil.py", line 559, in copytree
  [sagemath_doc_html-none]     with os.scandir(src) as itr:
  [sagemath_doc_html-none]          ^^^^^^^^^^^^^^^
  [sagemath_doc_html-none] FileNotFoundError: [Errno 2] No such file or directory: '/sage/local/share/doc/sage/html/en/website/_static'
  [sagemath_doc_html-none] 
  [sagemath_doc_html-none]     Note: incremental documentation builds sometimes cause spurious
  [sagemath_doc_html-none]     error messages. To be certain that these are real errors, run
  [sagemath_doc_html-none]     "make doc-clean doc-uninstall" first and try again.
  [sagemath_doc_html-none] make[3]: *** [Makefile:32: doc-html--website] Error 1

@kwankyu
Copy link
Collaborator Author

kwankyu commented Nov 11, 2023

Yes. I know.

Before, pdf docs were always built after html docs were built. That is how the code is organized. Now, for CI, we want to build pdf docs separately. This opens the can of worms...

But we are almost there.

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 12, 2023

Looking good now, from a brief look at the PDFs in the generated artifact and the HTML diff.

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 12, 2023

(2) Support .. ONLY:: (introduced by #36495) in generating rst files for sage modules.

Could you elaborate on that? (Here or in the developer's guide)

@kwankyu
Copy link
Collaborator Author

kwankyu commented Nov 12, 2023

In src/doc/en/reference/polynomial_rings/index.rst, there is


Boolean Polynomials
-------------------

.. ONLY:: feature_sage_rings_polynomial_pbori

   .. toctree::
      :maxdepth: 1

      sage/rings/polynomial/pbori/pbori

.. include:: ../footer.txt    

.. ONLY:: directive tells sphinx to not process the block. But Sage doc builders in src/sage_docbuild/builders.py does not know this. The reference builder scans the index file to make up the list of sage modules for each of which an rst file is auto-generated into src/doc/reference/polynomial_rings/sage/.... Since the builder does not know how to deal with .. ONLY::, it processes sage/rings/polynomial/pbori/pbori and generates the corresponding rst file. This causes a problem since there is no known item sage/rings/polynomial/pbori/pbori in the index file. I fixed the reference builder process .. ONLY:: directive correctly.

I don't think this explanation needs to go into the developer manual.

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 12, 2023

Thanks for the explanation, and for the fix!

I agree, no need to add to the manual.

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 12, 2023

Ready for review?

@kwankyu
Copy link
Collaborator Author

kwankyu commented Nov 12, 2023

I am testing the last commit on my fork.

Confused as already pushed to develop...

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 12, 2023

Confused as already pushed to develop...

Volker just force-pushed develop, as announced in https://groups.google.com/g/sage-release/c/iCofn2lztjY/m/Ka_T1juZCQAJ; strangely the release tag is still in the old place

@kwankyu kwankyu changed the title Build reference_top after sub docs of reference manual Make doc-pdf separate from making doc-html Nov 12, 2023
@kwankyu kwankyu changed the title Make doc-pdf separate from making doc-html Make doc-pdf separate from doc-html Nov 12, 2023
@kwankyu
Copy link
Collaborator Author

kwankyu commented Nov 12, 2023

OK. This is good to go. I tried to include pdf docs to the live doc preview but gave up for now.

Copy link
Contributor

@mkoeppe mkoeppe left a comment

Choose a reason for hiding this comment

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

LGTM.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Nov 12, 2023

Thanks!

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 12, 2023

Thanks a lot for this work. This is a great step towards #29868

@kwankyu
Copy link
Collaborator Author

kwankyu commented Nov 12, 2023

Sorry, but I found a defect that needs some work.

This is still good to fix the pdf doc workflow. So leave it as blocker.

@kwankyu
Copy link
Collaborator Author

kwankyu commented Nov 12, 2023

Now make doc-pdf correctly builds the pdf docs website if run after make doc-html.

vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 12, 2023
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

(1) Fixes the issue in
sagemath#36614 (comment) by
making doc-pdf target  separate from doc-html target.

(2) Support `.. ONLY::` (introduced by sagemath#36495) in generating rst files
for sage modules by the reference builder.

(3) Edited the pdf docs website to look consistent and tidy.

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

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

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36692
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
Copy link

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

@vbraun vbraun merged commit 613775f into sagemath:develop Nov 14, 2023
15 of 17 checks passed
@kwankyu kwankyu mentioned this pull request Nov 28, 2023
5 tasks
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 7, 2023
    
Currently live doc preview lacks pdf docs though links to them are
visible, as seen

https://deploy-livedoc--sagemath-tobias.netlify.app/

We fix the problem. The live doc preview built with this PR is here:

https://deploy-livedoc--sagemath.netlify.app/

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

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

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

- sagemath#36692

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36714
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 9, 2023
    
Currently live doc preview lacks pdf docs though links to them are
visible, as seen

https://deploy-livedoc--sagemath-tobias.netlify.app/

We fix the problem. The live doc preview built with this PR is here:

https://deploy-livedoc--sagemath.netlify.app/

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

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

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

- sagemath#36692

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36714
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
@kwankyu kwankyu deleted the fix-doc-build branch December 13, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants