-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Initial support for multiple LLVM versions #11639
Conversation
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.
The idea seems good, but without CI testing of 11, should we perhaps land this PR with all the future support ready, but not actually allow 11 atm?
@@ -17,6 +17,8 @@ See docs/process.md for how version tagging works. | |||
|
|||
Current Trunk | |||
------------- | |||
- A simultaneous support for both llvm 12.0 (tot) and llvm 11.0 (soon to be |
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.
- A simultaneous support for both llvm 12.0 (tot) and llvm 11.0 (soon to be | |
- Simultaneous support for both llvm 12.0 (tot) and llvm 11.0 (soon to be |
@@ -17,6 +17,8 @@ See docs/process.md for how version tagging works. | |||
|
|||
Current Trunk | |||
------------- | |||
- A simultaneous support for both llvm 12.0 (tot) and llvm 11.0 (soon to be | |||
stable). See https://github.com/emscripten-core/emscripten/issues/11362 |
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.
stable). See https://github.com/emscripten-core/emscripten/issues/11362 | |
stable). #11362 |
@@ -412,31 +412,29 @@ def fix_js_engine(old, new): | |||
return new | |||
|
|||
|
|||
def expected_llvm_version(): | |||
def supported_llvm_versions(): |
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.
perhaps a comment that they must be in order (as the callers seem to depend on that)?
else: | ||
return "6.0" | ||
return ("6.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.
maybe '
here like above, or both with "
?
How about we issue a different warning... I try an come up with one. |
Putting this on hold for a while. |
See #11362