-
Notifications
You must be signed in to change notification settings - Fork 30
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
Major cleanup. Remove compile
, compile_wasm
, and MixTape
#146
Conversation
Failing integration tests appear to be because of |
Thumbs up on this PR. I looked over the code, but I didn't really do a code review. If it passes tests, I'm good. I'm okay with removing both the Mixtape and the wasm support. For wasm, I think WebAssemblyCompiler will (should) compile more code and offer more capability. I agree that the |
As part of the cutting of features, I would like to keep the cross-compiling support. |
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.
This looks good to me! This package has really come a long way and taught us all a lot I think. This slimming-down should be great for the maintenance burden going forward, and anyone can always check out the older versions if they want to see all the other things we experimented with.
I'm happy with merging whenever it seems like tests are in as good shape as we can get them, as long as Tom's happy too
Okay, I've removed the wasm stuff, and the mixtape stuff. I also added an ability to specify your own |
compile
, do some cleaning upcompile
, compile_wasm
, and MixTape
Hooray! |
This removes the
compile
functionality (closes #119), and does some general cleanup and fixes.Before we merge this though, I'd also like to also discuss removing
Mixtape
stuff since I'm not really convinced it's worth the maintenance burden when we already have@device_override
which is easier to use, and doesn't rely on the broken, unmaintained CodeInfoTools.jl.@device_override
should cover every reasonable usecase as far as I'm aware.wasm
stuff because https://github.com/tshort/WebAssemblyCompiler.jl exists and appears to work, whereas I'm not sure the wasm stuff in here works or not. Alternative we could look at bringing the functionality from that package in to here. cc @tshort