tomlDependencies: pass allRefs=true when fetching a rev
#211
Conversation
|
sending this PR in case it is useful. no worries about closing if not 🙂 |
There was a problem hiding this comment.
Sounds sensible, but I'll let @Anderssorby and @blitz have the final word!
There was a problem hiding this comment.
Looks good to me, especially if it fixes pain people actually experience.
Not merging right now, because I can't test it at the moment. (Writing this from my phone.) I'll get back to this in a couple of days or maybe in the meantime @Anderssorby can take a look.
ah, neither did I! I am not sure how to do version-conditional code in Nix but I will check into it next week when I get back from the trip I'm on 👍 |
|
Hm, this may cause unnecessary data to be downloaded which is not desirable. I remember in another project we solved this by specifying the branch as well in the Cargo.toml |
|
perhaps the best way would be to add an optional attr my (not very informed) sense is that it'll be tough to do a kind of "granular enabling of allRefs=true", so maybe just a global (to the Cargo project which is pointed to by the reasoning being that many projects won't have this issue and won't notice the change, but for projects that do, downloading some extra data is preferable to not being able to build 🙂 oh, and then sorting out the pre-Nix-2.4 compatibility issue mentioned above. |
|
The latest stable release bumped the nix version again, if you push to the branch, it should re-trigger CI to run. Which will probably pass now. |
|
21.05 is EOL, may just ask people to pin to a commit before this change if they want to be compatible with nix 2.3 |
this allows us to avoid issues when the default branch is not named `master`. I believe this should fix the issues described in: nix-community#149 nix-community#140 using this change seems to fix this error I was seeing: ``` fetching Git repository 'https://github.com/sacredcapital/rep_lang.git'fatal: couldn't find remote ref refs/heads/master error: program 'git' failed with exit code 128 ``` and enables a successful build. I reproduced the successful build on multiple machines. however I cannot be full certain that it works in all cases. additionally, it seems possible that `allRefs = true` causes annoying extra fetching which might be noticeable for repos with many refs...
@jonringer thx. I just rebased and force pushed the branch. however I see:
so perhaps the check will not run. |
It's not that easy. Nix 2.3 is for some reason still the default on NixOS 21.11. As long as Nix 2.3 is actively shipped, it's a bit user hostile to require 2.4 by default. We could check |
|
Hi! Thanks for this merge request - for the time being, I'm going to go with #167, which allows for users to configure whether they want |
|
@Patryk27 good call, thx and yw! |
this allows us to avoid issues when the default branch is not named
master.I believe this should fix the issues described in:
#149
#140
using this change seems to fix this error I was seeing:
on a repo which had only a
mainbranch, and nomasterbranch. of note, I was able to work around the issue by creating a dummymasterbranch, but still using the rev on themainbranch.using this PR seems to enable a successful build, without requiring the dummy
masterbranch which was not actually used.I reproduced the successful build on multiple machines. however I cannot
be full certain that it works in all cases.
additionally, it seems possible that
allRefs = truecauses annoyingextra fetching which might be noticeable for repos with many refs...