Allow patching dependency sources #151

infinisil · 2022-03-15T02:45:56Z

Adds support for patching dependency sources with patches and postPatch derivation attributes. This is a bit hacky, as patching any files requires recalculating the integrity value, which needs to be done at build-time. But it works, and I made sure to make the code resilient and commented.

This is a solution for #110, as this allows doing patchShebangs on individual packages. npmlock2nix could also provide a predefined list of patches for individual packages for convenience in the future.

andir

Just a few comments that I had while going through the first couple of changes.

andir · 2022-03-15T09:57:15Z

internal.nix

@@ -76,20 +76,42 @@ rec {
            inherit rev ref;
            allRefs = true;
          };
+
+      # If sourceMods.${name} is set, call it with some information about this


Can we give strong gurantees what "some information" are going to be? It would be good to pick something that we can document somewhere. Having to guess or debug your way there wouldn't be the best experience.

In the API.md I at documented it as only a version attribute for now, leaving the possibility for more in the future open. The version however is a bit ambiguous, because for github: dependencies it could be a git hash or any other git reference. Maybe it would be better to let users distinguish between github: and url sources, e.g. by setting sourceType = "github" / "url"

I changed this a bit, now these attributes are passed:

version for HTTP tarball sources

github = { org, repo, rev, ref } for GitHub sources

Considering that sources for a specific package should typically all be of the same source type, this should also work fairly well.

I also thought a lot about providing a proper version in the GitHub case:

It's possible to get the version at build time via the .version field in the package.json file. This would be a uniform way of getting a correct version

However we lose the ability to decide on which patches to include at eval time

In order to still be able to limit patches to specific versions, it would be best to have some way of specifying NPM version ranges for each patch and calling runtime checks to figure out whether to include them.

I deemed this approach too complicated though, because it would have to involve coming up with our own custom scheme of declaring patches and patch commands, entirely separately from the current nixpkgs way.

The version and github based version decisions should be good enough for most use cases for now.

andir · 2022-03-15T09:58:58Z

internal.nix

+
+      # If sourceMods.${name} is set, call it with some information about this
+      # source for it to make a decision about when to do modifications
+      mods = lib.optionalAttrs (sourceOptions ? sourceMods.${name})


Here you call it mods (sourceMods) but it actually is extra attributes (attrs) in the packTgz invocation. Can we agree on one way to call these?

I called it sourceAttrs now, also relating to the thoughts in #151 (comment)

andir · 2022-03-15T10:04:06Z

internal.nix

+    inherit src;
+    nativeBuildInputs = attrs.nativeBuildInputs or [ ] ++ [
+      # Allows patchShebangs in postPatch to patch shebangs to nodejs
+      nodejs


This might not be required if patchShebangs isn't called at all.

It's a bit annoying to include the correct version of nodejs, and considering that it's always a dependency anyways, and that many scripts would use node as an executable, I think it makes sense to provide this as a default

andir · 2022-03-15T13:56:03Z

API.md

+
+    # Patching shebangs
+    node-pre-gyp = _: {
+      postPatch = "patchShebangs bin";


The interface here is a very leaky abstraction. We just plug some additional variables into the runCommand but only some will ever actually have an impact. I'd rather prefer an interface that isn't passing everything through from the user but defines a stricter interface.

The alternative is to provide an overrideAttrs interface here instead.

I prefer the first approach as it comes with a more limited set of features that we have to take care of.

Sounds good. I think we can limit this to only patches and postPatch for now, but it could be extended with support for other attributes in the future (for now other attributes should give an error)

After another thought, I think it's probably better to just support all attributes, both to keep it simpler, but also because people might have different needs. Notably there's also prePatch, you can add dependencies in buildInputs as needed by patchShebangs, you may want to set NIX_DEBUG for debugging, you may want to set sourceRoot to only get a subdirectory, you may want to set dontUnpack to preserve the original .tgz and patch that with tar directly, you may want to set environment variables.

I pushed support for this in this PR now, along with documentation for it

nixos-discourse · 2022-03-18T14:35:24Z

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

https://discourse.nixos.org/t/tweag-nix-dev-update-26/18252/1

andir · 2022-03-31T14:29:27Z

API.md

@@ -114,3 +115,32 @@ npmlock2nix.build {
  };
 }
 ```
+
+### Source derivation attributes


This addition seems sound. I just worry a bit about supporting this as an interface. Users should/will experience breakage over time as this is a very broad interface that we can't by no means keep stable/guarantee.

See #151 (comment), do you think it would be fine with to have it be like this considering the arguments there?

I get the appeal and I can see why it might be used. I just don't want to advertise it as the go-to solution. We should do most if not all things for the users instead of exposing these broad interfaces...

I'll give it a bit of though.

andir · 2022-03-31T14:32:35Z

internal.nix

+        # of entries, where each entry contains a JSON object path of the
+        # integrity field and the corresponding resolved file path, separated
+        # by a tab, ready for shell consumption
+        jqFindNullIntegrity = ''


Why do we have to do that during build time? Can't we have nix do this for us? I'd rather not depend on yet another DSL for this. Especially since I am not using jq very often and at some point changes to this will have to be made.

Ah that's a good point, this part could be done by Nix at eval time, I'll give that a go

Pushed a commit to do this with Nix instead. Also rebased this PR on top of master.

This allows more flexibility for the future, such as support for: - Source patches - Fetch authentication

Can be used to patchShebangs for specific packages, which with NPM version 7.0 can't be done globally for all packages anymore Could also be a replacement for preInstallLinks

Instead of jq

andir · 2022-04-06T11:16:14Z

Thank you for the updates you provided. I'll try to get through them some afternoon this week.

andir · 2022-04-12T18:54:18Z

API.md

+    # sourceInfo either contains:
+    # - A version attribute
+    # - A github = { org, repo, rev, ref } attribute for GitHub sources
+    package-name = sourceInfo: {


The more I think about this the interface should be:

sourceOverrides = { package-name = sourceInfo: drv: drv.overrideAttrs (_:{ }); }

As that is the standard way of doing overrides within nixpkgs. This also doesn't expose some half-baked custom solution but rather whatever the typical nix developer is already familiar with. IIRC poetry2nix does it very similar.

andir · 2022-04-12T18:59:12Z

Besides the override interface this looks fine and I'd accept it. @infinisil can you make that change?

infinisil · 2022-04-21T10:43:19Z

@andir Changed the interface according to your suggestion.

While I think there's some redundancy to it (because it's probably always going to be a .overrideAttrs, and the old argument isn't really needed), I think the benefit of familiarity is beneficial enough. Also the implementation becomes a bit nicer.

infinisil · 2022-05-16T19:12:20Z

(ping, does this look good?)

andir reviewed Mar 15, 2022

View reviewed changes

infinisil force-pushed the source-options branch 2 times, most recently from 4b3a198 to a236825 Compare March 17, 2022 16:28

This was referenced Mar 18, 2022

Introduce preInstallCustomCommands attribute for node modules #71

Open

fix: patchShebangs in nonexecutable bin files (#106) #107

Open

Support nodejs >= 15, npm >= 7, lockfile version >= 2 #153

Open

andir reviewed Mar 31, 2022

View reviewed changes

infinisil added 4 commits March 31, 2022 18:51

Remove unused sourceHashFunc arg from patchPackagefile

58ed1f4

Convert internal sourceHashFunc argument to more general sourceOptions

9551a85

This allows more flexibility for the future, such as support for: - Source patches - Fetch authentication

Add support for patching dependencies

a74e464

Can be used to patchShebangs for specific packages, which with NPM version 7.0 can't be done globally for all packages anymore Could also be a replacement for preInstallLinks

Collect necessary integrity changes with Nix

bbb6fd4

Instead of jq

infinisil force-pushed the source-options branch from 1a45e5b to bbb6fd4 Compare March 31, 2022 16:52

infinisil mentioned this pull request Apr 6, 2022

Node shebang remains unpatched for some binaries #155

Open

andir reviewed Apr 12, 2022

View reviewed changes

Change interface from sourceAttrs to sourceOverrides

60e0c0e

infinisil mentioned this pull request Apr 26, 2022

Authentication support #160

Closed

9 tasks

andir approved these changes Jun 9, 2022

View reviewed changes

andir merged commit 5c4f247 into nix-community:master Jun 9, 2022

infinisil deleted the source-options branch October 3, 2022 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow patching dependency sources #151

Allow patching dependency sources #151

infinisil commented Mar 15, 2022

andir left a comment

andir Mar 15, 2022

infinisil Mar 15, 2022

infinisil Mar 17, 2022

andir Mar 15, 2022

infinisil Mar 17, 2022

andir Mar 15, 2022

infinisil Mar 17, 2022

andir Mar 15, 2022

infinisil Mar 15, 2022

infinisil Mar 17, 2022 •

edited

Loading

nixos-discourse commented Mar 18, 2022

andir Mar 31, 2022

infinisil Mar 31, 2022 •

edited

Loading

andir Mar 31, 2022

andir Mar 31, 2022

infinisil Mar 31, 2022

infinisil Mar 31, 2022

andir commented Apr 6, 2022

andir Apr 12, 2022

andir commented Apr 12, 2022

infinisil commented Apr 21, 2022

infinisil commented May 16, 2022

Allow patching dependency sources #151

Allow patching dependency sources #151

Conversation

infinisil commented Mar 15, 2022

andir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil Mar 17, 2022 • edited Loading

Choose a reason for hiding this comment

nixos-discourse commented Mar 18, 2022

Choose a reason for hiding this comment

infinisil Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andir commented Apr 6, 2022

Choose a reason for hiding this comment

andir commented Apr 12, 2022

infinisil commented Apr 21, 2022

infinisil commented May 16, 2022

infinisil Mar 17, 2022 •

edited

Loading

infinisil Mar 31, 2022 •

edited

Loading