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

mix2nix: init at 0.1.2 #121555

Merged
merged 5 commits into from
May 20, 2021
Merged

mix2nix: init at 0.1.2 #121555

merged 5 commits into from
May 20, 2021

Conversation

ydlr
Copy link
Contributor

@ydlr ydlr commented May 3, 2021

Motivation for this change

Mix2nix is a tool for generating nix expressions from a mix.lock file. It makes developing elixir applications with nix a little easier.

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.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label May 3, 2021
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels May 3, 2021
@happysalada
Copy link
Contributor

@petabyteboy you might be interested

@happysalada
Copy link
Contributor

First review. This might take a bit of time, but feel free to ask me for help with anything. I want to try to make this as easy as possible for you!

First positive thing, it builds without a problem! :-)

I do think there will be a problem with usage though. I've removed buildMix in that PR https://github.com/NixOS/nixpkgs/pull/112477/files
Feel free to bring it back. (I've also removed the escript bootstrap, you can bring it back as will if you thing it's needed).
It might be a good occasion to remove some of the dust from that file (I feel there are a lot of legacy stuff, but I could be wrong).

I've tested the generations of the deps.nix, it works as expected! (just like you mentioned in the readme, we can figure out the git deps later.). One thing that would be good though is to show how it might be used to build a project.
Currently the beam readme reference a mixDeps variable that is just a directory including all of the files of the dependencies. Since this generates a deps.nix that is an attribute set, maybe we can include an example of how to use it with a phoenix project? You mentioned in the readme that it was working for you. Perhaps you can share the way you use it?
If you need help updating the docs, just let me know and I can suggest something.

I'm adding here for reference other efforts at doing this
https://gitlab.com/manveru/mixnix/-/tree/master
https://github.com/transumption/mix-to-nix

@happysalada
Copy link
Contributor

One more thing regarding the current state of things.
Recently I tried fetchMixDeps with a git dependency, it doesn't work when useSandBox = true.
So I don't think we lose anything by recommending mix2nix in the beam documentation.
Once this is merged I will have a go at the git dependencies (if somebody doesn't beat me to it).

@ydlr
Copy link
Contributor Author

ydlr commented May 4, 2021

The reason I relied so heavily on the buildMix helper is that it let me define a set of packages that follow a pattern familiar to most other nix packages, then list the dependencies with beamDeps. I was working off of the stable Nixpkgs documentation, so I wasn't aware it was being removed.

An example of a (very simple) phoenix project that uses this is at https://git.imnotsoup.com/inkhorn. You can see from the default.nix file that it uses the buildMix helper, except it overrides the installPhase and adds a postBuild step to create a release.

Personally, I find buildMix to be an intuitive and useful way of describing mix builds. But I will take some time to catch up on the discussions around why it is being removed. I will also try to get familiar with MixRelease and fetchMixDeps.

@happysalada
Copy link
Contributor

I removed it, but only because It didn't properly build a release for a mix project, and I thought it was not being used.
Also, I wasn't sure all the stuff around the bootstrap script was being used. When I asked other people that i thought were active, nobody else used it.

I would say here, we can bring it back, we just have to test that each piece is actually being used and perhaps document it.
I would bring it and only use things that are needed by the mix2nix project. We can add on top as we find things are needed. If we ever encounter a problem, we can always dig into the git commits too find old potential solutions.

Let me know if you want more information on anything.

@happysalada
Copy link
Contributor

By just copying and pasting the old files, I get this error on building my project

error: builder for '/nix/store/43cr5fv0gl725jxh56pv5n0rli0wxcyn-cowboy_telemetry-0.3.1.drv' failed with exit code 1;
       last 10 log lines:
       > ===> Compiling hex_core
       > ===> Compiling rebar3_hex
       > ===> Verifying dependencies...
       > ===> Upgrading telemetry v0.4.2
       > ===> Analyzing applications...
       > ===> Compiling telemetry
       > ===> Compiling ranch
       > ===> Compiling _build/default/lib/ranch/src/ranch_protocol.erl failed
       > _build/default/lib/ranch/ebin/ranch_protocol.bea#:none: error writing file: permission denied
       >
       For full logs, run 'nix log /nix/store/43cr5fv0gl725jxh56pv5n0rli0wxcyn-cowboy_telemetry-0.3.1.drv'.
error: 1 dependencies of derivation '/nix/store/fqz5211s0phjki38nngngl747d3kcycj-union-0.0.1.drv' failed to build

I'm going to do some more testing to try to figure out how a potentially new buildMix should work. Perhaps we can call it buildMixDep? I'll update with anything I find out.

@ydlr
Copy link
Contributor Author

ydlr commented May 4, 2021

I am seeing similar errors with both cowboy_telemetry and telemetry_poller. In both cases, they are not able to find telemetry, even though telemetry builds fine and is symlinked into the build path for the dependent package.

Both packages use buildRebar3, which may be a starting point for finding the problem.

fetch-mix-deps (and fetch-rebar-deps) is a clever and practical solution. Still, I'd to hate to give up on an approach that creates a separate derivation for each hex package because of build failures of a few (or even many) individual packages.

@ydlr
Copy link
Contributor Author

ydlr commented May 4, 2021

Before giving any more attention to mix2nix, I am going to:

  1. Investigate packages not building with buildMix and buildRebar3. I'll make PRs if I'm successful. Then,
  2. Restore build-mix and mix-bootstraper. Expand documentation to reflect that buildMix and buildRebar3 may not always work, or may need customization, and that the fetch-mix-deps / mix-release approach may be more practical in many cases.

@happysalada
Copy link
Contributor

I completely agree! Let's make this approach work!

The only thing I think would be great, would be to start from a minimal buildMix and add things from there. Just to try to get rid of unused stuff that might be uneeded at this point.

I'm going to investigate further on my side as well, let's share findings here.

Thank you so much for taking this on!

This was referenced May 7, 2021
@happysalada
Copy link
Contributor

Ok, I think I have a fix for the problem.
I made a PR here to show also a couple of additional changes that I think should be made
#122415

I wrote about the gist of the fix there, but here is a brief description.
The rebar3-bootstrap script will symlink the dependencies to _build/default/lib. However even if the dependencies are there and compiled, it will still try to compile it's dependencies. It seems that the _checkouts directory should be used instead to symlink dependencies
https://rebar3.readme.io/docs/dependencies#checkout-dependencies
@dlesl sorry to bring you into this, but perhaps you have an opinion on this? If you don't have time or don't have an opinion, no worries at all. since the failure happens with buildRebar3 I thought you might have relevant knowledge.

I think the last step is to symlink dependencies in mix-release and it should be good. (if you would rather do this in a separate PR, no worries at all).

One more thing that we might want to have a look at is the way phases are defined inside build-mix
the

    configurePhase =
      if configurePhase == null
      then ''
        runHook preConfigure
        ${erlang}/bin/escript ${bootstrapper}
        runHook postConfigure
      ''
      else configurePhase;

is not very idiomatic no?
should it not be something like

   configurePhase = attrs.configurePhase or ''
      runHook preConfigure
        ${erlang}/bin/escript ${bootstrapper}
        runHook postConfigure
      '';

Let me know if anything.

@happysalada
Copy link
Contributor

One more thing.
Here is a link to the mix2nix that somebody else is using https://git.petabyte.dev/petabyteboy/nixfiles/src/branch/main/pkgs/pleroma/mixnix/mix2nix.nix#L129

I haven't been through all the different implementations that I put a link to here. Maybe we should go through them and try to integrate what can be integrated? This is just a thought, I'm not sure I have the motivation for this myself. Maybe we can just try to get feedback from the people who implemented the other versions?

@happysalada
Copy link
Contributor

Also, it feels a little weird that mix-bootstrap is written in erlang and not Elixir. Maybe I'm being overzealous here.

@happysalada
Copy link
Contributor

Oh, one more thing I noticed.
using mix2nix all the dependencies are used (even the dev ones). There should be a way to specify only prod dependencies ? That's not a major concern, maybe we can leave that for improvements.
I know the node2nix things has a flag just for that.

@happysalada
Copy link
Contributor

I just tested and just adding

  bootstrapper = ./mix-bootstrap;

inside mix-release won't work directly. It seems that the symlinked dependencies are not compiled.

@ydlr
Copy link
Contributor Author

ydlr commented May 10, 2021

I think for for mix-release, it should work if you first build the release, then copy the result to $out, instead of doing it all on one line.

@ydlr
Copy link
Contributor Author

ydlr commented May 10, 2021

I'm not against a flag for including only prod dependencies, but I don't think it is a priority. mix2nix creates a package definition for dev dependencies, but if you do not need them, you can simply not list them in your default.nix file.

I intentionally did not want mix2nix to try to write the user's default.nix file, which is a big difference between it and some of the similar projects.

@gleber
Copy link
Contributor

gleber commented May 11, 2021

@happysalada I am a bit confused. As far as I know rebar3 compile builds stuff into _build/default/lib/$APP/ebin directory for each APP present in the project. Since buildRebar3 does use rebar3 compile under the hood, the beam files also land inside _build directory. IIUC this makes it not possible to distinguish between rebar3- and mix-built dependencies.

For rebar3, If there's a _checkout directory with a dependency, the dependency gets compiled in place and then both path types _build/default/lib/$APP/ebin and _checkouts/$APP/ebin show up in the output of code:get_path().

@happysalada
Copy link
Contributor

@gleber thanks a lot for this clarification. I wonder where the lib/erlang/lib stuff comes from then.
I will test tomorrow removing things from the lib directory, if only things from the _build directory should be there, I'll add the default directory too.
I won't touch the rebar boostrap thing.
The only thing I'm still kind of wondering is if the rebar depencies should still work with the buildMix. Something like https://github.com/deadtrickster/ssl_verify_fun.erl, should that be compiled only with buildRebar3 or is it supposed to work with buildMix too?
If it's supposed to work with buildMix, then we have to include a conditional compilation step if there is a rebar.lock file. It's probably easier to just expect the erl stuff to compile with rebar3 at first.

@dlesl
Copy link
Contributor

dlesl commented May 11, 2021

@happysalada buildRebar3 moves the built artifacts into lib/erlang/lib. Maybe buildMix should do the same, since AFAIK once compiled, elixir packages are no different from erlang ones.

@gleber The _checkouts behaviour seems to have changed recently (I think it was rebar3 3.14.4), there's now _build/default/checkouts with ebin and some symlinks to src, priv etc. It's a bit annoying because these symlinks get created even if they don't lead anywhere 😄

@dlesl
Copy link
Contributor

dlesl commented May 11, 2021

@dlesl thanks a lot for the quick feedback!
Do you think that bare rebar3 would be useful everywhere? If you think it would be ok, then perhaps you can issue a separate PR for it, and we can ask people to try it.
I like the approach of not using the bootstrapper!

Opened #122633 to gather feedback :)

@gleber
Copy link
Contributor

gleber commented May 11, 2021

@happysalada buildRebar3 moves the built artifacts into lib/erlang/lib. Maybe buildMix should do the same, since AFAIK once compiled, elixir packages are no different from erlang ones.

Moving .beam files (and other files) into $out/lib/erlang/lib/{ebin,private,include,...} is a convention which has been chosen (arbitrary) in Nixpkgs for BEAM packages, so that if they depend one on another, the dependent's build rule can find .beam files of all dependencies. I think buildMix should put files into the same location as buildRebar3 does (we can change the location if we see a reason for it).

@happysalada
Copy link
Contributor

@gleber thanks a lot for the explanation!

src = fetchFromGitHub {
owner = "ydlr";
repo = "mix2nix";
rev = "b153a252f0689d3f6919e16c2df2fb3b6bcc9d2a";
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be updated to the version after the fix for ssl_fun thing

@happysalada
Copy link
Contributor

I've tested, moving elixir to nativeBuildInputs.
I think this just needs the update of mix2nix and this should be good to go.
When you have a moment of course, no rush.
Thanks again for the PR!

@ofborg ofborg bot requested a review from happysalada May 17, 2021 23:29
@happysalada
Copy link
Contributor

@NixOS/beam this PR introduces mix2nix, a tool for generating a deps.nix file from a mix.lock.
As far as I'm concerned, it's good to merge.
I'm leaving this open a couple of days in case you want to have a look.

Usage is not immediately clear, but there should be a following PR to integrate it into mixRelease and update the docs.
What is testable here is just generating a deps.nix file and that it should build properly.

@happysalada
Copy link
Contributor

Just a quick note to say there were some changes since the initial mix2nix release. I tested with a private app and with the elixir_ls repo. I didn't see a problem.
The biggest change being that when the mix.lock doesn't specify a builder, mix2nix will default to buildRebar3.
This comment is just for information and doesn't require anything done.

@happysalada
Copy link
Contributor

@ydlr I tried fixing a conflict from github, however github still shows that there is a conflict. Sorry to ask you this, but when you have a moment, can you rebase off master?

yurrriq
yurrriq previously approved these changes May 20, 2021
@yurrriq yurrriq dismissed their stale review May 20, 2021 01:26

rethinking src.rev

pkgs/development/tools/mix2nix/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/mix2nix/default.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested a review from yurrriq May 20, 2021 02:58
@yurrriq
Copy link
Member

yurrriq commented May 20, 2021

Thanks, @ydlr! I'll leave this for @happysalada to merge, but lgtm.

@happysalada
Copy link
Contributor

@yurrriq thank you for reviewing !

@ydlr thanks a lot for this PR!

@happysalada happysalada merged commit 36bdd75 into NixOS:master May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants