Skip to content

Do not add precompiled bridging header to a Compile job's display inputs #530

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

Merged
merged 1 commit into from
Mar 8, 2021

Conversation

artemcm
Copy link
Contributor

@artemcm artemcm commented Mar 5, 2021

"Display inputs" are used to convey to the user/client the set of input files that a given compile job will build, e.g. in parseable-output.

The driver is relying on the presence of the bridging PCH in displayInputs because the printActions code relies on displayInputs, when they are present. Unlike parseable-output, action printing is used for compiler debugging purposes only, and given that printActions is a best-effort emulation of the legacy driver behavior that doesn't really exist in the new driver, we can just as well get away with using primaryInputs and ensure compile job actions refer to the corresponding precompiled header, to keep the C++ tests passing (test/Driver/actions.swift & test/Driver/actions-dsym.swift).

"Display inputs" are used to convey to the user/client the set of input files that a given compile job will build, e.g. in parseable-output.

The driver is relying on the presence of the bridging PCH in `displayInputs` because the `printActions` code relies on `displayInputs`, when they are present. Unlike parseable-output, action printing is used for compiler debugging purposes only, and given that `printActions` is a best-effort emulation of the legacy driver behavior that doesn't really exist in the new driver, we can just as well get away with using `primaryInputs` and ensure `compile` job actions refer to the corresponding precompiled header, to keep the C++ tests passing (`test/Driver/actions.swift` & `test/Driver/actions-dsym.swift`).
Copy link
Contributor

@cltnschlosser cltnschlosser left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks for the documentation updates too. I have a much better understanding of what the differences are now.

Copy link
Contributor

@nkcsgexi nkcsgexi left a comment

Choose a reason for hiding this comment

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

Thanks for fixing it!

@artemcm
Copy link
Contributor Author

artemcm commented Mar 5, 2021

@swift-ci please test

Copy link
Contributor

@BenchR267 BenchR267 left a comment

Choose a reason for hiding this comment

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

Thanks!

@artemcm artemcm merged commit 4b6b994 into swiftlang:main Mar 8, 2021
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.

4 participants