Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fetchFromGitHub: add fetchSubmodules option #20518

Merged
merged 1 commit into from
Nov 18, 2016

Conversation

rasendubi
Copy link
Member

@rasendubi rasendubi commented Nov 18, 2016

Motivation for this change
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

This commit extends fetchFromGitHub with ability to fetch GitHub
repositories with submodules, so we can use the function consistently
with all GitHub repositories.

Note it doesn't change the previous behavior.

@mention-bot
Copy link

@rasendubi, thanks for your PR! By analyzing the history of the files in this pull request, we identified @MarcWeber, @shlevy and @rbvermaa to be potential reviewers.

This commit extends fetchFromGitHub with ability to fetch GitHub
repositories with submodules, so we can use the function consistently
with all GitHub repositories.

Note it doesn't change the previous behavior.
@rasendubi rasendubi force-pushed the fetchFromGitHub branch 2 times, most recently from ee67b3b to a80cacd Compare November 18, 2016 11:07
@rasendubi
Copy link
Member Author

rasen@ashmalko ~/nixpkgs> nix-shell -p nox --run 'nox-review -k pr 20518'
=== Reviewing PR 20518 : fetchFromGitHub: add fetchSubmodules option
==> We're in a git repo, trying to fetch it
==> Fetching base (master)
==> Fetching PR
==> Fetching extra history for merging
==> Merging PR into base
... md5 warnings
Nothing changed

@rasendubi
Copy link
Member Author

@Profpatsch, you might like it ;)

@copumpkin
Copy link
Member

Awesome! That fixes #15559

@copumpkin
Copy link
Member

copumpkin commented Nov 18, 2016

Actually, no, not this implementation. We can do fetchzip recursively to do this without using fetchgit. fetchgit feels like a regression in performance, as it can take arbitrarily long. In big-O notation, fetchgit takes O(size of repo), whereas fetchFromGitHub nowadays takes O(size of tree you request).

@copumpkin
Copy link
Member

Anyway, I'm not super opposed to this version, but there are a few large git repos that I currently fetch via fetchFromGitHub and then manually plug submodules into. I could keep doing that with fetchSubmodules = false of course, but I'd prefer the recursive fetchzip implementation if not too much work.

@rasendubi
Copy link
Member Author

@copumpkin I have no idea how can I do that in general fashion now. Could you point me to the mentioned packages?

@copumpkin
Copy link
Member

Sorry, they're mostly in non-public projects, so not really. I just use fetchFromGitHub all over the place, basically, so I'm sensitive to it not getting slower 😄

} // passthruAttrs)
else
# We prefer fetchzip in cases we don't need submodules as the hash
# is more stable in that case.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could someone approve this? I'm starting to think the real reason is not hash stability but the speed and saving network usage.

@rasendubi
Copy link
Member Author

rasendubi commented Nov 18, 2016

It won't get slower, as the default value for fetchSubmodules is false. So basically, this patch makes it convenient to use fetchFromGitHub all over the place.

@copumpkin
Copy link
Member

I have no idea how can I do that in general fashion now

I go into that a bit in the other ticket, fwiw.

Either way this improves things, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants