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

Fix #2418: Building docs should be a part of the CI. #2431

Merged
merged 6 commits into from
May 22, 2023

Conversation

ankusharya
Copy link
Contributor

In this commit:

  • Add action build-docs
  • Modify docs/user/sbt.rst to remove the warning

Explanation about build-docs action

  • Build on top of sphinx-action
  • Update Sphinx to 4.2.0
  • Warning in building docs bubble as GitHub warning
  • Action build-docs is tested locally by nektos/act

Copy link
Contributor

@WojciechMazur WojciechMazur left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. I have two comments that should be addressed before merging this change.

.github/workflows/build-docs.yml Outdated Show resolved Hide resolved
- uses: ammaraskar/sphinx-action@master
with:
pre-build-command: "pip install Sphinx==4.2.0 recommonmark==0.7.1"
docs-folder: "docs/"
Copy link
Contributor

Choose a reason for hiding this comment

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

The current setup would not treat warnings as errors, most of the problems with our docs building in the past were in most cases informed as warnings, but they were ignored, eg. missing entry in the index. We should add "-W" option to enable fatal warnings, --keep-going to not stop after the first warning and optionally -n which would try to check for missing references

Suggested change
docs-folder: "docs/"
build-command: 'make html SPHINXOPTS="-W --keep-going -n"'
docs-folder: "docs/"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,
Before raising pull request, I thoroughly checked for all the options to make warning as error.
I have checked on adding -W but it does not work.
To solve it I had already raised issue.

Even with one issue resolved, we can enable warning as error.

I also looked into catching warning as they bubble as Github warning, but i dont see any reference of any such workflow commands
If you find any resources/approach, please share. I will implement it.
Any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ankusharya I think that maybe problems with log files might be also a problem with running with act. We should check if it behaves the same in GH Actions environments in the same way. I don't have a large experience with this tool, but even then I came to corner cases where build was failing locally with act and worked well in the CI

Copy link
Member

Choose a reason for hiding this comment

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

It seems like -W didn't work with sphinx-action (action will fail with /tmp/sphinx-log couldn't find) when we use make to invoke sphinx. In this case the SPHINXOPTS collapses between our SPHINXOPTS and internal SPHINXOPTS.

in env=dict(os.environ, SPHINXOPTS=sphinx_options), the sphinx_options will be ignored.
https://github.com/ammaraskar/sphinx-action/blob/e781e9af3e80bfe0ea539e4ea46858d51e027214/sphinx_action/action.py#L114-L123

To workaround this, we can build sphinx by sphinx-build instead of make.

ebb3ac8

ankusharya and others added 4 commits May 22, 2023 14:26
@tanishiking
Copy link
Member

tanishiking commented May 22, 2023

FYI @ankusharya @WojciechMazur
I rebased the PR on main branch and made it work with sphinx-build on CI, if there's a warning the CI will fail like this:
https://github.com/scala-native/scala-native/actions/runs/5043307827/jobs/9044935416
(sorry to directly push changes onto your branch)

@WojciechMazur WojciechMazur merged commit ea27479 into scala-native:main May 22, 2023
WojciechMazur pushed a commit that referenced this pull request May 23, 2023
* Fix #2418: Building docs should be a part of the CI. Modify sbt.rst warning issue
* Make sphinx-build fails if there's warning
* Install in pre-build-command

Co-authored-by: Wojciech Mazur <wojciech.mazur95@gmail.com>
Co-authored-by: Rikito Taniguchi <rikiriki1238@gmail.com>
(cherry picked from commit ea27479)
WojciechMazur pushed a commit that referenced this pull request Jun 5, 2023
* Fix #2418: Building docs should be a part of the CI. Modify sbt.rst warning issue
* Make sphinx-build fails if there's warning
* Install in pre-build-command

Co-authored-by: Wojciech Mazur <wojciech.mazur95@gmail.com>
Co-authored-by: Rikito Taniguchi <rikiriki1238@gmail.com>
(cherry picked from commit ea27479)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants