crate2nix icon indicating copy to clipboard operation
crate2nix copied to clipboard

Enhance IFD in tools.nix (+workaround for a recent Nix issue)

Open marius851000 opened this issue 1 year ago • 12 comments

What I did:

  • It seems that Nix prior to 2.5 (instead of 2.3) can’t fetch submodules with builtins.fetchGit. (from my own experience), which caused the tests to fail for the new check I add (I later added a crate-hashes.json).
  • Added workarounds for https://github.com/NixOS/nix/issues/6647 when I failed to run tests with the Nix version installed on my computer (which is recent enought to have this issue). The workaround is presented here : https://github.com/NixOS/nixpkgs/pull/182980/files
  • It doesn't copy the source of the input crates in tools.nix if it’s a folder anymore (and instead link). This help to make it greatly faster when I was working on the OS installed on my external HDD, going from 5 minutes to 10 seconds when working on Veloren.
  • The first issue I encountered is that the some repos where added in the vendor config. I also fixed fetching if a tag is specified.
  • A fix for https://github.com/kolloch/crate2nix/issues/207 (mostly from https://github.com/kolloch/crate2nix/pull/166).

With all this (and a bit of overriding), Veloren can be compiled with nix again.

(I also found an issue with the default libgit2-sys that cause issue when updating cargo-release in the niv lock. I openend a PR against nixpkgs to fix it https://github.com/NixOS/nixpkgs/pull/187591)

marius851000 avatar Aug 20 '22 15:08 marius851000

I’ll comment that the unsafeDiscardStringContext scare me a bit, and that I would like to take more time to understand it before this would be ready to merge due to the work around not working with flakes in pure mode

marius851000 avatar Aug 20 '22 20:08 marius851000

Fixed this strange stuff that scared, which was in fact caused by an implementation detail in nixpkgs (to see if a path is a directory, it list child of the parent dir, This resulted in listing /nix/store, which is impure. Instead decide on what to do with src at run-time)

marius851000 avatar Aug 20 '22 23:08 marius851000

I am afraid I don't feel very comfortable reviewing changes to tools.nix yet. When I tried to use myself, I got error from crate2nix calling nix inside the derivation --- which I doubt is what is supposed to happen.

@kolloch, are you around to review this?

Ericson2314 avatar Aug 21 '22 01:08 Ericson2314

I figured that there are no documentation on using this tools.nix. I might work on it. (and I checked that the use of unsafeDiscardStringContext is safe, as the only reference it should remove is the one to the lockfile and the crate-hashes.json, which isn’t needed as all the data relevant is in the string anyway. Thought it lacks documentation too.)

marius851000 avatar Aug 21 '22 08:08 marius851000

Also, it seems that generatedCargoNix is made for when src is a folder, and that it actually never worked when it was a package. I’ll just remove all the handling in this case, and assume that src is a folder.

(Or if we want to allow non-folder, using an additionnal derivation on top that unpack the source if it is a file or link it otherwise likely already exist in nixpkg)

marius851000 avatar Aug 21 '22 09:08 marius851000

@marius851000 Yes if you were to take on the docs, https://github.com/kolloch/crate2nix/issues/110, prior to this PR, I would then feel comfortable reviewing this (which would modify the code and the new docs in tandem).

Ericson2314 avatar Aug 21 '22 13:08 Ericson2314

@Ericson2314 That’s done!

marius851000 avatar Aug 26 '22 20:08 marius851000

So I've since figured out how to use the IFD stuff, and I think I feel more comfortable reviewing this.

Do you mean making the docs a separate PR? I think we can do something a bit different to avoid unsafeDiscardStringContext :)

Ericson2314 avatar Aug 27 '22 20:08 Ericson2314

Well... I understood "I would then feel comfortable reviewing this (which would modify the code and the new docs in tandem)." as putting the docs in this PR. But I could do a separate PR if needed. And I’m indeed interested in what you have to avoid unsafeDiscardStringContext.

marius851000 avatar Aug 31 '22 09:08 marius851000

@marius851000 sorry I was indeed not clear. I had meant this:

  1. Document status quo
  2. Change code (with unsafeDiscardStringContext or something else) and docs (if need be)

Ericson2314 avatar Sep 09 '22 15:09 Ericson2314

I think unsafeDiscardStringContext is fine for now, but the longer term solution is to consider why the json file is an input to the IFD thing rather than in nix (not json file) to begin with.

Ericson2314 avatar Sep 09 '22 15:09 Ericson2314

The improve redo fetching would also be good to split out, so I think this should actually be 3 parts:

  1. Document status quo
  2. Change code for new Nix IFD (with unsafeDiscardStringContext or something else) and docs (if need be)
  3. Change code for #207

The #207 fix I am also hesitant on as I think builtins.fromToml is out of step with the rest of the project.

Ericson2314 avatar Sep 09 '22 23:09 Ericson2314