Skip to content

Add missing code for parser entry template and fix build-script.py #1352

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 2 commits into from
Feb 19, 2023

Conversation

kimdv
Copy link
Contributor

@kimdv kimdv commented Feb 18, 2023

In #1346 code was modified on the file and not in the template

@kimdv kimdv requested a review from ahoppen as a code owner February 18, 2023 22:40
@kimdv
Copy link
Contributor Author

kimdv commented Feb 18, 2023

@swift-ci please test

@kimdv
Copy link
Contributor Author

kimdv commented Feb 18, 2023

The error was properly introduced in #1149 where we refactored the code-gen into a single executable.

Not sure why the CI didn't catch that before

@kimdv
Copy link
Contributor Author

kimdv commented Feb 18, 2023

@swift-ci please test

@kimdv kimdv changed the title Add missing code for parser entry template Add missing code for parser entry template and fix build-script.py Feb 18, 2023
@keith
Copy link
Member

keith commented Feb 19, 2023

@kimdv looks like things don't build without this, is it good to merge?

@kimdv
Copy link
Contributor Author

kimdv commented Feb 19, 2023

@kimdv looks like things don't build without this, is it good to merge?

For me yea. But we need approval from @ahoppen I think 🤔

@keith
Copy link
Member

keith commented Feb 19, 2023

@swift-ci please test and merge

@keith
Copy link
Member

keith commented Feb 19, 2023

Let's merge and do post-commit review if needed to unblock folks

@keith keith merged commit 8eb538d into swiftlang:main Feb 19, 2023
@kimdv kimdv deleted the kimdv/add-missing-gen-code branch February 19, 2023 20:41
@gottesmm
Copy link
Contributor

@keith this broke the build: https://ci.swift.org/job/oss-swift-incremental-RA-macos/2331/. I am going to try reverting it.

@keith
Copy link
Member

keith commented Feb 20, 2023

dang, sorry, thanks! looks like we need to run the integration tests before merging again

@ahoppen
Copy link
Member

ahoppen commented Feb 20, 2023

Not sure why the CI didn't catch that before

We don’t verify that the CodeGenerated files match the templates because running code generation requires us to check out an older version of SwiftSyntax (the one specified in the Package.swift of CodeGeneration). But CI doesn’t have internet access at that point to perform that checkout… So it’s a little tricky to do.

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