Skip to content

ci: doc-build: add new check time-incremental-build #58165

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

Closed
wants to merge 1 commit into from
Closed

ci: doc-build: add new check time-incremental-build #58165

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .github/workflows/doc-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,18 @@ jobs:
name: pr_num
path: pr_num

# Keep this last not to break upload and other "functional" tests above.
- name: time-incremental-build
if: github.event_name == 'pull_request'
run: |
# Catch major incremental build time regressions like the ones
# fixed by commits 52842317505f, a140a249b33b and 9d119431945f.
sed -i -e 's/Zephyr /Xephyr /' doc/index.rst # make a small .rst change
export DOC_TAG=development SPHINXOPTS='-j auto -t publish' # as above
time timeout 35 make -C doc/ html-fast # ${DOC_TARGET} as above
Copy link
Contributor

@keith-zephyr keith-zephyr Jul 14, 2023

Choose a reason for hiding this comment

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

Probably better to be explicit about the timeout units.

Suggested change
time timeout 35 make -C doc/ html-fast # ${DOC_TARGET} as above
time timeout 35s make -C doc/ html-fast # ${DOC_TARGET} as above

git checkout HEAD -- doc/index.rst # restore to be safe
Comment on lines +117 to +126
Copy link
Member

@gmarull gmarull May 23, 2023

Choose a reason for hiding this comment

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

I don't see the need to add such hacks in CI. If something breaks, one can open an issue. GH actions also support timeouts.

Copy link
Member

@stephanosio stephanosio May 23, 2023

Choose a reason for hiding this comment

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

No strong opinion on what to do here; though, given that there were multiple changes in the past that resulted in increased incremental build time and that seems to be a recurring problem, I can see this being useful in catching problems like that before they get merged; moreover, at most, it only adds 35 seconds to the workflow run time.

Copy link
Contributor Author

@marc-hb marc-hb May 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see the need to add such hacks in CI. If something breaks, one can open an issue.

The purpose of CI is to catch regressions when they happen. It took me a day to isolate the numfig regression, long after it was merged. I don't make doc changes every day, very far from it.

Few people contribute to documentation at all and from past experience it seems many of them don't notice how slow the incremental build becomes; they just assume that "building documentation is very slow" - which I think makes even fewer people contribute to documentation.

GH actions also support timeouts.

I will take a look, thanks. See comments below.

Copy link
Contributor

@kartben kartben May 23, 2023

Choose a reason for hiding this comment

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

+1

Re: GH Actions, it's not clear if this would allow to set a timeout of less than a minute https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepstimeout-minutes

Copy link
Member

@stephanosio stephanosio May 23, 2023

Choose a reason for hiding this comment

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

We would not want to use the timeout-minutes here since that is supposed to be the last resort when something gets stuck, which is not the case here. We want the job result to be "failed" (i.e. not "timeout") when incremental build takes too long.



doc-build-pdf:
name: "Documentation Build (PDF)"
if: github.event_name != 'pull_request'
Expand Down