-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add build phase to detect nested frameworks #21130
Add build phase to detect nested frameworks #21130
Conversation
#!/bin/sh -eu | ||
|
||
set pipefail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only now notice that the shebang doesn't use Bash. Possibly why I ShellCheck warned me about having pipefail
in there.
runOnlyForDeploymentPostprocessing = 0; | ||
shellPath = /bin/sh; | ||
shellScript = "\"$SRCROOT/../Scripts/BuildPhases/LintNestedFrameworks.sh\"\n"; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the build phase in both WordPress and Jetpack. We cannot run this before building the apps in a centralized aggregated target because the script looks into the target's frameworks.
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder who wrote that shell script but it looks great 😃
See #21129 picking up the issue in the "faulty" Gutenberg version.
👌
I left a comment to suggest keeping the P2 post reference, as it can still be helpful for the screenshots and explanations it provides. Other than that, LGTM 👍
Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>
For the record, I did not miss the RuboCop violation in CI. I decided to ignore it because it's unrelated to the changes in this PR and the Ruby code is being modified elsewhere in #21021 |
The Hermes nested framework issue that gave us some grief could have been avoided if we had preemptively checked against that kind of setup.
The issue should be fixed now (either via #21021 or #21128) but we can benefit with this check to avoid paying the price again in the future.
How to test
See #21129 picking up the issue in the "faulty" Gutenberg version.
Regression Notes
PR submission checklist:
RELEASE-NOTES.txt
if necessary. N.A.UI changes testing checklist: Not a UI PR.