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
asls: init at 0.4.0 #91133
asls: init at 0.4.0 #91133
Conversation
ea0f4aa
to
59ae5f8
Compare
/marvin opt-in |
Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here. |
/status needs_review |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
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 have added an initial set of comments. Thanks for your first submission!
src = fetchurl { | ||
url = "https://github.com/saulecabrera/asls/releases/download/v0.3.0/bin.tar.gz"; | ||
sha256 = "01d6v79zqw62pkv33km090nyn78zxr0vs3yvnw24ykysvqjyaqd3"; | ||
}; |
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.
Why not build from 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.
Two main reasons why I went with this approach:
- The available tools for the BEAM seem to have several drawbacks not suited for this package:
buildHex
: requires the package to be published in https://hex.pm/ which is not the case forasls
buildMix
: requires the package to be a library,asls
is an application (BEAM CLI application)
- The other option is to run
mix
commands during the build phase, that works and that was my initial approach, but that breaks sandboxing (a similar case can be found in this thread: https://discourse.nixos.org/t/question-about-packaging-elixirls/4056)
Until there is a more mature built-in approach I believe this is the most straightforward way in this particular case.
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.
Didn't mean to approve, sorry ;).
Thanks for your initial comments @danieldk. I've polished the PR, it's ready for another round of reviews. |
Thanks, I am afraid that I cannot add further comments, since I am not familiar with the Erlang ecosystem. So, I think at this point it's best to wait for someone with Erlang experience to do another review. |
Agreed. @DianaOlympos would you have some time for a review? (Apologies for randomly pinging you, but I've noticed that you have been very involved in Nix <> BEAM conversations, if that's not the case, please disregard this message 😄) |
I don't see anything problematic here. Does remind me i need to bring erlang 23 though. LGTM |
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.
Just one small nit, other than that looks good to me.
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.
Looks good to me now (pending ofBorg's approval), welcome to the team!
Motivation for this change
Add a new package for the AssemblyScript Language Server
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)