nix-community / crate2nix

rebuild only changed crates in CI with crate2nix and nix
https://nix-community.github.io/crate2nix/
Apache License 2.0
354 stars 83 forks source link

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

Closed marius851000 closed 10 months ago

marius851000 commented 2 years ago

What I did:

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 commented 2 years ago

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 commented 2 years ago

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)

Ericson2314 commented 2 years ago

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?

marius851000 commented 2 years ago

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 commented 2 years ago

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)

Ericson2314 commented 2 years ago

@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).

marius851000 commented 2 years ago

@Ericson2314 That’s done!

Ericson2314 commented 2 years ago

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 :)

marius851000 commented 2 years ago

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.

Ericson2314 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

https://github.com/NixOS/nix/pull/7260 this should help a bit with the original issue :)

marius851000 commented 1 year ago

I’m sorry, but I have other thing I would like to work right now. I’ll probably return to this hopefully in less than a few weeks.

Ericson2314 commented 1 year ago

All good, no rush, just wanted to make sure github knew these things were related.

kolloch commented 10 months ago

Sorry for not reacting earlier but I guess, I have waited long enough, right?

We can close this now or is there something left to do?