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.

Sign up for GitHub

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

Jump to bottom

fetchipfs: draft #18296

Closed
wants to merge 2 commits into from
Closed

fetchipfs: draft #18296

wants to merge 2 commits into from
+148 −4

Conversation

knupfer
Copy link
Contributor

@knupfer knupfer commented Sep 4, 2016

Motivation for this change

A first draft to start discussion for a fetchipfs fixed output derivation.

Things done
  • Built on platform(s)
    • NixOS
    • OS X
    • 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 is a wip:
fetchipfs will take an ipfs path and an optional url. It tries first to download from ipfs via the localhost api and if that fails it'll try to download from the url and add the path to ipfs.

I've changed the hello program to showcase usage. After reaching consensus in discussion and filling holes, I'll force push to clean up the branch and remove the changes to hello.

The reasoning to not depend on ipfs is that it isn't that small and needs to be run as daemon, which would be only for nixos possible in an efficient manner.

Limitations:

  • It expects to find the ipfs api under localhost:5001, this should be probably customizable with environment variables.
  • The ipfs path can and will be only verified when the ipfs daemon is running and reachable (if it's not in ipfs, but the daemon is reachable, it will download from the url, verify the sha and verify the resulting ipfs path by adding it to ipfs).

Sorry, something went wrong.

@mention-bot
Copy link

@knupfer, thanks for your PR! By analyzing the annotation information on this pull request, we identified @edolstra, @vcunat and @domenkozar to be potential reviewers

@fpletz fpletz added the 2.status: work-in-progress This PR isn't done label Oct 8, 2016
fpletz
fpletz reviewed Oct 8, 2016
View reviewed changes
pkgs/applications/misc/hello/default.nix
url = "mirror://gnu/hello/${name}.tar.gz";
sha256 = "0ssi1wpaf7plaswqqjwigppsg5fyh99vdlb9kzl7c9lng89ndq1i";
src = fetchipfs {
url = "https://ftp.gnu.org/gnu/hello/hello-2.10.tar.gz";
Copy link
Member

Choose a reason for hiding this comment

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

I think it would make sense to be able to specify a fetch* function here so other fetchers would also be supported, like fetchFromGitHub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that that would be possible. The hash must be a recursive hash of the unpacked source, so this part has to be hard coded and wouldn't be possible with other fetchers.

@rehno-lindeque rehno-lindeque mentioned this pull request Nov 21, 2016
Closed
@7c6f434c 7c6f434c closed this in d71833e Nov 4, 2017
@cstrahan
Copy link
Contributor

cstrahan commented Nov 5, 2017

/cc @veprbl

Could this perhaps use something like ipget (introduced in #30704) instead of depending on a server running on localhost?

/cc @7c6f434c

It looks like the change to the hello package was also applied, but I don't think that was desired (per comments above).

@cstrahan cstrahan self-assigned this Nov 5, 2017
@7c6f434c
Copy link
Member

7c6f434c commented Nov 5, 2017 via email

@veprbl
Copy link
Member

veprbl commented Nov 6, 2017 •
edited
Loading

@cstrahan
I'm not an expert on the IPFS technology, but I think using ipget in fetchipfs feasible. There are however arguments against doing that. My biggest concern is that ipget has many dependencies and it would require for go infrastructure in nixpkgs to be in working state at all times for fetchipfs to work. There is also issue that ipget only leeches and never seeds, which might be slightly harmful to the IPFS network. Using fallback to a public gateway doesn't seem as such a bad idea to me, as it requires no extra dependency. If reliability is a concern, we could have a list of gateways implemented similarly to mirrors in nixpkgs.fetchurl.

edit: Another thing is that to build ipget they use gx/gx-go build system which also tries to use local IPFS daemon and falls back to the public gateway. https://github.com/whyrusleeping/gx/blob/8c59fe068225a38dc315a80543570a3ba9091da0/README.md#requirements

@knupfer
Copy link
Contributor Author

knupfer commented Nov 6, 2017

At the moment, the usage of ipget is a bit problematic. It doesn't support getting tars added with tar cat, therefore we would loose deduplicating content of tarballs and only dedupe on tarball granularity... Besides it would add ipget as a dependency. We could add as a fallback curling from a gateway.

My intention was, as stated in the first comment, not to alter hello, but on the other hand, it works.

@edolstra
Copy link
Member

Please revert. Switching to IPFS as a mechanism for fetching package sources is a major decision that should be done via the RFC process.

7c6f434c added a commit to 7c6f434c/nixpkgs that referenced this pull request Nov 13, 2017
@7c6f434c
Revert switching hello to fetchipfs, re: NixOS#18296
bd8cd3e
@vcunat
Copy link
Member

vcunat commented Nov 13, 2017

You mean reverting the hello change?

@7c6f434c
Copy link
Member

@vcunat I have reverted hello.src and the related change.

@davidak
Copy link
Member

davidak commented Apr 29, 2018

@knupfer could you add it again without touching hello?

@7c6f434c
Copy link
Member

It is in master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpletz fpletz

Assignees

@cstrahan cstrahan

Labels
2.status: work-in-progress This PR isn't done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@knupfer @mention-bot @cstrahan @7c6f434c @veprbl @edolstra @vcunat @davidak @fpletz