Skip to content

[CI] Disable precompiled headers in monolithic-*.sh #143369

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mrkajetanp
Copy link
Contributor

Using precompiled headers can result in code with missing explicit includes building correctly because the headers will be already included through the pch. If code like this is committed, however, then trying to build the project with precompiled headers disabled will cause build errors.

Disable precompiled headers in pre-merge CI scripts to make sure these issues are caught early.

Resolves #143234.

Using precompiled headers can result in code with missing explicit
includes building correctly because the headers will be already included
through the pch. If code like this is committed, however, then trying to
build the project with precompiled headers disabled will cause build
errors.

Disable precompiled headers in pre-merge CI scripts to make sure these
issues are caught early.

Signed-off-by: Kajetan Puchalski <[email protected]>
Copy link
Member

@mgorny mgorny left a comment

Choose a reason for hiding this comment

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

I don't know whether these are the right scripts, but the concept looks good. Thanks!

@mrkajetanp
Copy link
Contributor Author

I don't know whether these are the right scripts, but the concept looks good. Thanks!

These are the ones that run in GitHub actions, e.g. in the checks running for this PR. I've not touched anything CI-related in LLVM before so there might be something else that should be adjusted too.

@DavidSpickett
Copy link
Collaborator

I agree this is a useful change to make. Wait for @boomanaiden154 to review and confirm this is the right place to make the change.

Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

Have there been any reports of failure modes related to PCH on Github Actions?

@boomanaiden154
Copy link
Contributor

This also does nothing to adjust the behavior on the buildbots. That needs to get in https://github.com/llvm/llvm-zorg.

@mrkajetanp
Copy link
Contributor Author

No failures that I've seen, it seems to be working well. I don't even necessarily think the buildbots need it, depends where we want to make the trade-off with the build time.
This change is just so that there's an extra check to make sure the PRs actually build without pch to not accidentally end up in a situation where the code only builds if pch is enabled.

@boomanaiden154
Copy link
Contributor

This change is just so that there's an extra check to make sure the PRs actually build without pch to not accidentally end up in a situation where the code only builds if pch is enabled.

I'm not sure premerge is the place for that. We want to be testing as standard of a configuration as possible, and since PCH is enabled by default for Flang, premerge should probably stick with that (barring other issues).

I hadn't seen #131137 until just now. That's useful context that I wish I had seen earlier...

@mrkajetanp
Copy link
Contributor Author

I'm not sure premerge is the place for that. We want to be testing as standard of a configuration as possible, and since PCH is enabled by default for Flang, premerge should probably stick with that (barring other issues).

I don't have very strong opinions here to be honest, so I'm happy with this going wherever you think it should go. Either way it's a slight tradeoff. I'd go the way of putting it in pre-merge just on account of it being a pretty easy mistake for people to make. The intention with introducing pch was for people to still write code as if they weren't there but use pch to speed up the compilation times. Putting it in post-merge bots might cause some commit inflation with extra "fix missing includes" commits after the fact. But maybe that's not actually an issue in the grand scheme of things.

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.

[flang] Use of precompiled headers leads to undetected missing includes
4 participants