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

fetchFromGitiles: init #74862

Merged
merged 2 commits into from Dec 17, 2019
Merged

fetchFromGitiles: init #74862

merged 2 commits into from Dec 17, 2019

Conversation

@alyssais
Copy link
Member

alyssais commented Dec 2, 2019

Motivation for this change

This has the same motivation as fetchFromGitHub/fetchFromGitLab --
it's cheaper to download a tarball of a single revision than it is to
download a whole history.

I could have gone with domain/group/repo, like fetchFromGitLab, but it
would have made implementation more difficult, and this syntax means
it's a drop-in replacement for fetchgit, so I decided it wasn't worth
it.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

alyssais added 2 commits Nov 24, 2019
This has the same motivation as fetchFromGitHub/fetchFromGitLab --
it's cheaper to download a tarball of a single revision than it is to
download a whole history.

I could have gone with domain/group/repo, like fetchFromGitLab, but it
would have made implementation more difficult, and this syntax means
it's a drop-in replacement for fetchgit, so I decided it wasn't worth
it.
This is only the easy cases -- some fetchgit uses that point to
Gitiles instances are in generated code, where the generating code
would have to know in advance if it was fetching from Gitiles or not.
I don't think this is worth it.
@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Dec 15, 2019

What can be rev? Literal commit ID or a tag name?

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Dec 15, 2019

rev can be anything that git understands. so a commitish, tag, or ref. https://git-scm.com/docs/git-rev-parse#_specifying_revisions

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Dec 15, 2019

Then was «refs/tags/version» replacement to «version» necessary when switching to Gitiles?

@alyssais

This comment has been minimized.

Copy link
Member Author

alyssais commented Dec 16, 2019

I think what you’re talking about is actually a change from fetchgit to fetchFromGitHub I did as cleanup while introducing fetchFromGitiles in the same file. I dropped the refs/tags/ prefix for consistency with the rest of Nixpkgs, where refs/tags/ is used rarely.

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Dec 16, 2019

unless a repo is doing something like using branches and tags with the same names, but different commits, then it wont matter.

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Dec 16, 2019

I am just an annoying literalist: fetchFromGitiles says it accepts arguments similar to fetchgit, and the latter says it prefers full refs/tags/ form so I wonder whether Gitiles fully supports the full form.

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Dec 16, 2019

not familiar with gitiles, 🤷‍♂

@alyssais

This comment has been minimized.

Copy link
Member Author

alyssais commented Dec 16, 2019

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Dec 16, 2019

@alyssais

This comment has been minimized.

Copy link
Member Author

alyssais commented Dec 16, 2019

@alyssais alyssais mentioned this pull request Dec 16, 2019
4 of 10 tasks complete
@7c6f434c 7c6f434c merged commit 26df2f4 into NixOS:master Dec 17, 2019
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.