Обсуждение: Build the docs if there are changes in docs and don't run other tasks if the changes are only in docs

Hi,

There is an old work item about building the docs if there are changes
in the docs, otherwise don't build the docs. I wanted to make an
addition to that idea; if the changes are only in the docs, don't run
all tasks except building the docs task; this could help to save more
CI times. I attached two patches.

I assumed that the docs related changes are limited with the changes
in the docs folder but I am not really sure about that.

v1-0001-Only-built-the-docs-if-there-are-changes-are-in-t.patch:
This patch creates another task named 'Building the Docs' and moves
building the doc script from 'CompilerWarnings' task to this task.
This building the docs task only runs if there are changes in the docs
(under the doc/**) or in the CI files ('.cirrus.yml',
'.cirrus.tasks.yml') and if a specific OS is not requested.

v1-0002-Just-run-the-Build-the-Docs-task-if-the-changes-a.patch:
This patch adds that: if the changes are *only* in the docs (under the
doc/**), *only* run building the docs task.

As a summary:
1- If the changes are not in the docs: Don't run build the docs task.
2- If the changes are in the docs or in the CI files : Run build the docs task.
3- If the changes are only in the docs: Only run build the docs task.
4- If 'ci-os-only:' set (There could be changes in the docs): Don't
run build the docs task.

Any kind of feedback would be appreciated.

Regards,
Nazir Bilal Yavuz
Microsoft

> On 7 Sep 2023, at 18:06, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:

> if the changes are only in the docs, don't run
> all tasks except building the docs task; this could help to save more
> CI times.

A related idea for docs in order to save CI time: if the changes are only in
internal docs, ie README files, then don't run any tasks at all.  Looking at
src/backend/parser/README the last two commits only touched that file, and
while such patches might not be all that common, spending precious CI resources
on them seems silly if we can avoid it.

It doesn't have to be included in this, just wanted to bring it up as it's
related.

> I attached two patches.
>
> I assumed that the docs related changes are limited with the changes
> in the docs folder but I am not really sure about that.

Almost, but not entirely.  There are a set of scripts which generate content
for the docs based on files in src/, like src/backend/catalog/sql_features.txt
and src/include/parser/kwlist.h.  If those source files change, or their
scripts, it would be helpful to build docs.

--
Daniel Gustafsson

Hi,

Thanks for the reply!

On Fri, 8 Sept 2023 at 11:05, Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 7 Sep 2023, at 18:06, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
>
> > if the changes are only in the docs, don't run
> > all tasks except building the docs task; this could help to save more
> > CI times.
>
> A related idea for docs in order to save CI time: if the changes are only in
> internal docs, ie README files, then don't run any tasks at all.  Looking at
> src/backend/parser/README the last two commits only touched that file, and
> while such patches might not be all that common, spending precious CI resources
> on them seems silly if we can avoid it.
>
> It doesn't have to be included in this, just wanted to bring it up as it's
> related.

I liked the idea, I am planning to edit the 0002 patch. CI won't run
any tasks if the changes are only in the README files.

> Almost, but not entirely.  There are a set of scripts which generate content
> for the docs based on files in src/, like src/backend/catalog/sql_features.txt
> and src/include/parser/kwlist.h.  If those source files change, or their
> scripts, it would be helpful to build docs.

Thanks! Are these the only files that are not in the doc subfolders
but effect docs?

Regards,
Nazir Bilal Yavuz
Microsoft

> On 11 Sep 2023, at 13:03, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:

>> Almost, but not entirely.  There are a set of scripts which generate content
>> for the docs based on files in src/, like src/backend/catalog/sql_features.txt
>> and src/include/parser/kwlist.h.  If those source files change, or their
>> scripts, it would be helpful to build docs.
>
> Thanks! Are these the only files that are not in the doc subfolders
> but effect docs?

I believe so, the list of scripts and input files can be teased out of the
doc/src/sgml/meson.build file.

--
Daniel Gustafsson

Hi,

I attached the second version of the patch.

On Mon, 11 Sept 2023 at 15:11, Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 11 Sep 2023, at 13:03, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
>
> >> Almost, but not entirely.  There are a set of scripts which generate content
> >> for the docs based on files in src/, like src/backend/catalog/sql_features.txt
> >> and src/include/parser/kwlist.h.  If those source files change, or their
> >> scripts, it would be helpful to build docs.
> >
> > Thanks! Are these the only files that are not in the doc subfolders
> > but effect docs?
>
> I believe so, the list of scripts and input files can be teased out of the
> doc/src/sgml/meson.build file.

Now the files mentioned in the doc/src/sgml/meson.build file will
trigger building the docs task. Also, if the changes are only in the
README files, CI will be skipped completely.

Any kind of feedback would be appreciated.

Regards,
Nazir Bilal Yavuz
Microsoft

On 25.09.23 12:56, Nazir Bilal Yavuz wrote:
> +  # Only run if a specific OS is not requested and if there are changes in docs
> +  # or in the CI files.
> +  skip: >
> +    $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:.*' ||
> +    !changesInclude('doc/**',
> +                    '.cirrus.yml',
> +                    '.cirrus.tasks.yml',
> +                    'src/backend/catalog/sql_feature_packages.txt',
> +                    'src/backend/catalog/sql_features.txt',
> +                    'src/backend/utils/errcodes.txt',
> +                    'src/backend/utils/activity/wait_event_names.txt',
> +                    'src/backend/utils/activity/generate-wait_event_types.pl',
> +                    'src/include/parser/kwlist.h')

This is kind of annoying.  Now we need to maintain yet another list of 
these dependencies and keep it in sync with the build systems.

I think meson can produce a dependency tree from a build.  Maybe we 
could use that somehow and have Cirrus cache it between runs?

Also note that there are also dependencies in the other direction.  For 
example, the psql help is compiled from XML DocBook sources.  So your 
other patch would also need to include similar changesInclude() clauses.

Hi,

On Tue, 26 Sept 2023 at 13:48, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 25.09.23 12:56, Nazir Bilal Yavuz wrote:
> > +  # Only run if a specific OS is not requested and if there are changes in docs
> > +  # or in the CI files.
> > +  skip: >
> > +    $CIRRUS_CHANGE_MESSAGE =~ '.*\nci-os-only:.*' ||
> > +    !changesInclude('doc/**',
> > +                    '.cirrus.yml',
> > +                    '.cirrus.tasks.yml',
> > +                    'src/backend/catalog/sql_feature_packages.txt',
> > +                    'src/backend/catalog/sql_features.txt',
> > +                    'src/backend/utils/errcodes.txt',
> > +                    'src/backend/utils/activity/wait_event_names.txt',
> > +                    'src/backend/utils/activity/generate-wait_event_types.pl',
> > +                    'src/include/parser/kwlist.h')
>
> This is kind of annoying.  Now we need to maintain yet another list of
> these dependencies and keep it in sync with the build systems.

I agree.

>
> I think meson can produce a dependency tree from a build.  Maybe we
> could use that somehow and have Cirrus cache it between runs?

I will check that.

>
> Also note that there are also dependencies in the other direction.  For
> example, the psql help is compiled from XML DocBook sources.  So your
> other patch would also need to include similar changesInclude() clauses.
>

If there are more cases like this, it may not be worth it. Instead, we can just:

- Build the docs when the doc related files are changed (This still
creates a dependency like you said).

- Skip CI completely if the README files are changed.

What are your opinions on these?

Regards,
Nazir Bilal Yavuz
Microsoft

On 26.09.23 16:51, Nazir Bilal Yavuz wrote:
>> Also note that there are also dependencies in the other direction.  For
>> example, the psql help is compiled from XML DocBook sources.  So your
>> other patch would also need to include similar changesInclude() clauses.
> 
> If there are more cases like this, it may not be worth it. Instead, we can just:
> 
> - Build the docs when the doc related files are changed (This still
> creates a dependency like you said).
> 
> - Skip CI completely if the README files are changed.
> 
> What are your opinions on these?

I don't have a good sense of what you are trying to optimize for.  If 
it's the mainline build-on-every-commit type, then I wonder how many 
commits would really be affected by this.  Like, how many commits touch 
only a README file.  If it's for things like the cfbot, then I think the 
time-triggered builds would be more frequent than new patch versions, so 
I don't know if these kinds of optimizations would affect anything.

Peter Eisentraut <peter@eisentraut.org> writes:
> I don't have a good sense of what you are trying to optimize for.  If 
> it's the mainline build-on-every-commit type, then I wonder how many 
> commits would really be affected by this.  Like, how many commits touch 
> only a README file.  If it's for things like the cfbot, then I think the 
> time-triggered builds would be more frequent than new patch versions, so 
> I don't know if these kinds of optimizations would affect anything.

As a quick cross-check, I searched our commit log to see how many
README-only commits there were so far this year.  I found 11 since
January.  (Several were triggered by the latest round of pgindent
code and process changes, so maybe this is more than typical.)

Not sure what that tells us about the value of changing the CI
logic, but it does seem like it could be worth the one-liner
change needed to teach buildfarm animals to ignore READMEs.

-    trigger_exclude => qr[^doc/|\.po$],
+    trigger_exclude => qr[^doc/|/README$|\.po$],

            regards, tom lane

Hi,

Sorry for the late reply.

On Fri, 6 Oct 2023 at 17:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> As a quick cross-check, I searched our commit log to see how many
> README-only commits there were so far this year.  I found 11 since
> January.  (Several were triggered by the latest round of pgindent
> code and process changes, so maybe this is more than typical.)
>
> Not sure what that tells us about the value of changing the CI
> logic, but it does seem like it could be worth the one-liner
> change needed to teach buildfarm animals to ignore READMEs.

I agree that it could be worth implementing this logic on buildfarm animals.

In case we want to implement the same logic on the CI, I added a new
version of the patch; it skips CI completely if the changes are only
in the README files.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

> On 14 Dec 2023, at 14:40, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> On Fri, 6 Oct 2023 at 17:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

>> Not sure what that tells us about the value of changing the CI
>> logic, but it does seem like it could be worth the one-liner
>> change needed to teach buildfarm animals to ignore READMEs.
>
> I agree that it could be worth implementing this logic on buildfarm animals.
>
> In case we want to implement the same logic on the CI, I added a new
> version of the patch; it skips CI completely if the changes are only
> in the README files.

I think it makes sense to avoid wasting CI cycles on commits only changing
README files since we know it will be a no-op.  A README documentation patch
going through N revisions will incur at least N full CI runs which are
resources we can spend on other things.  So +1 for doing this both in CI and in
the buildfarm.

--
Daniel Gustafsson

On 2023-10-06 Fr 10:07, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> I don't have a good sense of what you are trying to optimize for.  If
>> it's the mainline build-on-every-commit type, then I wonder how many
>> commits would really be affected by this.  Like, how many commits touch
>> only a README file.  If it's for things like the cfbot, then I think the
>> time-triggered builds would be more frequent than new patch versions, so
>> I don't know if these kinds of optimizations would affect anything.
> As a quick cross-check, I searched our commit log to see how many
> README-only commits there were so far this year.  I found 11 since
> January.  (Several were triggered by the latest round of pgindent
> code and process changes, so maybe this is more than typical.)
>
> Not sure what that tells us about the value of changing the CI
> logic, but it does seem like it could be worth the one-liner
> change needed to teach buildfarm animals to ignore READMEs.
>
> -    trigger_exclude => qr[^doc/|\.po$],
> +    trigger_exclude => qr[^doc/|/README$|\.po$],
>
>             



I've put that in the sample config file for the next release.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 14.12.23 14:40, Nazir Bilal Yavuz wrote:
> On Fri, 6 Oct 2023 at 17:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> As a quick cross-check, I searched our commit log to see how many
>> README-only commits there were so far this year.  I found 11 since
>> January.  (Several were triggered by the latest round of pgindent
>> code and process changes, so maybe this is more than typical.)
>>
>> Not sure what that tells us about the value of changing the CI
>> logic, but it does seem like it could be worth the one-liner
>> change needed to teach buildfarm animals to ignore READMEs.
> 
> I agree that it could be worth implementing this logic on buildfarm animals.
> 
> In case we want to implement the same logic on the CI, I added a new
> version of the patch; it skips CI completely if the changes are only
> in the README files.

I don't see how this could be applicable widely enough to be useful:

- While there are some patches that touch on README files, very few of 
those end up in a commit fest.

- If someone manually pushes a change to their own CI environment, I 
don't see why we need to second-guess that.

- Buildfarm runs generally batch several commits together, so it is very 
unlikely that this would be hit.

I think unless some concrete reason for this change can be shown, we 
should drop it.

Hi,

On Sun, 12 May 2024 at 14:53, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 14.12.23 14:40, Nazir Bilal Yavuz wrote:
> > On Fri, 6 Oct 2023 at 17:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>
> >> As a quick cross-check, I searched our commit log to see how many
> >> README-only commits there were so far this year.  I found 11 since
> >> January.  (Several were triggered by the latest round of pgindent
> >> code and process changes, so maybe this is more than typical.)
> >>
> >> Not sure what that tells us about the value of changing the CI
> >> logic, but it does seem like it could be worth the one-liner
> >> change needed to teach buildfarm animals to ignore READMEs.
> >
> > I agree that it could be worth implementing this logic on buildfarm animals.
> >
> > In case we want to implement the same logic on the CI, I added a new
> > version of the patch; it skips CI completely if the changes are only
> > in the README files.
>
> I don't see how this could be applicable widely enough to be useful:
>
> - While there are some patches that touch on README files, very few of
> those end up in a commit fest.
>
> - If someone manually pushes a change to their own CI environment, I
> don't see why we need to second-guess that.
>
> - Buildfarm runs generally batch several commits together, so it is very
> unlikely that this would be hit.
>
> I think unless some concrete reason for this change can be shown, we
> should drop it.

These points make sense. I thought that it is useful regarding Tom's
'11 README-only commit since January' analysis (at 6 Oct 2023) but
this may not be enough on its own. If there are no objections, I will
withdraw this soon.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft