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

Jit build matrix #9

Merged
merged 9 commits into from
Jan 20, 2020
Merged

Jit build matrix #9

merged 9 commits into from
Jan 20, 2020

Conversation

sirpy
Copy link
Contributor

@sirpy sirpy commented Jan 13, 2020

This PRs creates a build matrix with the nointl + jit features.
So result is
v8, v8-nointl, v8-jit, v8-jit-nointl

The new configuration also use jobs to run builds in parallel

@sirpy sirpy mentioned this pull request Jan 13, 2020
Copy link
Owner

@Kudo Kudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks for your help.
Just leaving a nit inline comment.
Please help with this.

scripts/build.sh Outdated
@@ -11,7 +11,7 @@ GN_ARGS_BASE="
v8_use_snapshot=true
v8_use_external_startup_data=false
icu_use_data_file=false
v8_enable_lite_mode=true
v8_enable_lite_mode=${DISABLE_JIT}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit.
Could you setup v8_enable_lite_mode like what NO_INTL did?
I.e.
Removing this line and add

if [[ ${DISABLE_JIT} -ne "false" ]]; then
  GN_ARGS_BASE="${GN_ARGS_BASE} v8_enable_i18n_support=false"
fi

Otherwise, for DISABLE_JIT mode, the generated GN config will be:
v8_enable_lite_mode= Explicit is better than implicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've made the change but i'm not sure its correct what you requested (in terms of true/false)
what is the default if you dont specify v8_enable_lite_mode?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the change is exactly what I talked about.
The default value should be JIT enabled.

I am going to merge this PR and publish JIT versions once builds ready.
Thank you so much for your fantastic work.

@Kudo Kudo merged commit ca7a588 into Kudo:master Jan 20, 2020
@coder-wbw coder-wbw mentioned this pull request Aug 17, 2024
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.

2 participants