beamPackages.mixRelease: deterministic erlang builds#271288
beamPackages.mixRelease: deterministic erlang builds#271288happysalada merged 5 commits intoNixOS:masterfrom
Conversation
|
Hey @mattpolzin, I just tested your code in the branch It works for all the
|
|
Ah, interesting. If this PR merges, I'd be happy to look into a similar change for the Elixir build since I don't imagine that will be any more complicated. |
|
Hey, @mattpolzin . I have created a PR for deterministic Elixir builds. After that PR is merged, we should do something on this PR:
|
I was hoping to keep this PR very targeted and follow it with another PR to do any remaining work to clean up Erlang references. I’m ok either way in the end. |
|
@NobbZ you've taken a crack at this before as well I believe. Pinging in case you've got any experience with the |
|
Hi, @mattpolzin. The PR of elixir deterministic build has been merged. Can you rebase your branch on Then, I can:
Then, you can merge my contributes into your branch. And the code will appear on this PR. |
cd8890c to
ba21735
Compare
|
@c4710n sure thing. rebased. |
|
Given that we had problems in the past with software that relied on certain stack information available, I am not a friend of making this an unalterable default. We need to check whether all currently packaged BEAM applications can actually be built and run using this option. Based on that we should choose a default that then can be toggled. |
|
@NobbZ i can throw in a Boolean toggle for this option. I'm guessing based on your comment that you would lean toward defaulting that off so that we don't break any existing packages rather than on so that any broken packages need to be updated to override the toggle, but want to confirm that with you. |
|
Ive read his comment as
I agree with both of these statements. |
…rministic Beam packages mix release deterministic
|
I've merged in changes from @c4710n, who wanted to go ahead and wrap up the remaining work to remove They asked me to post the following after merging their changes: This PR is trying to add funtionality for:
We can take Before this PR: After this PR: |
|
Yeah, basically what happysalada wrote. We should perhaps also ping maintainers of affected packages to let them briefly check and opt for an override early if they know that there will be a breakage, such that they have time to fix. When I completely disabled debugsymbols by default it took about 3 months until the broken packages have been recognized and the root cause identified. Then stripping got disabled by default and needs to be enabled by anyone who actually needs it, eg. to create a release in a minified OCI container. This time we should work closer with affected maintainers before simply changing things. |
|
I'll have a change pushed that adds that default-on toggle to both Then I'll look into who I should ping as package maintainers. |
|
Here's a heads up to at least one maintainer from each of the following packages that use either Hopefully this does not have a negative impact on any existing Nixpkgs projects, but please let me know if you foresee any problems with building or running your packages as a result. Any testing of this new feature against projects you maintain is greatly appreciated, but I realize that's a lot to ask depending on what else you have going on.
|
|
Exciting work @mattpolzin! Everything seems to be in order with the livebook build. @happysalada perhaps you'd like to chime in whether you think this will affect the livebook service? Edit: Oops, you're already here @happysalada! I meant to tag @scvalex instead. Sorry! |
|
before I forget, Thank you also to everyone who took time out of their day to comment/review ! if everyone is fine, I'd like to wait for one week (it's an unofficially considered "enough" time for people to review) before merging this. |
|
I tested the That said, while mentions of erlang have disappeared from the |
Agreed, that sounds intentional and the correct result. |
|
The only thing i can think of in term of potential problems is if perf/ebpf/crash have the correct data if used with +JPperf true. I do not think it should fail, but it is the one I am deeply interested in 😄 Thanks so much for this PR, really appreciating it! |
|
It might be incompatible with |
|
Make sense, thanks for the contribution! |
The motivation is similar to (and implementation taken partly from) NixOS#271288 --- some rebar3-compiled `.beam` files contain debug information exposing references to `.hrl` files from the Erlang distribution it was compiled with. This results in unnecessary store references. See also NixOS#423588's change to Elixir's `generic-builder.nix` where the same option is used when building to avoid the same issue.
Description of changes
This PR sets an Erlang compiler option that makes Erlang builds "deterministic." The real upshot of this is that debug information inside compiled Erlang modules no longer contains full system paths. These system paths would sometimes refer to the Erlang Nix package making them one of the trickier reasons why Erlang was being kept around as a runtime dependency for built mix releases.
The new option can be disabled with
erlangDeterministicBuilds = false.As a side effect of adding the
deterministicErlang option, I exposed an optional attr so that it is easy to specify any additional options without worrying about conflict with the deterministic option.In addition to removing those references previously located inside
beamfiles, this PR reintroduces logic previously in place to clean Erlang references from other non-beam files. As far as I understand, removing the beam references was the biggest remaining unknown (ref. https://github.com/plastic-gun/nix-mix-release-unwanted-references). Speaking of which, I would be very grateful for eyes on this from @c4710n who worked on the issue of mistaken Erlang references most recently.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Priorities
Add a 👍 reaction to pull requests you find important.