-
Notifications
You must be signed in to change notification settings - Fork 0
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
Support JavaScriptCore 6.0 #53
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Robert Günzler <r@gnzler.io>
Signed-off-by: Robert Günzler <r@gnzler.io>
Signed-off-by: Robert Günzler <r@gnzler.io>
Signed-off-by: Robert Günzler <r@gnzler.io>
Signed-off-by: Robert Günzler <r@gnzler.io>
@tony-go i guess there was some kind of credit limit for CI on this org/repo? |
one thing i noticed... we also install |
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.
Looks good to me
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.
Asked for a minor change, but looks good
.github/workflows/ci.yml
Outdated
javascriptcore_version: "4.0" | ||
options: -DINCLUDEJS_BACKEND_JAVASCRIPTCORE_API_VERSION:STRING=4.0 |
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 if we could not use javascriptcore_version to set DINCLUDEJS_BACKEND_JAVASCRIPTCORE_API_VERSION
later.
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.
see my latest commit, we could do it like that... but then it ends up being specified (as empty) for non-linux ci runs
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.
hmm, you're right.
Do you prefer to revert 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.
i mean it being empty on non-linux isn't really an issue... since the FindJavaScriptCore.cmake
just ignores this on macos anyway. although we might want to check that it's not set and warn otherwise
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.
any idea why the asan would fail here? i think nothing should have changed
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.
Hey I already experienced weird stuff with ASAN: https://bugs.webkit.org/show_bug.cgi?id=269791
Signed-off-by: Robert Günzler <r@gnzler.io>
left some TODOs in the PR description |
TODO