Skip to content
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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Support JavaScriptCore 6.0 #53

wants to merge 7 commits into from

Conversation

robertgzr
Copy link

@robertgzr robertgzr commented Mar 12, 2024

  • chore: vendor webkit javascriptcore headers
  • chore: extend ci matrix to include jobs for jsc-4.0 and jsc-6.0
  • Support selecting JavaScriptCore API version

TODO

  • make FindJSC.cmake smart enough to detect headers present in vendor/ and system paths
  • install JSC headers as part of includejs bundle

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>
@robertgzr
Copy link
Author

@tony-go i guess there was some kind of credit limit for CI on this org/repo?

@robertgzr
Copy link
Author

one thing i noticed... we also install FindJavaScriptCore.cmake into the dist dir. that means it will leak our approach to vendor the headers and will not work if used on a system that doesn't also vendor them in the exact same way we do

Copy link
Contributor

@jviotti jviotti left a 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

Copy link
Member

@tony-go tony-go left a 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

Comment on lines 41 to 42
javascriptcore_version: "4.0"
options: -DINCLUDEJS_BACKEND_JAVASCRIPTCORE_API_VERSION:STRING=4.0
Copy link
Member

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.

Copy link
Author

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

Copy link
Member

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?

Copy link
Author

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

Copy link
Author

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

Copy link
Member

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>
@robertgzr
Copy link
Author

robertgzr commented Mar 14, 2024

left some TODOs in the PR description

@robertgzr robertgzr marked this pull request as draft March 14, 2024 15:49
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.

3 participants