-
Notifications
You must be signed in to change notification settings - Fork 225
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
Common cargo #302
base: master
Are you sure you want to change the base?
Common cargo #302
Conversation
0b435b3
to
52f2574
Compare
I will clean this up further (and will publish |
@@ -24,7 +24,6 @@ defmodule Mix.Tasks.Rustler.New do | |||
|
|||
for {format, source, _} <- @basic do | |||
unless format == :keep do | |||
@external_resource Path.join(root, source) |
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 and the other occurrences of @external_resource
will be readded.
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.
Hmm, actually I don't like this, it would mean (if I understand things correctly) that automatic recompilation is only triggered if lib.rs
is changed, not if any of its dependencies (like other .rs
files or a crate dependencies) are changed, is that right?
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.
Yes, I'll do this via __mix_recompile__?
instead.
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.
__mix_recompile__?
seems very new, I think we might not be able to support older versions of Elixir with that anymore?
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.
I just found it in the sources. Worst case: It won't be better than what we have now :)
The issue is that you can't really tell mix
up-front what exact files will influence the final object files of a Rust project. I'll think about it.
* `:env` - Specify a list of environment variables when envoking the compiler. | ||
|
||
* `:features` - a list of features to enable when compiling the crate. | ||
|
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.
I can probably readd this if required.
@@ -52,12 +47,6 @@ defmodule Rustler do | |||
- When `Mix.env()` is `:dev` or `:test`, the crate will be compiled in `:debug` mode. | |||
- When `Mix.env()` is `:prod` or `:bench`, the crate will be compiled in `:release` mode. | |||
|
|||
* `:path` - By default, rustler expects the crate to be found in `native/<crate>` in the | |||
root of the project. Use this option to override this. | |||
|
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 is not necessary anymore, as we now expect a Cargo.toml
in the root of the project.
c5a278c
to
4975455
Compare
b00b210
to
76ff5dc
Compare
end | ||
shell.info(" Copying #{rel_filename} to #{rel_dest}") | ||
File.mkdir_p!(Path.dirname(dest)) | ||
File.copy!(filename, dest) |
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.
I think we should delete the destination first, see the deleted code: https://github.com/rusterlium/rustler/pull/302/files#diff-cb1f5883582c0a6149e7a7c774f4702cR39
We found a problem due to mmap
when simply copying, see #128: If another node is currently using the NIF, it has the lib mmap'ed, which results in segfaults if we overwrite it with copy.
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.
Actually, I'd rather have this case error out entirely. You can get a new version pushed (safely) by bumping the crate version (there might be some trickyness involved with dynamic linkers that remember the filenames). On Windows you will never be able to overwrite an in-use DLL file.
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.
To give some more context: I encountered segfaults when running mix test
and editing+compiling in two separate terminals. So this is something which we need to guard against here.
TODO:
|
c0fc38e
to
c579b20
Compare
- Implement basic compile functionality to make tests pass - Run a single common compilation
@filmor should we pick this PR up again? Can I somehow help to move it along? |
As a first step, I'll rebase it. Maybe we could have a call on this to talk about the next steps? |
Yep, lets do that. |
Use https://github.com/rusterlium/erlang-cargo/ to have a common understanding with https://github.com/rusterlium/rebar3_cargo/ about how to compile and where to put the results.