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

buildRebar3: use rebar3 bare compile #122633

Merged
merged 3 commits into from
May 17, 2021
Merged

Conversation

dlesl
Copy link
Contributor

@dlesl dlesl commented May 11, 2021

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@dlesl
Copy link
Contributor Author

dlesl commented May 11, 2021

I have opened this PR to get feedback on this idea which I think could simplify buildRebar3 a bit. It uses the rebar3 bare compile command which as I understand it was added for use by mix to achieve a similar aim (compilation without any deps fetching).

@petabyteboy you mentioned that you were using buildRebar3, would you be interested in testing this?

I removed the compilePorts option because I wasn't sure what it's used for; shouldn't rebar3 packages that include native code also include the machinery to build it, for example by using pc via hooks? There aren't any packages in nixpkgs using it. @gleber you might have some more context here, and maybe some feedback on the approach generally?

This was referenced May 11, 2021
buildPhase = ''
runHook preBuild
HOME=. rebar3 compile
${if compilePorts then ''
HOME=. rebar3 pc compile
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that removal of this and use of rebar3 bare compile will break projects which use the recommended rebar3 NIF/port compilation mechanism as described here: https://github.com/blt/port_compiler#use-in-your-project

Previously one could set compilePorts to true and the ports would have been force-compiled. If compilePorts is not set, the hooks in rebar.config could have triggered to compile them. Now both mechanisms are gone.

I suspect that if we ditch rebar3 compile we'll be forced to reproduce a lot of rebar3's logic, while in reality we want to replace just the dependency fetching part (or any other network-using functionality).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was that if a project used the pc configuration with provider_hooks, then hopefully it would be sufficient to make pc available through the buildPlugins argument? I guess what we need to do is find a few packages with native dependencies and test them.

Regarding the rebar3 logic we might be missing out on, I'm also worried about this. From these docs it looks like compile depends on install_deps which depends on lock. I'll look into what those steps do

Copy link
Contributor Author

@dlesl dlesl May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the link to packages using compilePorts, that looks like a good starting point for testing this!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that packages should use the hooks, but IIRC a lot of packages in hex.pm in ~2015 did not do that in practice. Also the provider_hooks recommended in the port_compiler README won't trigger for rebar3 bare compile, since it is not the rebar3 compile which everyone sets the hooks for.

Do you know if we can use rebar3 bare compile and rebar3 pc compile together at all? If so, maybe keeping compilePorts flag as an option might be a good middle ground.

IMO the best solution is to introduce a REBAR3_HERMETIC flag of sorts into upstream of rebar3 so we can use it. I tried and failed to convince rebar maintainers to make it so.

Second best might be to maintain an always-included rebar3 patch, so that we can set the flag only inside of buildRebar3.

Copy link
Contributor Author

@dlesl dlesl May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fortunately it looks like rebar3 bare compile still triggers the provider_hooks! I tested a few different nif-containing packages below (some use pc, others like katipo and jiffy have their own custom compilation) and it all seems to work, I'm quite surprised actually. So I think we can drop compilePorts, at this stage I think if we encounter any packages for which it doesn't work, the best approach would be to fix those specific packages

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unexpected and great news!

@gleber
Copy link
Contributor

gleber commented May 11, 2021

@dlesl There were some packages in nixpkgs which used to use compilePorts flag and which did not have properly configured hooks for calls into pc. I do not remember which one it was though.

@gleber
Copy link
Contributor

gleber commented May 11, 2021

Found its use in https://raw.githubusercontent.com/NixOS/nixpkgs/a21cb752426a57888d8bc0ef957b45e7033177e7/pkgs/development/beam-modules/hex-packages.nix in multiple packages, e.g. erlexec.

@r-rmcgibbo
Copy link

r-rmcgibbo commented May 11, 2021

Result of nixpkgs-review pr 122633 at bb52ccef run on aarch64-linux 1

1 package marked as broken and skipped:
  • lfe_1_2
1 package failed to build:
  • lfe (lfe_1_3)
3 packages built successfully:
  • elixir_ls
  • rebar3
  • relxExe

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.


Result of nixpkgs-review pr 122633 at bb52ccef run on x86_64-linux 1

1 package marked as broken and skipped:
  • lfe_1_2
1 package failed to build:
  • lfe (lfe_1_3)
3 packages built successfully:
  • elixir_ls
  • rebar3
  • relxExe
2 suggestions:
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/development/tools/build-managers/rebar3/default.nix:41:5:

       |
    41 |     installPhase = ''
       |     ^
    
  • warning: missing-phase-hooks

    buildPhase should probably contain runHook preBuild and runHook postBuild.

    Near pkgs/development/tools/build-managers/rebar3/default.nix:37:5:

       |
    37 |     buildPhase = ''
       |     ^
    

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.

[ -d "$fd" ] || continue
cp -Hrt "$out/lib/erlang/lib/${name}-${version}" "$fd"
success=1
[ -d "$reldir" ] || continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just asking a question since I'm not sure of what was happening before and after.

As I understand it
Before

  • in the build phase files are compiled to _build/default/${name}
  • check for the presence of a specific directory (src ebin priv include) inside the build
  • if the directory is there, copy the contents of the directory into the convention defined path
    After
  • check for the presence of (src ebin priv include) directly inside $src
  • if the directory is there, copy it's content too the convention defined path

In this pr does this mean that the bare compile will compile directly into $src and not into _build ?
There is one step I'm missing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, yes you've got this exactly right. AFAIK the only documentation for rebar3 bare compile is the source code, there's a parameter called outdir but it defaults to the working directory.

@happysalada
Copy link
Contributor

One more question I have is around the making of symlinks.
if rebar3 finds dependencies with $ERL_LIBS is there a need even to make the symlinks in the first place?
The initial bootstrap script is making symlinks into _build/default/lib . Is the reason so that rebar does not compile it again?
Therefore using rebar3 bare compile would mean that those symlinks are no longer needed?

@dlesl
Copy link
Contributor Author

dlesl commented May 12, 2021

One more question I have is around the making of symlinks.
if rebar3 finds dependencies with $ERL_LIBS is there a need even to make the symlinks in the first place?

Rebar3 looks in ERL_LIBS but only as a last resort, so if deps is specified in rebar.config it will always go out to the internet to try to fetch those, the symlinking is necessary to prevent that. I'm not sure how it stops rebar3 from trying to recompile the dependencies, but it seems to work somehow 😆

The initial bootstrap script is making symlinks into _build/default/lib . Is the reason so that rebar does not compile it again?
Therefore using rebar3 bare compile would mean that those symlinks are no longer needed?

That's what I'm hoping!

@dlesl dlesl force-pushed the rebar3-bare-compile2 branch from bb52cce to 6b494f0 Compare May 12, 2021 16:27
@dlesl
Copy link
Contributor Author

dlesl commented May 12, 2021

Some test expressions:

includes.nix

This tests compiling code that uses -include_lib from a dependency

{ pkgs ? import <nixpkgs> { } }:
with pkgs;
let
  cowboyDeps = {
    ranch = fetchFromGitHub {
      owner = "ninenines";
      repo = "ranch";
      rev = "a692f44567034dacf5efcaa24a24183788594eb7";
      sha256 = "03naawrq8qpv9al915didl4aicinj79f067isc21dbir0lhn1lgn";
    };
    cowlib = fetchFromGitHub {
      owner = "ninenines";
      repo = "cowlib";
      rev = "e9448e5628c8c1d9083223ff973af8de31a566d1";
      sha256 = "1j7b602hq9ndh0w3s7jcs923jclmiwfdmbfxaljcra5sl23ydwgf";
    };
  };
  beamDeps = lib.mapAttrsToList (name: src:
    beamPackages.buildRebar3 {
      inherit name src;
      version = "none";
    }) cowboyDeps;
  cowboy = beamPackages.buildRebar3 rec {
    inherit beamDeps;
    name = "cowboy";
    version = "2.8.0";
    src = fetchHex {
      inherit version;
      pkg = name;
      sha256 = "sha256-RkPk+6dKyW1NFSx1gD3m+tCz+l3zVMca/dbL7rFfrIo=";
    };
  };
in mkShell {
  name = "cowboy-shell";
  buildInputs = [ erlang cowboy ];
}
nifs.nix

This is just a bunch of modules I could think of that use nifs

{ pkgs ? import <nixpkgs> { } }:
with pkgs;
let
  deps = {
    worker_pool = fetchHex {
      pkg = "worker_pool";
      version = "4.0.3";
      sha256 = "sha256-Lyq/0/ZJMbjfNF1+/oCjV+3HhYSMOTGSJcoyj0S8cZI=";
    };
    metrics = fetchHex {
      pkg = "metrics";
      version = "2.5.0";
      sha256 = "sha256-rgYZOMjDvel9F76aeStj4VweHv/hZZirh6YjhPXzcUs=";
    };
    snappyer = fetchHex {
      pkg = "snappyer";
      version = "1.2.8";
      sha256 = "sha256-NVGOeaKFSLVtj9au4vVl8S9RwtPQU/nPqBfIO+iMTz0=";
    };
    katipo = fetchHex {
      pkg = "katipo";
      version = "1.0.1";
      sha256 = "sha256-j84vFnm76ayQC7On5cUhK8x2fEh6Y7pZSMfISmHvGKI=";
    };
    jiffy = fetchHex {
      pkg = "jiffy";
      version = "1.0.8";
      sha256 = "sha256-+a6Ya6WghU60jPanYZLZNnCG2obCAZfaQwYwvnwIek4=";
    };
    erlexec = fetchHex {
      pkg = "erlexec";
      version = "1.18.11";
      sha256 = "sha256-K6ko8b33TgmMQTRl8QnBC10eE1lWjre/TJn5GsmDuP8=";
    };
    crc32cer = fetchHex {
      pkg = "crc32cer";
      version = "0.1.10";
      sha256 = "sha256-Wx9H79ChtLdBHx814U08jG2m5qKnJeyPLPGrE3A+Xzg=";
    };
  };
  compiled = lib.mapAttrsToList (name: src:
    beamPackages.buildRebar3 {
      inherit name src;
      buildInputs = [ libevent curl ];
      buildPlugins = [ beamPackages.pc ];
      version = "none";
    }) deps;
in mkShell {
  name = "nifs-shell";
  buildInputs = [ erlang ] ++ compiled;
}

How I tested:

$ NIX_PATH=nixpkgs=$NIXPKGS_CHECKOUT nix-shell nifs.nix --run erl

For cowboy, I just tested application:ensure_all_started(cowboy). For the nifs, I tested a nif-requiring function from each package and everything seemed to work!

@dlesl
Copy link
Contributor Author

dlesl commented May 12, 2021

BTW I had to update pc to get it to work, I think that is due to unrelated changes in OTP 23. I can move that to a separate PR if that would be neater

@ofborg ofborg bot requested a review from happysalada May 12, 2021 16:44
@dlesl dlesl force-pushed the rebar3-bare-compile2 branch from 6b494f0 to 1f1b199 Compare May 12, 2021 16:47
@gleber
Copy link
Contributor

gleber commented May 12, 2021

@dlesl Wow! I did not expect it to work :) I have a non-trivial private rebar3 project (although without nifs), which I'll try this out on.

Do you know what causes lfe failures?

@dlesl
Copy link
Contributor Author

dlesl commented May 12, 2021

It had some hooks in its rebar.config that were trying to write to $REBAR_DEPS_DIR (ie. _build). We could look into creating a _build directory for that, especially if this turns out to be a common pattern, but probably lfe is a bit special there? I updated the rebar.config patch to fix it

@berbiche berbiche mentioned this pull request May 12, 2021
@happysalada
Copy link
Contributor

As I understand it, this is good to merge. Leaving this open a little more in case somebody wants to comment.
@dlesl let me know if I misunderstood anything of course.

@dlesl
Copy link
Contributor Author

dlesl commented May 15, 2021

@happysalada I agree, I think it's ready to merge, but I would be interested to hear how things went with @gleber's private rebar3 project first. I did a bit more testing by just going through the top packages on hex.pm, and adding those that looked interesting (in an admittedly non-scientific way) to https://github.com/dlesl/build-rebar3-hex-test and was able to compile them all with this (and that didn't work with the buildRebar3 on nixpkgs master)

@gleber
Copy link
Contributor

gleber commented May 15, 2021

@dlesl Sorry it took a while. I tried building the app with this and could not make it work. I am actually not sure what I am doing wrong here.

I've distilled the problem into a standalone repo at https://github.com/gleber/buildRebar3-test-app which pins Nixpkgs to https://github.com/dlesl/nixpkgs/tree/rebar3-bare-compile2 using Niv.

Please take a look what happens when you do:

git clone https://github.com/gleber/buildRebar3-test-app.git
cd buildRebar3-test-app
nix-build

@dlesl
Copy link
Contributor Author

dlesl commented May 16, 2021

@gleber This buildRebar3 will only be able to build projects that are valid as rebar3 deps, ie. single-app libs. I think building releases and escripts is what we need to tackle next (in a new PR). For example we could modify rebar3Relx to accept the beamDeps argument, so we can pass in precompiled deps. I think rebar3 release will work out-of-the-box with this setup, but rebar3 escriptize uses a different method to resolve dependencies that doesn't play nice with ERL_LIBS, so that will take a bit more work (with the current _checkouts/recompile everything solution as an interim solution).

rebar3Relx compiles an entire umbrella project in one go, but there's another way we could go about things which I think has a lot of potential, namely building each app in the project individually with buildRebar3. This has nice properties such as letting nix do fine-grained change detection and also lets nix build all of the apps in parallel (as far as inter-app dependencies allow). I think this could be really useful in monorepo setups. I've attached a demonstration of what I mean below (in the context of your buildRebar3-test-app repo). The other thing I had to change to build it was to change the deps to use the style output by erlang-nix/rebar3_nix#2, since cowboy has a compile-time dependency on cowlib. I put everything into one file so it's a bit of a mess.

default.nix
let
  pkgs = import (builtins.fetchTarball
    "https://github.com/dlesl/nixpkgs/archive/rebar3-bare-compile2.tar.gz") { };
  depsFn = { builder, fetchHex, fetchFromGitHub }: rec {
    ranch = builder {
      name = "ranch";
      version = "1.7.1";
      src = fetchHex {
        pkg = "ranch";
        version = "1.7.1";
        sha256 = "sha256-RR2FJ3h99xbZncNhYvygWTSRXbC2FBu9rC6o08evx9c=";
      };
      beamDeps = [ ];
    };
    cowlib = builder {
      name = "cowlib";
      version = "2.9.1";
      src = fetchHex {
        pkg = "cowlib";
        version = "2.9.1";
        sha256 = "sha256-5BddwkCnDZlhVhYIkeHGIjjt4XKeRXQL3TgGTa1HYXA=";
      };
      beamDeps = [ ];
    };
    cowboy = builder {
      name = "cowboy";
      version = "2.8.0";
      src = fetchHex {
        pkg = "cowboy";
        version = "2.8.0";
        sha256 = "sha256-RkPk+6dKyW1NFSx1gD3m+tCz+l3zVMca/dbL7rFfrIo=";
      };
      beamDeps = [ cowlib ranch ];
    };
    lager = builder {
      name = "lager";
      version = "3.9.1";
      src = fetchHex {
        pkg = "lager";
        version = "3.9.1";
        sha256 = "sha256-P1m6daBKmeXxi/kcifRtzlNvg8bLQV/ibm51pivvN9w=";
      };
      beamDeps = [ ];
    };
  };
  deps = builtins.attrValues (depsFn {
    inherit (pkgs) fetchHex fetchFromGitHub;
    builder = pkgs.beamPackages.buildRebar3;
  });
  appsDirs = pkgs.lib.filterAttrs (_: type: type == "directory")
    (builtins.readDir ./apps);
  mkApp = name: src:
    pkgs.beamPackages.buildRebar3 {
      inherit name src;
      beamDeps = deps;
      version = "none";
    };
  apps = pkgs.lib.mapAttrsToList mkApp
    (pkgs.lib.mapAttrs (name: _: ./apps + "/${name}") appsDirs);

in pkgs.stdenvNoCC.mkDerivation {
  name = "test-app";
  version = "0";
  buildInputs = with pkgs; [ makeWrapper ] ++ apps;
  unpackPhase = "true";
  installPhase = ''
    mkdir -p $out/bin
    cat <<EOF > $out/bin/test-app
    #!${pkgs.erlang}/bin/escript

    main(Args) ->
      myapp:main(Args).
    EOF
    chmod +x $out/bin/test-app
    wrapProgram $out/bin/test-app --set ERL_LIBS "$ERL_LIBS"
  '';
}

@dlesl dlesl mentioned this pull request May 17, 2021
10 tasks
@dlesl
Copy link
Contributor Author

dlesl commented May 17, 2021

Update: erlang/rebar3#2552 has been merged making it possible to use ERL_LIBS with rebar3 escriptize. I've opened #123384 to demonstrate how that could work with rebar3Relx using erlang-ls as an example

@happysalada
Copy link
Contributor

Let's merge this and take the discussion around building projects in that other PR.

@happysalada happysalada merged commit 9f4878d into NixOS:master May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants