Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
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

Switch default backend from 'fastcomp' to 'upstream' #373

Merged
merged 2 commits into from Oct 21, 2019
Merged

Conversation

@sbc100
Copy link
Contributor

sbc100 commented Oct 16, 2019

When users as for 'latest' or just '1.39.0' we now default to the upstream llvm backend.

For versions before 1.39.0 we continue to default to fastcomp.

Fixes: emscripten-core/emscripten#5488

@sbc100 sbc100 force-pushed the upstream_by_default branch from 572ae99 to 9e7f798 Oct 16, 2019
@sbc100 sbc100 requested review from kripken and dschuff and removed request for kripken Oct 18, 2019
@kripken

This comment has been minimized.

Copy link
Member

kripken commented Oct 18, 2019

Before we land this we should have docs probably, but I'm not sure where?

@kripken

This comment has been minimized.

Copy link
Member

kripken commented Oct 18, 2019

Actually, maybe we could have the emsdk say something? Like show a message, "the default backend has changed" on first run after the change?

@sbc100

This comment has been minimized.

Copy link
Contributor Author

sbc100 commented Oct 18, 2019

Good idea. Added a bit notice.

@sbc100 sbc100 force-pushed the upstream_by_default branch from 5efc809 to aabbf8d Oct 18, 2019
@kripken

This comment has been minimized.

Copy link
Member

kripken commented Oct 18, 2019

Looks good, thanks!

@kripken

This comment has been minimized.

Copy link
Member

kripken commented Oct 18, 2019

In offline discussion we had the idea to maybe not always default to upstream, but just from 1.39.0 and onwards. The idea being that say at a version from last year, upstream's support was not that great yet, so it would be bad. Only at 1.39.0 are we confident the new backend is good enough for general use.

Let's discuss more and decide next week.

@sbc100 sbc100 force-pushed the upstream_by_default branch from aabbf8d to 588d8bb Oct 21, 2019
@sbc100

This comment has been minimized.

Copy link
Contributor Author

sbc100 commented Oct 21, 2019

Updated so that we now only default for upstream for 'latest' and for 1.39.0 and above.

@sbc100 sbc100 changed the title Switch 'latest' from 'latest-fastcomp' to 'latest-upstream' Switch default backend from 'fastcomp' to 'latest' Oct 21, 2019
@sbc100 sbc100 force-pushed the upstream_by_default branch from 588d8bb to 12a6db9 Oct 21, 2019
@sbc100 sbc100 changed the title Switch default backend from 'fastcomp' to 'latest' Switch default backend from 'fastcomp' to 'upstream' Oct 21, 2019
When users as for 'latest' or just '1.39.0' we now default to the
upstream llvm backend.

For versions before 1.39.0 we continue to default to fastcomp.

Fixes: emscripten-core/emscripten#5488
@sbc100 sbc100 force-pushed the upstream_by_default branch from 12a6db9 to 25835aa Oct 21, 2019
emsdk.py Outdated Show resolved Hide resolved
emsdk.py Outdated Show resolved Hide resolved
if arg in ('latest', 'sdk-latest', 'latest-64bit', 'sdk-latest-64bit', 'latest-fastcomp', 'latest-releases-fastcomp'):
if arg in ('latest', 'sdk-latest', 'latest-64bit', 'sdk-latest-64bit'):
# This is effectly the default SDK
report_upstream_by_default()

This comment has been minimized.

Copy link
@kripken

kripken Oct 21, 2019

Member

I think we can report this only if the version is high enough? Can maybe share code with the existing check later down.

This comment has been minimized.

Copy link
@sbc100

sbc100 Oct 21, 2019

Author Contributor

The version check make sense below, but here we know that we want to unconditionally select upstream right? We never want latest to point to fastcomp ever again, do we? And if we do we would revert this code.

@sbc100 sbc100 merged commit ae5044e into master Oct 21, 2019
4 checks passed
4 checks passed
ci/circleci: flake8 Your tests passed on CircleCI!
Details
ci/circleci: test-linux Your tests passed on CircleCI!
Details
ci/circleci: test-mac Your tests passed on CircleCI!
Details
ci/circleci: test-windows Your tests passed on CircleCI!
Details
@sbc100 sbc100 deleted the upstream_by_default branch Oct 21, 2019
@aheejin

This comment has been minimized.

Copy link
Member

aheejin commented Oct 21, 2019

🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.