Skip to content

Conversation

@StanFromIreland
Copy link
Member

@StanFromIreland StanFromIreland commented Jan 8, 2026

As mentioned in #143564 (review) by @ZeroIntensity:

One extra thing I would like to do is run the script on docs PRs, since a name being documented while also being in ignored_c_api.txt will cause it to fail. So, if someone merges new documentation without updating ignored_c_api.txt, we'll see blocking failures on all C code PRs until we fix it. I haven't looked too closely at the GHA for change detection, but if you see an easy way to do that, would you mind doing it in this PR?

We could keep it in the check-generated-files job, but that would make it quite messy as all the other checks would have to be excluded in the case when only run-docs is true. And, to avoid having to configure, I run the script directly (since it uses PYTHON_FOR_REGEN anyway, there shouldn't be a difference).

On this PR, the new job ran in 13 seconds.

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

This makes sense to me, but I'd like one of the GHA experts to have a look.

@encukou
Copy link
Member

encukou commented Jan 9, 2026

@webknjaz, does this look interesting?

@webknjaz
Copy link
Contributor

webknjaz commented Jan 9, 2026

Personally, I've been leaning towards having a single workflow tool target invocation per job. In this case it's one make command (I've implemented my reusable-tox.yml thing around the same approach and a it turned out to be a pretty powerful strategy).

So yes, it's a good idea — it can run a check that would otherwise be skipped, sometimes. But I also foresee better responsiveness even in full runs. This could be taken farther at some point (follow-ups) but it's well-scoped as it is.

One thing I'd ask, though would be moving the new job into a reusable-*.yml module so this doesn't contribute to cluttering the huge YAML file more than it is already. No other comments otherwise.


On this PR, the new job ran in 13 seconds.

In that case, I'd maybe reduce the job timeout to 5 minutes or less. This tends to improve responsiveness too, plus catch problems with sudden slowdowns w/o wasting too much CPU.

@StanFromIreland
Copy link
Member Author

One thing I'd ask, though would be moving the new job into a reusable-*.yml module
...
reduce the job timeout to 5 minutes

Done :-)

Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Copy link
Contributor

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

I think this is ready, I left a style note, though. But that's inconsistent across the CI definitions anyway, so not important.

Comment on lines +150 to +151
needs.build-context.outputs.run-tests == 'true'
|| needs.build-context.outputs.run-docs == 'true'
Copy link
Contributor

Choose a reason for hiding this comment

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

(personal style preference, optional)

Suggested change
needs.build-context.outputs.run-tests == 'true'
|| needs.build-context.outputs.run-docs == 'true'
fromJSON(needs.build-context.outputs.run-tests)
|| fromJSON(needs.build-context.outputs.run-docs)

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to remain consistent, I'll leave it up to whoever merges, however.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, this is consistent with a bunch of places while differs from the others. I think GHA infra could use some linting and will probably look into that later. But again, this is probably something that would be improved in follow-ups.

Copy link
Member

Choose a reason for hiding this comment

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

As a truthiness check, thing == 'true' is clearer to me than fromJSON(thing).

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with Hugo that is indeed more readable/logical.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright.. String comparison it is, then. It's just that the readability is poor when multiple conditionals are chained and having function calls makes it a little bit better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright.. String comparison it is, then. It's just that the readability is poor when multiple conditionals are chained and having function calls makes it a little bit better.

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.

5 participants