Fetch submodules if supported, and warn if submodules are used but not supported #352
Conversation
|
@nmattia i am not sure where the error in the CI comes from. Looking at the source code we have a I currently don't see how i would fix that, can you give me a pointer? |
|
Ah yes! Each version of |
|
Interesting, thank you. I added it all. Let's see what the pipeline says. |
|
Looking at the changes, it seems that you may be using a different version of |
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 If repos cannot live without their submodules, then we get silent failures here, which is very bad. |
|
@nmattia it seems this time everything looks green! Thank you for your help. |
|
Is anything left to do before this can be merged? |
nix/sources.nix
Outdated
| mapAttrs | ||
| ( | ||
| name: spec: | ||
| if builtins.hasAttr "outPath" spec | ||
| then | ||
| abort | ||
| "The values in sources.json should not have an 'outPath' attribute" | ||
| else | ||
| spec // { outPath = replace name (fetch config.pkgs name spec); } | ||
| ) | ||
| config.sources; |
There was a problem hiding this comment.
Any idea why this got (re-)formatted?
There was a problem hiding this comment.
I have absolutely no clue. It seems like the nixpkgs-fmt version used in this project is relatively old
|
@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? |
|
@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. |
|
@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 |
|
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" |
|
There you go, thanks a lot! |
Referencing @awidegreen's PR #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.