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.

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Common cargo #302

wants to merge 8 commits into from

Conversation

filmor
Copy link
Member

@filmor filmor commented Feb 6, 2020

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.

@filmor filmor force-pushed the common-cargo branch 2 times, most recently from 0b435b3 to 52f2574 Compare April 25, 2020 20:56
@filmor filmor marked this pull request as ready for review April 30, 2020 12:29
@filmor
Copy link
Member Author

filmor commented Apr 30, 2020

I will clean this up further (and will publish erlang-cargo to Hex for use in here), but this is finally passing all tests, would be nice if you could review and also check in your own applications, whether this works for you.

@@ -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)
Copy link
Member Author

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.

Copy link
Member Author

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?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

@filmor filmor force-pushed the common-cargo branch 2 times, most recently from c5a278c to 4975455 Compare May 3, 2020 13:34
.github/workflows/main.yml Outdated Show resolved Hide resolved
@filmor filmor force-pushed the common-cargo branch 2 times, most recently from b00b210 to 76ff5dc Compare May 4, 2020 09:47
end
shell.info(" Copying #{rel_filename} to #{rel_dest}")
File.mkdir_p!(Path.dirname(dest))
File.copy!(filename, dest)
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

@filmor
Copy link
Member Author

filmor commented May 18, 2020

TODO:

  • Add load_from back to side-step the whole recompilation process
  • Allow overriding the default env-dependent is_release per module

@evnu
Copy link
Member

evnu commented Mar 18, 2022

@filmor should we pick this PR up again? Can I somehow help to move it along?

@filmor
Copy link
Member Author

filmor commented Mar 19, 2022

As a first step, I'll rebase it. Maybe we could have a call on this to talk about the next steps?

@evnu
Copy link
Member

evnu commented Mar 21, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants