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

runtime-c-api: not installing cranelift if not necessary #708

Merged
merged 14 commits into from
Aug 22, 2019
Merged

runtime-c-api: not installing cranelift if not necessary #708

merged 14 commits into from
Aug 22, 2019

Conversation

YaronWittenstein
Copy link
Contributor

having default-features = false lets us install the runtime-c-api with default-backend-singlepass without installing also cranelift

@YaronWittenstein YaronWittenstein changed the title runtime-c-api: default features disabled by default runtime-c-api: not installing cranelift if not necessary Aug 22, 2019
@Hywan
Copy link
Contributor

Hywan commented Aug 22, 2019

I'm not sure we want Singlepass as the default backend for the C API.
I'm OK though to be able to disable it when selecting another backend.

@Hywan Hywan self-assigned this Aug 22, 2019
@Hywan Hywan added 📦 lib-c-api About wasmer-c-api 🎉 enhancement New feature! labels Aug 22, 2019
@YaronWittenstein
Copy link
Contributor Author

YaronWittenstein commented Aug 22, 2019 •
edited
Loading

I'm not sure we want Singlepass as the default backend for the C API.
I'm OK though to be able to disable it when selecting another backend.

@Hywan
The singlepass isn't the default
My bad, I've used bad naming here.

I've pushed a fix

Hywan
Hywan suggested changes Aug 22, 2019
View reviewed changes
Copy link
Contributor

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

You are getting close. I take the opportunity you're working on this file to rename some features, just to clarify and cleaning things up.

Thanks!

lib/runtime-c-api/Cargo.toml Outdated Show resolved Hide resolved
lib/runtime-c-api/Cargo.toml Outdated Show resolved Hide resolved
lib/runtime-c-api/Cargo.toml Outdated Show resolved Hide resolved
Hywan
Hywan reviewed Aug 22, 2019
View reviewed changes
lib/runtime-c-api/Cargo.toml Outdated
llvm = ["wasmer-runtime/llvm"]
cranelift-backend = ["wasmer-runtime/cranelift", "wasmer-runtime/default-backend-cranelift"]
llvm-backend = ["wasmer-runtime/llvm"]
singlepass-backend = ["wasmer-runtime/default-backend-singlepass", "wasmer-runtime-core/backend-singlepass"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is:

- wasmer-runtime-core/backend-singlepass
+ wasmer-runtime/backend-singlepass

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checking shortly

Copy link
Contributor Author

@YaronWittenstein YaronWittenstein Aug 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@Hywan I seems it should stay wasmer-runtime/default-backend-singlepass

https://github.com/wasmerio/wasmer/blob/master/lib/runtime/Cargo.toml#L41

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm talking about the second item of the list. You are using wasmer-runtime-core/…, I think it is wasmer-runtime/….

@YaronWittenstein YaronWittenstein force-pushed the c-api-cargo-singlepass-feature branch from da28f91 to dd705d8 Compare August 22, 2019 13:36
Hywan
Hywan reviewed Aug 22, 2019
View reviewed changes
lib/runtime-c-api/Cargo.toml Outdated
debug = ["wasmer-runtime/debug"]
llvm = ["wasmer-runtime/llvm"]
cranelift-backend = ["wasmer-runtime/cranelift", "wasmer-runtime/default-backend-cranelift"]
llvm-backend = ["wasmer-runtime/llvm"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not adding default-backend-llvm to the list of features to enable with llvm-backend?

Hywan
Hywan approved these changes Aug 22, 2019
View reviewed changes
Copy link
Contributor

@Hywan Hywan 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

@Hywan
Copy link
Contributor

Hywan commented Aug 22, 2019

bors r+

bors bot added a commit that referenced this pull request Aug 22, 2019
@bors @YaronWittenstein
Merge #708
877fe44 
708: runtime-c-api: not installing cranelift if not necessary r=Hywan a=YaronWittenstein

having `default-features = false` lets us install the `runtime-c-api` with `default-backend-singlepass` without installing also `cranelift`

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

bors bot commented Aug 22, 2019

Build succeeded

@bors bors bot merged commit 86a3a75 into wasmerio:master Aug 22, 2019
@YaronWittenstein YaronWittenstein deleted the c-api-cargo-singlepass-feature branch August 22, 2019 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hywan Hywan Hywan approved these changes

@bjfish bjfish Awaiting requested review from bjfish

@MarkMcCaskey MarkMcCaskey Awaiting requested review from MarkMcCaskey

Assignees

@Hywan Hywan

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

Successfully merging this pull request may close these issues.
2 participants
@YaronWittenstein @Hywan