nmattia / niv

Easy dependency management for Nix projects
https://github.com/nmattia/niv
MIT License
1.52k stars 74 forks source link

Fetch submodules if supported, and warn if submodules are used but not supported #352

Closed tfc closed 2 years ago

tfc commented 2 years ago

Referencing @awidegreen's PR https://github.com/nmattia/niv/pull/351, this one here does the same but also adds a warning if submodules are used but not supported by the user's nix version.

In that corner case, the user get's at least a warning trace on their shell that explains what's wrong here. Builds can still fail, but not silently without some hint on the reason.

tfc commented 2 years ago

@nmattia i am not sure where the error in the CI comes from. Looking at the source code we have a $(embedFile "nix/sources.nix") which should automatically be the right thing, right?

I currently don't see how i would fix that, can you give me a pointer?

nmattia commented 2 years ago

Ah yes! Each version of sources.nix is versioned here. If you allow me, I'll make the necessary changes and push to your branch; if on the other hand you feel like doing it yourself, it should be as simple as:

tfc commented 2 years ago

Interesting, thank you. I added it all. Let's see what the pipeline says.

nmattia commented 2 years ago

Looking at the changes, it seems that you may be using a different version of nixpkgs-fmt than the CI. Did you run ./scripts/fmt or did you use a version of nixpkgs-fmt installed on your system?

SuperSandro2000 commented 2 years ago

if submodules are used but not supported by the user's nix version.

I don't think it should warn there. Often submodules are vendored projects which can be replaced by system libraries.

tfc commented 2 years ago

I don't think it should warn there. Often submodules are vendored projects which can be replaced by system libraries.

I am sure such repos exist. In such cases you are still free to set submodules = false and use it as you describe.

If repos cannot live without their submodules, then we get silent failures here, which is very bad.

tfc commented 2 years ago

@nmattia it seems this time everything looks green! Thank you for your help.

tfc commented 2 years ago

Is anything left to do before this can be merged?

tfc commented 2 years ago

@nmattia ok, does the weird formatting block the pull request now, so do i need to analyze why the tool decided to format this unrelated block of code, or is it ok?

tfc commented 2 years ago

@nmattia i am happy to do anything that's necessary to make this mergeable. Right now i don't know what's missing and i also don't know if there is anything else that you need (time to review because you are busy right now (in that case shall we add any other reviewers? do we need more testing? or anything else having to be merged before this or something?), so please let me know.

nmattia commented 2 years ago

@tfc my bad, I was out for a few days! Should have been clearer: I'm confused as to why the formatting was changed around line 170, but it looks good to me nonetheless! For me you can merge at will :) You could revert the formatting on line 170 (maybe it was done by your editor? either way nixpkgs-fmt doesn't seem to mind, but probably cleaner to remove it so that future git blamers don't get confused) but you could also just merge as is. Your call, and thanks a lot for the contribution!

tfc commented 2 years ago

Ok. So the additional format change was my mistake, sorry for the resulting noise. I removed it.

Btw. once the tests are confirmed green: i have no merge rights so i can't follow the "you could also just merge"

nmattia commented 2 years ago

There you go, thanks a lot!