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

rebar3: 3.6.1 -> 3.9.0 #54115

Merged
merged 1 commit into from
Feb 9, 2019
Merged

rebar3: 3.6.1 -> 3.9.0 #54115

merged 1 commit into from
Feb 9, 2019

Conversation

k32
Copy link
Contributor

@k32 k32 commented Jan 16, 2019

Motivation for this change
  • Update package
  • Simplify build
Things done
  • Updated rebar3 package

  • Removed incompatible hermetic patch (nix-build runs with network isolation, hence the hermetic patch is not needed anymore)

  • Started eliminating the need of hex pac

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)

  • 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 nox --run "nox-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)

  • Assured whether relevant documentation is up to date

  • Fits CONTRIBUTING.md.


@Mic92
Copy link
Member

Mic92 commented Jan 16, 2019

cc @sjmackenzie for review

@k32
Copy link
Contributor Author

k32 commented Jan 23, 2019

Anyone? @gleber @couchemar @sjmackenzie @the-kenny

@Mic92
Copy link
Member

Mic92 commented Jan 23, 2019

@GrahamcOfBorg build hex2nix relxExe

@Mic92
Copy link
Member

Mic92 commented Jan 23, 2019

Are hex2nix and relxExe known to fail?

@k32
Copy link
Contributor Author

k32 commented Jan 23, 2019

Could be something related to the hermetic patch. I'll take a look later.

@k32
Copy link
Contributor Author

k32 commented Jan 24, 2019

Ok, these errors were caused by not updating hex cache, which is completely useless when building with nix-build. My workaround with unpacking contents of hex packages to _checkouts directory prior to running rebar3 seems to work just fine, so I think I'll proceed with changing the rest of the packages to use the same trick. ETA: weekend

@k32
Copy link
Contributor Author

k32 commented Feb 3, 2019

Unfortunately hashes used by hex are different from what could be used by nix. They concatenate the source tarball together with some metadata, which is unknown in advance [1]. It is still possible to use hex-style hash in a fixed-output derivation, but via a hack so dirty I don't even want to talk about. So I had to resort to fixed-output derivations containing downloaded sources. Similar approach is used by Rust people, however this practice is disputed [2]

[1] https://github.com/hexpm/hex_core/blob/bfd012f7200091cac2901850b2f7a828f7bf6301/src/hex_tarball.erl#L244
[2] NixOS/nix#2270

@k32
Copy link
Contributor Author

k32 commented Feb 3, 2019

@GrahamcOfBorg build hex2nix relxExe

@k32
Copy link
Contributor Author

k32 commented Feb 4, 2019

@Mic92 Sorry for the delay. Both hex2nix and relxExe build now and nox doesn't report any errors.

hermeticRebar3 = true;
};
rebar3-open = callPackage ../tools/build-managers/rebar3 { };
rebar3 = callPackage ../tools/build-managers/rebar3 { };
Copy link
Contributor

Choose a reason for hiding this comment

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

These need the hermeticRebar3 option. It defaults to true, which means rebar3-open and rebar3 are now the same things. And users of rebar3-open will be unable to download packages from hex because it is hermetic.

Copy link
Contributor

Choose a reason for hiding this comment

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

After going through it a little further, I see that you removed the hermetic patch, which means (I think) it now goes against how Nix is expecting things. It is free to download anything that it wants instead of you explicitly saying what the applications dependencies are. I would like someone with more knowledge than myself to say whether this is acceptable or not.

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,

In short, removal of the hermetic patch isn't against Nix's way of doing things: nix-build runs with network isolation (unless it's a fixed-output derivation) therefore rebar3 won't be able to download anything during the process of build.

Fetching dependencies is a different story. It was moved to a separate step. Using rebar3 get-deps produces fixed output (rebar3 itself uses sha256 to freeze the dependencies), so this is no worse than using fetchurl.

I described my reasoning more in depth here: #53834


downloadPhase = ''
cp ${src} .
HOME='.' DEBUG=1 ${rebar3}/bin/rebar3 get-deps
Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth, this will only work on rebar3-open because it is not hermetic. How does this really differ from the current fetchHex (aside from it possibly getting non-hex deps)?

Copy link
Contributor Author

@k32 k32 Feb 6, 2019

Choose a reason for hiding this comment

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

The current way of dealing erlang dependencies is not viable. Not only hermetic patch breaks rebar3 for everyone, but it also heavily relies on undocumented rebar3 features and file locations.

With the current approach in order to update a package one needs to

  1. Ask @gleber to update hex index cache repo (note that hex index is not actually needed. it's only used to avert rebar3 errors)
  2. Wait until index hash repo is merged and the derivation is updated in nixpkgs repo
  3. Ping someone to run hex2nix and update hex packages in this repo.
  4. Wait again until the new PR is reviewed and merged
  5. Manually convert lock file contents to the Nix expression
  6. Pray that nothing doesn't have non-trivial build steps
  7. Pray especially hard that no one uses rebar3 override feature

With the new approach every package can be maintained completely independently.

@ankhers
Copy link
Contributor

ankhers commented Feb 6, 2019

Since this is still open, would you mind bumping to 3.9.0?

Remove hermetic patch (make it compatible with the upstream)
(Mostly) eliminate the need for hex package registry
@k32 k32 changed the title rebar3: 3.6.1 -> 3.8.0 rebar3: 3.6.1 -> 3.9.0 Feb 6, 2019
@k32
Copy link
Contributor Author

k32 commented Feb 6, 2019

@ankhers Done, also squashed commits to reduce noise

@ankhers
Copy link
Contributor

ankhers commented Feb 6, 2019

I'm good with this. My only other suggestion would be to drop rebar3-open since it is effectively useless. But we can do that in another PR if preferred.

@k32
Copy link
Contributor Author

k32 commented Feb 6, 2019

Someone may use it. Is there a good way to deprecate a package?

@ankhers
Copy link
Contributor

ankhers commented Feb 6, 2019

Based on a conversation in IRC, there doesn't seem to be a way to deprecate things. We may just need to change some docs the refer to it. Lets keep it for now and we can clear it out in a different PR.

@k32
Copy link
Contributor Author

k32 commented Feb 6, 2019

If possible, I would like to update docs through a separate PR after the rest of planned changes are proven viable and implemented. After that both old hex2nix packages and rebar3-open can be nuked.

@k32
Copy link
Contributor Author

k32 commented Feb 8, 2019

@ankhers May I have this one approved so I can move on to the next step?

Copy link
Contributor

@ankhers ankhers left a comment

Choose a reason for hiding this comment

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

This should be good.

@ankhers
Copy link
Contributor

ankhers commented Feb 9, 2019

I can't actually merge. Will need to wait for someone else to do that.

@infinisil infinisil merged commit 18d059a into NixOS:master Feb 9, 2019
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/state-of-the-beam-ecosystem-in-nix/4202/2

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/state-of-the-beam-ecosystem-in-nix/4202/2

1 similar comment
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/state-of-the-beam-ecosystem-in-nix/4202/2

@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
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.

7 participants