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.

Sign up for GitHub

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

Jump to bottom

Make Cranelift unwind feature optional #2249

Merged
merged 4 commits into from
May 11, 2021
Merged

Make Cranelift unwind feature optional #2249

merged 4 commits into from
May 11, 2021

Conversation

syrusakbary
Copy link
Member

Description

Make unwind feature optional

@syrusakbary syrusakbary requested a review from nlewycky as a code owner April 22, 2021 01:31
@Hywan Hywan changed the title Make Carnelift unwind feature optional Make Cranelift unwind feature optional Apr 22, 2021
@Hywan
Copy link
Contributor

Hywan commented Apr 22, 2021

Why do we want that? :-)

@Hywan Hywan self-assigned this Apr 22, 2021
@Hywan Hywan added 🎉 enhancement New feature! 📦 lib-compiler-cranelift About wasmer-compiler-cranelift labels Apr 22, 2021
@syrusakbary
Copy link
Member Author

Why do we want that? :-)

Just so the crate compiles if the feature is disabled.

@nlewycky
Copy link
Contributor

How is this tested? I realize this makes it compile when the feature is disabled, but does it work?

@syrusakbary
Copy link
Member Author

How is this tested? I realize this makes it compile when the feature is disabled, but does it work?

I tested manually, it was the easiest path. Do you have thoughts on how this should be tested.

@nlewycky
Copy link
Contributor

nlewycky commented Apr 28, 2021 •
edited
Loading

What was the manual test? Can we pass spectests with cranelift and unwind feature disabled? If you have a command line for that, we could add it to the Makefile?

@syrusakbary
Remove unused features
5dfdb14
@syrusakbary
Merge branch 'master' into unwind-optional
48139c3 
# Conflicts:
#	lib/compiler-singlepass/Cargo.toml
@syrusakbary syrusakbary requested a review from losfair as a code owner May 11, 2021 18:29
@syrusakbary
Copy link
Member Author

What was the manual test? Can we pass spectests with cranelift and unwind feature disabled? If you have a command line for that, we could add it to the Makefile?

Done!

MarkMcCaskey
MarkMcCaskey reviewed May 11, 2021
View reviewed changes
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

Looks fine to me, but I don't have context on what's going on here

@syrusakbary
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request May 11, 2021
@bors @syrusakbary
Merge #2249
273e802 
2249: Make Cranelift unwind feature optional r=syrusakbary a=syrusakbary

<!-- 
Prior to submitting a PR, review the CONTRIBUTING.md document for recommendations on how to test:
https://github.com/wasmerio/wasmer/blob/master/CONTRIBUTING.md#pull-requests

-->

# Description

Make unwind feature optional


Co-authored-by: Syrus Akbary <[email protected]>
@bors
Copy link
Contributor

bors bot commented May 11, 2021

Build failed:

@syrusakbary
Copy link
Member Author

Merging manually as all tests already passed

@syrusakbary syrusakbary merged commit 7d491b4 into master May 11, 2021
@bors bors bot deleted the unwind-optional branch May 11, 2021 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarkMcCaskey MarkMcCaskey MarkMcCaskey left review comments

@nlewycky nlewycky nlewycky approved these changes

@losfair losfair Awaiting requested review from losfair

Assignees

@Hywan Hywan

Labels
🎉 enhancement New feature! 📦 lib-compiler-cranelift About wasmer-compiler-cranelift
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@syrusakbary @Hywan @nlewycky @MarkMcCaskey