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

.github/workflows/doc-build-pdf.yml: Do not build HTML documentation, fix upload of PDFs #36614

Merged
merged 8 commits into from
Nov 5, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Oct 31, 2023

Our make target doc-pdf depends on doc-html only to avoid concurrency bugs when updating the inventory files.

To speed up the Build Documentation (PDF) workflow, we skip the HTML build.

📝 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
Copy link
Collaborator

kwankyu commented Nov 1, 2023

Where can I see the built pdf docs?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 1, 2023

The upload step was being skipped. Fixed now, it should be available as an artifact.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 1, 2023

(I've cherry-picked this last commit from #36508.)

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.

LGTM.

@mkoeppe mkoeppe changed the title .github/workflows/doc-build-pdf.yml: Do not build HTML documentation .github/workflows/doc-build-pdf.yml: Do not build HTML documentation, fix upload of PDFs Nov 1, 2023
@kwankyu
Copy link
Collaborator

kwankyu commented Nov 1, 2023

On the other hand, it would be nice to publish the built pdf doc to the same place where html doc is published so that html and pdf docs are available together in the doc preview. pdf docs may be published later after html docs so that developers can examine html docs first. But I am not sure if Tobias' netlify account allows that much space...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 1, 2023

I agree that it would be nice, but I have no experience with the netlify upload, so if at all possible, it will have to be done in a separate PR.

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 1, 2023

Yes, of course.

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 1, 2023

@tobiasdiez ?

@vbraun
Copy link
Member

vbraun commented Nov 1, 2023

merge conflict

@tobiasdiez
Copy link
Contributor

On the other hand, it would be nice to publish the built pdf doc to the same place where html doc is published so that html and pdf docs are available together in the doc preview. pdf docs may be published later after html docs so that developers can examine html docs first. But I am not sure if Tobias' netlify account allows that much space...

Space is not the issue, but I don't think its possible to only add the pdf later (so you would need to download the build artifact from the html build, add the pdf, then upload everything).

Copy link

github-actions bot commented Nov 2, 2023

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

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 2, 2023

On the other hand, it would be nice to publish the built pdf doc to the same place where html doc is published so that html and pdf docs are available together in the doc preview. pdf docs may be published later after html docs so that developers can examine html docs first. But I am not sure if Tobias' netlify account allows that much space...

Space is not the issue, but I don't think it's possible to only add the pdf later (so you would need to download the build artifact from the html build, add the pdf, then upload everything).

It may not be worth while for PRs. But for pushes to develop, it may be worth another workflow. After #36601, the netlify site will have this

html/(en/...)
index.html

By the new workflow (perhaps doc-publish-pdf), it would be

html/(en/...)
pdf/
index.html

The two workflows should publish to the same url, of course.

@vbraun vbraun merged commit 9f448dd into sagemath:develop Nov 5, 2023
@mkoeppe mkoeppe added this to the sage-10.2 milestone Nov 5, 2023
@kwankyu
Copy link
Collaborator

kwankyu commented Nov 10, 2023

I suspect that the change of this PR

-make doc-clean doc-uninstall; make doc-pdf
+make doc-clean doc-uninstall; make sagemath_doc_html-build-deps sagemath_doc_pdf-no-deps

triggered failures like https://github.com/sagemath/sage/actions/runs/6811957358/job/18523428561?pr=36592

From the log,

2023-11-09T13:28:20.2254069Z [sagemath_doc_pdf-none] [reference] WARNING: Unable to fetch /sage/local/share/doc/sage/html/en/reference/schemes/searchindex.js
2023-11-09T13:28:20.2254851Z [sagemath_doc_pdf-none] [reference]     semirings:
2023-11-09T13:28:20.2255695Z [sagemath_doc_pdf-none] [reference] WARNING: Unable to fetch /sage/local/share/doc/sage/html/en/reference/semirings/searchindex.js
2023-11-09T13:28:20.2256474Z [sagemath_doc_pdf-none] [reference]     sets:
2023-11-09T13:28:20.2257257Z [sagemath_doc_pdf-none] [reference] WARNING: Unable to fetch /sage/local/share/doc/sage/html/en/reference/sets/searchindex.js
2023-11-09T13:28:20.2258166Z [sagemath_doc_pdf-none] [reference]     spkg: 4399 js index entries
2023-11-09T13:28:20.2258671Z [sagemath_doc_pdf-none] [reference]     stats:
2023-11-09T13:28:20.2259469Z [sagemath_doc_pdf-none] [reference] WARNING: Unable to fetch /sage/local/share/doc/sage/html/en/reference/stats/searchindex.js
2023-11-09T13:28:20.2260236Z [sagemath_doc_pdf-none] [reference]     structure:
2023-11-09T13:28:20.2261068Z [sagemath_doc_pdf-none] [reference] WARNING: Unable to fetch /sage/local/share/doc/sage/html/en/reference/structure/searchindex.js
2023-11-09T13:28:20.2261978Z [sagemath_doc_pdf-none] [reference]     tensor_free_modules: 1379 js index entries
2023-11-09T13:28:20.2262631Z [sagemath_doc_pdf-none] [reference]     topology: 2090 js index entries
2023-11-09T13:28:20.2263165Z [sagemath_doc_pdf-none] [reference]     valuations:
2023-11-09T13:28:20.2264011Z [sagemath_doc_pdf-none] [reference] WARNING: Unable to fetch /sage/local/share/doc/sage/html/en/reference/valuations/searchindex.js
2023-11-09T13:28:20.2264897Z [sagemath_doc_pdf-none] [reference] ... done (65918 js index entries)
2023-11-09T13:28:20.2265577Z [sagemath_doc_pdf-none] [reference] dumping search index in English (code: en)... done
2023-11-09T13:28:20.2266410Z [sagemath_doc_pdf-none] [reference] The HTML pages are in ../../local/share/doc/sage/html/en/reference.
2023-11-09T13:28:20.2267178Z [sagemath_doc_pdf-none] Error building the documentation.
2023-11-09T13:28:20.2267697Z [sagemath_doc_pdf-none] Traceback (most recent call last):
2023-11-09T13:28:20.2268302Z [sagemath_doc_pdf-none]   File "<frozen runpy>", line 198, in _run_module_as_main
2023-11-09T13:28:20.2268941Z [sagemath_doc_pdf-none]   File "<frozen runpy>", line 88, in _run_code
2023-11-09T13:28:20.2269716Z [sagemath_doc_pdf-none]   File "/sage/pkgs/sage-docbuild/sage_docbuild/__main__.py", line 508, in <module>
2023-11-09T13:28:20.2270366Z [sagemath_doc_pdf-none]     sys.exit(main())
2023-11-09T13:28:20.2270760Z [sagemath_doc_pdf-none]              ^^^^^^
2023-11-09T13:28:20.2271399Z [sagemath_doc_pdf-none]   File "/sage/pkgs/sage-docbuild/sage_docbuild/__main__.py", line 504, in main
2023-11-09T13:28:20.2272000Z [sagemath_doc_pdf-none]     builder()
2023-11-09T13:28:20.2272626Z [sagemath_doc_pdf-none]   File "/sage/pkgs/sage-docbuild/sage_docbuild/builders.py", line 645, in pdf
2023-11-09T13:28:20.2273394Z [sagemath_doc_pdf-none]     getattr(get_builder('reference_top'), 'html')()
2023-11-09T13:28:20.2274154Z [sagemath_doc_pdf-none]   File "/sage/pkgs/sage-docbuild/sage_docbuild/builders.py", line 162, in f
2023-11-09T13:28:20.2274754Z [sagemath_doc_pdf-none]     runsphinx()
2023-11-09T13:28:20.2275429Z [sagemath_doc_pdf-none]   File "/sage/pkgs/sage-docbuild/sage_docbuild/sphinxbuild.py", line 327, in runsphinx
2023-11-09T13:28:20.2276128Z [sagemath_doc_pdf-none]     sys.stderr.raise_errors()
2023-11-09T13:28:20.2276945Z [sagemath_doc_pdf-none]   File "/sage/pkgs/sage-docbuild/sage_docbuild/sphinxbuild.py", line 263, in raise_errors
2023-11-09T13:28:20.2277660Z [sagemath_doc_pdf-none]     raise OSError(self._error)
2023-11-09T13:28:20.2278590Z [sagemath_doc_pdf-none] OSError: WARNING: Unable to fetch /sage/local/share/doc/sage/html/en/reference/history_and_license/searchindex.js 
2023-11-09T13:28:20.2279460Z [sagemath_doc_pdf-none] 
2023-11-09T13:28:20.2279989Z [sagemath_doc_pdf-none]     Note: incremental documentation builds sometimes cause spurious
2023-11-09T13:28:20.2280765Z [sagemath_doc_pdf-none]     error messages. To be certain that these are real errors, run
2023-11-09T13:28:20.2281481Z [sagemath_doc_pdf-none]     "make doc-clean doc-uninstall" first and try again.

@mkoeppe mkoeppe deleted the ci_docbuild_pdf_no_html branch November 10, 2023 15:54
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 12, 2023
sagemathgh-36692: Make doc-pdf separate from doc-html
    
<!-- ^^^^^
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
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 14, 2023
sagemathgh-36692: Make doc-pdf separate from doc-html
    
<!-- ^^^^^
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
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 16, 2023
sagemathgh-36617: CI: Make jobs more responsive to canceling
    
... by replacing `always()` by `success() || failure()`, except for
steps such as uploading / printing out logs or similar artifacts that
have already been built.

Even upon canceling a workflow (manually or automatically when a new
commit has been pushed to the same branch), a new step that uses `if:
always() ....` will still be started, which can clog the GH Actions
pipeline.

<!-- ^^^^^
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.
- [ ] 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: ...
-->
- Depends on sagemath#36614 (merged here to remove merge conflict)

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

4 participants