nix-community / napalm

Support for building npm packages in Nix and lightweight npm registry [maintainer=?]
MIT License
104 stars 17 forks source link

General Napalm improvements #38

Closed Mazurel closed 2 years ago

Mazurel commented 3 years ago

This PR contains changes made in #37

Short description

This PR contains some improvements that may be helpful for some projects. I have made them, because I needed them myself and decided to try to upstream them. I have tried to keep backwards compatibility as much as I could. From my tests, buildPackage should work exactly the same as it worked before this PR (or better).

Issues that are related or fixed by this PR

Changes

jtojnar commented 3 years ago

Thanks for the contribution, there are many useful fixes but now it is even harder to review than the previous one.

For example, just the first commit:

  • Added development shell

Looks good.

  • Improved overlay for other flakes to use

As far as I can tell, it just makes the whole default.nix available instead of just the napalm helper functions? Why would we want to pollute the namespace in the overlay with stuff that is mostly just useful for testing napalm by CI?

  • Added packages that are part of default.nix to the packages field

Again, with the exception of napalm-registry, these are unrelated to napalm’s purpose and people should not use them.

  • Removed checks, as nix flake check automatically checks packages

Makes sense if we use the previous one.

Just having the four concerns in the single commit made it harder to review, making me unsure if the second part did anything other than what I noticed.

Mazurel commented 3 years ago

Sorry if the commits are confusing, I tried my best to split it up as well as I could. I have made a small rebase, with intention to make things more clear.

As far as I can tell, it just makes the whole default.nix available instead of just the napalm helper functions? Why would we want to pollute the namespace in the overlay with stuff that is mostly just useful for testing napalm by CI? Again, with the exception of napalm-registry, these are unrelated to napalm’s purpose and people should not use them.

I put everything into overlay, because then I wanted to use it in packages field. I know that most people are not likely to use anything other than buildPackage, but I though that it doesn't really matter as Nix is lazy evaluated, and it is not likely to affect anything. If it bothers you, I can remove it from the overlay and move it into the checks field.

Mic92 commented 3 years ago

@jtojnar have your comments been addressed?

Mazurel commented 2 years ago

Also, can you run nixpkgs-fmt on all of it?

I did run it on flake.nix, although I am not sure about using it on default.nix as it wasn't used before on this file. I didn't want to change most of the file with formatting.

I have also made flake.nix use flake-utils as it makes it more compact in my opinion.

Mic92 commented 2 years ago

@gytis-ivaskevicius when you give me green light for the changes here I can merge it. I currently don't have the capacity to review it myself.

Mazurel commented 2 years ago

@gytis-ivaskevicius I have applied formatting and fixed ternary operator formatting.

Also, I don't think that you can overwrite nodejs version without overlays?

I am not sure what do you mean to be honest, but I will try to explain what do I mean by "custom node.js version". I have made it possible to pass custom NodeJS package like pkgs.nodejs-14_x to the buildPackage function and then whenever NodeJS may be used, this version is used. There is an example of that in the README.md (rendered reference). This way, you can also override some NodeJS major version and downgrade it to a specific minor version that you may need, there is an example of that in that README.

gytis-ivaskevicius commented 2 years ago

Did a couple of tweaks, from my point of view this is ready to be merged. @Mazurel Are you fine with these last 3 commits? Do you think this PR is ready to be merged?

Mazurel commented 2 years ago

@gytis-ivaskevicius They look fine to me, the only thing that could be implemented are some tests for the new features, but I think they could be added in another PR (when nix flake check will work).

gytis-ivaskevicius commented 2 years ago

@Mic92 This PR is ready to merge :)