-
Notifications
You must be signed in to change notification settings - Fork 10
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
Handle type parameter change in crnl 0.45.1 #182
Conversation
@@ -174,6 +174,11 @@ create_library() { | |||
rm -rf "$base" || error "Failed to remove existing directory $base" | |||
fi | |||
|
|||
local type="turbo-module" | |||
if [[ "$BOB_VERSION" != "0.45.1" && "$BOB_VERSION" = "`echo -e "0.45.1\n$BOB_VERSION" | sort -V | head -n1`" ]]; then |
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.
This is probably fine for now, but this breaks once builder-bob v0.100.0 ships 😊
If this was TypeScript (which I actually tend to favor for scripts like these) you could have used the server
package to do this reliably. If you want to make this more robust you could also add it as a dev-dependency and call into node
here.
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'm going to delegate my review approval power to @kraenhansen who seems to be better placed to approve or disapprove.
I would suggest, however, not translating this script into TS/JS, but do it in a different PR.
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 think every shell script reaches a point in its evolution where it's time to be converted to a more powerful language ... and I've been there numerous times before. 😅
I wouldn't be opposed to rewriting the script at all but do agree that we should probably make a small fix to unbreak the CI first and then queue up the rewrite for afterwards.
If I'm not mistaken, sort -V
should handle larger version numbers, too. So I'd hope this is alright for now.
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.
You're right - I didn't realize the meaning of that 👍
I'll land this so that the other PRs are unblocked. Happy to refine or rewrite the script in the future. |
Looks like this was renamed in callstack/react-native-builder-bob@e9f80ea which now breaks some of the compat jobs.
https://github.com/jhugman/uniffi-bindgen-react-native/actions/runs/12208165279/job/34060907217