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

erlang: R22 -> R23 #109080

Merged
merged 2 commits into from Jan 22, 2021
Merged

erlang: R22 -> R23 #109080

merged 2 commits into from Jan 22, 2021

Conversation

happysalada
Copy link
Contributor

@happysalada happysalada commented Jan 12, 2021

Motivation for this change

Erlang 23 has been out for a while, I don't see any reason to hold back on using the latest version.
I only updated erlang to 23, using nixfmt, some lines were affected, I hope that's okay.

curious for feedback.

@FRidh
@alyssais
@mtanzi
@webuhu

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.

@cw789
Copy link
Contributor

cw789 commented Jan 12, 2021

I (webuhu) would encourage to upgrade Erlang default to v23.
But I'm not a good guideline.

There exists this opinion: #87738 (comment)_
So it's probably time now.

I in my opinion am always for update things fast (& let other things break ;).
But in the case if it will break a lot of packages within nixpkgs that's not the way to go...

For external apps if things are not allowed to break the should anyway use pinned nixpkgs or fixed version.
Default should be the version to check if it builds against the latest version - that's my comment.

@happysalada
Copy link
Contributor Author

@yurrriq @globin @ehamberg perhaps if you have a moment you can take a look at this? If you are not the right person, please forgive me.

@ehamberg
Copy link
Contributor

Unfortunately I'm not familiar enough with the Erlang ecosystem in Nix to be able to judge if this can be merged. I have only bumped Erlang versions within major releases. (Would love to see this go in, though!)

(Btw, if I put on my code reviewer hat, I'd prefer to have the formatting changes in a separate commit, preferably in a different pull request. Right now the formatting changes are in a commit titled “erlang: R22 -> R23” within a pull request called “erlang: R22 -> R23”. :)

@happysalada
Copy link
Contributor Author

@ehamberg thanks for the feedback. I'll make sure to separate the formatting before this goes in.

@yurrriq
Copy link
Member

yurrriq commented Jan 19, 2021

Thanks! This looks good to me at a glance. I can try to run some tests this weekend, but don't let me hold this up.

@happysalada
Copy link
Contributor Author

@yorickvP You're the last one who managed to update the major erlang version. Perhaps you have some time?

otherwise maybe @LnL7 @goertzenator @andir can help?

No worries if you are busy.

@yorickvP
Copy link
Contributor

Last time, I checked if it broke any packages and added overrides for them.

The easiest way to separate the formatting is checkout master, nixfmt, commit, copy code from this, commit again.

@happysalada
Copy link
Contributor Author

@yorickvP thanks for the quick response!
I separated the format changes in a separate commit.
The build passes when updating the major erlang version, not sure if there is anything else to do.

@happysalada
Copy link
Contributor Author

@vcunat @madjar @veprbl @Mic92
I don't think there is anything holding this PR, perhaps if you have a moment, you can help merging this?
No worries if you are busy.

Also if anybody has any idea on how to get write permission, I'm interested in trying to get it, to be a maintainer of the Erlang/Elixir eco-system.

@madjar madjar merged commit 6613bc1 into NixOS:master Jan 22, 2021
@vcunat
Copy link
Member

vcunat commented Jan 22, 2021

Getting into meta.maintainers of a package is very easy. That doesn't give you write permission (which we still can only do tree-wide, I think). Anyway, I expect that's a good start...

@vcunat
Copy link
Member

vcunat commented Jan 22, 2021

... as for the push access itself, I don't know how it is now, but perhaps this thread is a good current reference.

@happysalada
Copy link
Contributor Author

@yurrriq @vcunat @madjar Thanks a lot for your help! ❤️

@yurrriq
Copy link
Member

yurrriq commented Jan 23, 2021

Should we create a BEAM group? https://github.com/NixOS/nixpkgs/blob/master/maintainers/team-list.nix

@happysalada
Copy link
Contributor Author

@yurrriq amazing idea. I have an issue active around the elixir stuff, I'll post it there as well.
I can make the PR. Is anybody interested in joining?

@yorickvP
Copy link
Contributor

Now that this gathered everyone interested in erlang in the same place, could anyone please review #107394 ? (this pr caused it to no longer apply, as did another recent one)

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.

None yet

7 participants