-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
erlang: refactor to generalize Erlang/OTP derivations #26381
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
Conversation
erlang erlang_odbc erlang_javac erlang_odbc_javac | ||
elixir | ||
lfe | ||
erlangR16 erlangR16_odbc |
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.
Do we really have to expose all these versions toplevel? See #24047 (comment).
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.
Probably not, but let's leave these two aspects separate. I want this PR to be purely a refactoring.
I'm mobile atm. Does this include the generic LFE builder from my fork? |
@yurrriq It does not, on purpose. This PR is supposed to be purely a refactoring. |
It builds on Ubuntu, but still fails on NixOs, debugging |
I'm also getting a failure on OSX 10.11.6 You can see the entire output of nox-review here |
c9d5451
to
39cf2ae
Compare
It compiles now both on Ubuntu and NixOs (or more specifically all Erlang/OTP derivations compile and nox preview has been running for 3 hours now and still did not fail). Please review. |
pkgs/top-level/beam-packages.nix
Outdated
rec { | ||
lib = import ../development/beam-modules/lib.nix { inherit pkgs; }; | ||
|
||
inherit (pkgs) darwin wxGTK30; |
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.
these can be moved to the arguments, now they end up in the beam
attrset
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 replaced 'foo' with 'pkgs.foo' everywhere below.
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.
Moved these to arguments of this file (i.e. lambda defined at top level of this file)
|
||
buildInputs = [ perl gnum4 ncurses openssl autoconf libxslt libxml2 makeWrapper ] | ||
++ optionals wxSupport wxPackages2 | ||
++ optional odbcSupport odbcPackages |
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.
use optionals
since it's already a list, nested buildInputs work but it should be avoided
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.
Done
Please note that "mkDerivation" defined here is the one called from R16.nix and similar files. | ||
*/ | ||
callErlang = drv: gargs: args: pkgs.callPackage drv ( |
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.
Using makeOverridable
for the arguments instead here would be more flexible and also makes the top-level definitions a bit nicer.
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 applied this patch
nox-review passed on OSX 10.11.6 |
This applies to Erlang, Elixir and LFE packages. beam-packages provides interpreters and packages sets separately. This is in preparation of generalizing BEAM interpreters definitions.
Switch official Erlang versions to use a common builder.
@LnL7 All fixed. Please take a look again (PTAL) |
@LnL7 I have added a commit which moves basho's fork over too. PTAL |
Failing again on OSX for the basho stuff. https://gist.github.com/ankhers/e36e7e6f4908446ec6de6c0ee30ff592 |
I'm seeing an issue with
|
Another shebang issue (on darwin), not sure what's going on here.
|
@LnL7 That's this line: Previously Basho Erlang fork would require another Erlang to be already installed so that the There are three options:
I think I have found a solution using the last option. Can you retest? |
I think I have found a less hacky way which should be more resilient. Can you retest it on OS X? |
Nox review just finished and everything built on OSX. |
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.
Everything looks good now.
Thanks for finishing this! |
Yay! There's still plenty to do |
Motivation for this change
This PR does not change anything feature-wise, and instead aims at unify how different Erlang versions are built.
This is a subset of #17240 (and is a heavily-inspired re-implementation of it). It contains:
It does not contain:
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandbox
innix.conf
on non-NixOS)
nix-shell -p nox --run "nox-review wip"
./result/bin/
)/cc @LnL7 @ankhers @ericbmerritt @yurrriq