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

beam-packages: init elixir_ls 0.7.0 #118950

Merged
merged 1 commit into from Apr 19, 2021
Merged

Conversation

happysalada
Copy link
Contributor

@happysalada happysalada commented Apr 10, 2021

Motivation for this change

add elixir_ls to nixpkgs

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.

additional context

This is pretty much copyPasta from https://github.com/hauleth/nix-elixir/blob/master/lib/elixir-ls.nix

@minijackson @hauleth @NobbZ @afontaine @balsoft @yurrriq @cw789 you might be interested in this

Copy link
Member

@balsoft balsoft left a comment

Choose a reason for hiding this comment

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

Since this is just copying someone else's code, wouldn't it need a copyright notice as required by the license?

@minijackson
Copy link
Member

Damn, and here I wanted to be the first one to package something with the new mixRelease ^^

The project builds, and runs fine. Tested with NixOS and LanguageClient-neovim. I haven't tested the debugger, since I don't have a VS code installation.

One weird thing is that nix-review doesn't detect a change in packages, and so builds nothing. This might have to do with beamPackages not being recognized as a "Package Set" (example).

Since this is just copying someone else's code, wouldn't it need a copyright notice as required by the license?

Another possibility would be to ask the original developer for his explicit permission. I also think a mention of its origins in a comment would be nice. I don't think recall having a precedent in nixpkgs where the derivation code mentions its previous license.

Now, I am not a lawyer, but Nixpkgs is also licensed under MIT, with a copyright to edolstra and Nixpkgs/NixOS contributors, and it seems @hauleth has contributed to Nixpkgs, so the Nixpkgs license should be a superset of the one from @hauleth's repository? But I think it's still better to have his explicit permission.

@happysalada
Copy link
Contributor Author

more than happy to include anything @hauleth judges appropriate.
Everytime I interacted with him though, he has been forward about not caring about attribution.
Since he might not be available, perhaps I can add a TODO comment about attribution and whenever he comes forward we can rectify that? Just a thought to get this merged to provide value to other people faster.

@minijackson This one doesn't count, it's just a copy paste. You can still be the first one that packages something original! :-)

Also regarding the nixpkgs-review I've added elixir_ls to all_packages, it looks like it's working

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Apr 10, 2021

One weird thing is that nix-review doesn't detect a change in packages, and so builds nothing. This might have to do with beamPackages not being recognized as a "Package Set" (example).

That needs fixing rather sooner than later because ofborg does not detect eval errors in this case.

Now, I am not a lawyer, but Nixpkgs is also licensed under MIT, with a copyright to edolstra and Nixpkgs/NixOS contributors, and it seems @hauleth has contributed to Nixpkgs, so the Nixpkgs license should be a superset of the one from @hauleth's repository? But I think it's still better to have his explicit permission.

Since he might not be available, perhaps I can add a TODO comment about attribution and whenever he comes forward we can rectify that?

Adding him to the commit as an author is enough. His code is also licensed under MIT.

@@ -11586,7 +11586,8 @@ in
inherit (beam.interpreters)
erlang erlangR23 erlangR22 erlangR21 erlangR20 erlangR19 erlangR18
erlang_odbc erlang_javac erlang_odbc_javac erlang_basho_R16B02
elixir elixir_1_11 elixir_1_10 elixir_1_9 elixir_1_8 elixir_1_7;
elixir elixir_1_11 elixir_1_10 elixir_1_9 elixir_1_8 elixir_1_7
elixir_ls;
Copy link
Member

Choose a reason for hiding this comment

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

This should be done automatically for all packages because we don't want to maintain a list of packages 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.

I've added the suggested fix. Since it was adding some other packages on top, I've removed those.
Waiting for the build to check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran into an interesting problem here.
You can recursively add packages, that was not a problem.
The solution looked something like

beam = (let
   packages = callPackage ./beam-packages.nix { };
   interpreters = packages.interpreters;
in 
   packages // removeAttrs interpreters [ "lfe" "lfe_1_2" "lfe_1_3" ]);

However you still need to inherit those. I haven't found a way to recursively inherit something.
So unless I am missing something, we need to keep the current syntax

Copy link
Member

Choose a reason for hiding this comment

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

I think you need to use passthru in the packages for those attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have opened a separate issue for this
#119201
just so that it doesn't block this PR.

pkgs/top-level/all-packages.nix Show resolved Hide resolved
pkgs/development/beam-modules/elixir_ls.nix Outdated Show resolved Hide resolved
pkgs/development/beam-modules/elixir_ls.nix Outdated Show resolved Hide resolved
pkgs/development/beam-modules/elixir_ls.nix Outdated Show resolved Hide resolved
pkgs/development/beam-modules/elixir_ls.nix Show resolved Hide resolved
@happysalada
Copy link
Contributor Author

Result of nixpkgs-review pr 118950 run on x86_64-darwin 1

1 package built:
  • elixir_ls

@happysalada happysalada force-pushed the add_elixir_ls branch 3 times, most recently from 91daa57 to 277a21d Compare April 10, 2021 15:09
@afontaine
Copy link
Contributor

Result of nixpkgs-review pr 118950 run on x86_64-linux 1

1 package built:
  • elixir_ls

@happysalada happysalada force-pushed the add_elixir_ls branch 3 times, most recently from 22e903e to 306f325 Compare April 11, 2021 06:45
@happysalada
Copy link
Contributor Author

Regarding contribution, if I added a message in the commit saying something like

taken from code written by github.com/hauleth

Would that work for you @balsoft ?
Or perhaps including the full source of the code?

@balsoft
Copy link
Member

balsoft commented Apr 11, 2021

It's not up to me to decide, ask @hauleth .

@happysalada
Copy link
Contributor Author

sure, I would say let's leave it like this for a week and we can try to think of an intermediate solution if there is no feedback.

@hauleth
Copy link
Contributor

hauleth commented Apr 11, 2021

@balsoft @happysalada You can just add comment on top

Based on work of Hauleth

And it will be enough for me

@happysalada
Copy link
Contributor Author

@hauleth thanks for your feedback! Btw I'm going to leave the erlang_ls, if you are interested in adding it, I'm sure @balsoft would be happy to help you merge it.
I've added the attribution, plus a little sentence of mine :-)

@balsoft let me know if you feels it needs anything else.

@happysalada
Copy link
Contributor Author

I'm planning on leaving this open until next monday 19th of April.
Please feel free to comment on anything you think I overlooked.

@Br1ght0ne
Copy link
Member

Thanks a lot @happysalada, @hauleth and everyone else - I tried to add this a long time ago, and it's a pleasure to see a good PR. Waiting for April 19th

@happysalada happysalada merged commit aa41080 into NixOS:master Apr 19, 2021
@happysalada happysalada deleted the add_elixir_ls branch April 28, 2023 20:01
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