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

Don't call cargo when skip_compilation? is true #389

Merged
merged 1 commit into from
Oct 7, 2021

Conversation

karolsluszniak
Copy link
Contributor

This fixes a regression from #386 which made rustler_mix call cargo metadata even when skip_compilation? is set to true,
breaking setups in which the rust library is built separately, e.g. in separate docker stage with proper rust versioning and in which the elixir compilation stage doesn't have cargo.

This fix assumes that the external_resources attribute is irrelevant when skip_compilation? is set to true.

This fixes a regression from rusterlium#386 which made rustler_mix call
`cargo metadata` even when skip_compilation? is set to true,
breaking setups in which the rust library is built separately, e.g.
in separate docker stage with proper rust versioning and in which
the elixir compilation stage doesn't have cargo.

This fix assumes that the external_resources attribute is
irrelevant when skip_compilation? is set to true.
@evnu
Copy link
Member

evnu commented Oct 6, 2021

Thanks for the pull request! I wonder if we should instead skip calling into Rustler.Compiler.compile_crate/2 altogether if skip_compilation? is set. @scrogson what do you think?

@evnu evnu requested a review from scrogson October 6, 2021 17:43
@scrogson
Copy link
Member

scrogson commented Oct 6, 2021

I wonder if we should instead skip calling into Rustler.Compiler.compile_crate/2 altogether if skip_compilation? is set.

@evnu makes sense to me.

@evnu
Copy link
Member

evnu commented Oct 6, 2021

I wonder if we should instead skip calling into Rustler.Compiler.compile_crate/2 altogether if skip_compilation? is set.

@evnu makes sense to me.

I think we should go ahead with this PR as is and release a patch version, and refactor later.

@evnu evnu merged commit fbedf17 into rusterlium:master Oct 7, 2021
evnu added a commit that referenced this pull request Oct 7, 2021
@evnu
Copy link
Member

evnu commented Oct 7, 2021

@karolsluszniak I released v0.22.2 with this fix. Thanks again!

@karolsluszniak
Copy link
Contributor Author

karolsluszniak commented Oct 7, 2021

@evnu @scrogson Thanks for a quick turnaround! I basically pushed this at the end of the day in Warsaw, and here's a freshly baked package right in the morning!

Btw I've stumbled upon some trouble when creating the multistage Dockerfile with separate rust stage based on rust-alpine. And the final result is great with properly versioned rust & elixir, optimized layer caching & final image @ 40 MB. Perhaps a complete example would be fitting? If so, how would you prefer I do it (PR with guide in Hexdocs, PR with Markdown guide in repo, blog post - I write at https://cloudless.studio)?

@karolsluszniak karolsluszniak deleted the no-cargo branch October 7, 2021 08:59
@evnu
Copy link
Member

evnu commented Oct 11, 2021

@karolsluszniak I actually don't know where such a guide would be placed best. We don't have guides yet in the repository, but something like that might be nice. Maybe @hansihe can chime in here. A simple directory within the repository with guides as Markdown sounds reasonable to me.

@karolsluszniak
Copy link
Contributor Author

karolsluszniak commented Oct 11, 2021

👍 Personally, I'd introduce the Guides section in rustler_mix

  • looks nice, e.g. here's how I did it for one of my own packages: https://hexdocs.pm/sea/getting_started.html
  • this particular problem is specific to Elixir side of things (and probably most future guides will be except for some deep details of Rust parts, but that's probably more of a material for inline docs)
  • this makes the guide very visible when visiting both Hex and Github (by putting a simple readme link to markdown file)

Btw another guide that I could do is to explain how to setup code testing & formatting in local development and on CI using the ex_check package. It would be a shameless plug :), but really works great for rustler. Basically, you put .check.exs with:

[
  tools: [
    {:mycrate_formatter, "cargo fmt -- --check", cd: "native/mycrate", fix: "cargo fmt"},
    {:mycrate_test, "cargo test", cd: "native/mycrate"}
  ]
]

And then mix check validates runs elixir & rust checks & formatters, all with one command.

Let me know what you think. I love when things have neat docs, making the getting started process effortless.

@hansihe
Copy link
Member

hansihe commented Oct 11, 2021

I have been working on more guide like documentation in here. https://github.com/rusterlium/web/tree/main/docs

Master of that repo is automatically published to https://rustler-web.onrender.com/, so feel free to submit a PR if you have anything you want included.

@hansihe
Copy link
Member

hansihe commented Oct 11, 2021

@scrogson what's status on getting DNS for http://www.rusterlium.org/ pointed to that?

@karolsluszniak
Copy link
Contributor Author

karolsluszniak commented Oct 11, 2021

@hansihe Thanks for the heads up! I either missed that or it wasn't actually linked anywhere yet. Nice to see some intro to mapping types as I was indeed missing that e.g. for maps/structs in the docs.

Of course it's not my call, but if I may make a suggestion, I'd reconsider moving that do Hexdocs guides because that's how mainstream packages do it - including Phoenix (which at some point moved the docs completely from phoenixframework.org), LiveView, Absinthe, Ecto and many more. For that reason, most Elixir devs will first look on Hexdocs. And you'd have site design, DNS etc out of your list of concerns.

EDIT: and of course there could be a rusterlium.org website + Hexdocs link like on Phoenix website - e.g. if you do want to establish branding, embed videos or do other stuff that are out of Hexdocs's scope.

@hansihe
Copy link
Member

hansihe commented Oct 11, 2021

@karolsluszniak To give some background on why I think it's a good idea to maintain guides/high level documentation outside of the hex docs:

The core of Rustler is the Rust library. This means that the user will presumably use the cargo docs for most of their engagement with the library. The hex documentation documents how to use the elixir library, but this is pretty much just a utility library, with not a lot of direct coupling to the Rust parts.

Although I am not 100% on their current status, there are even libraries that enable you to compile rustler NIF crates with rebar3.

The guides we write should aim to document all parts of rustler, the Elixir compiler, the Rust library, and even third party libraries like the rebar plugin or the serde serialization/deserialization library. Because of this, I think it's a good idea in general to put high level docs outside the api-level documentation for any parts of Rustler.

I would very much like to avoid dropping non-rustler_mix specific documentation into the docs for rustler_mix. Of course the documentation we are talking about here could probably fit in quite well to the hex documentation, but if we are doing guides on a standalone site, I think we might as well not have separate locations for documentation like that.

I am of course open to input on this, these are just my thoughts.

@karolsluszniak
Copy link
Contributor Author

karolsluszniak commented Oct 11, 2021

@hansihe 👍 Your reasoning is clear, thanks for the explanation.

Indeed, proper doc facilitation for multi-tech library ain't simple. Hex may not work well for Rust APIs and vice-versa. So perhaps putting it higher on separate website with proper linking to all pieces seems reasonable, although much more work to do (but package with 3k stars will probably cope with that :)).

For what it's worth, my very subjective take is more Elixir-ish than Rust-ish, I'm used to Hexdocs much more than to crates.io and so "Hexdocs by default + only that which cannot be there linked outside" seems most straightforward and in line with what other top libs do (like LiveView that covers JS pieces in Hexdocs as well - but as you pointed rustler does much more rust than live view does js :)).

Regardless, what really matters is for guides to be out there and be linked across, so that more devs will embrace the wonderful Elixir + Rust combo. 🚀

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.

None yet

4 participants