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

Add fetcher: fetchFromSourcehut #102225

Merged
merged 2 commits into from Mar 11, 2021
Merged

Add fetcher: fetchFromSourcehut #102225

merged 2 commits into from Mar 11, 2021

Conversation

@luc65r
Copy link
Contributor

@luc65r luc65r commented Oct 31, 2020

Motivation for this change

I start to see programs hosted on sourcehut, so I think adding a fetcher will be helpful.

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 nixpkgs-review --run "nixpkgs-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.
@luc65r
Copy link
Contributor Author

@luc65r luc65r commented Oct 31, 2020

Should I continue modifying srcs to use fetchFromSourcehut instead of fetchurl?

@matthiasbeyer
Copy link
Contributor

@matthiasbeyer matthiasbeyer commented Oct 31, 2020

One quick question: is the sourcehut url format stable? If yes, never mind, but if it isn't, maybe we should be able to override the function that generates the url!

@eyJhb
Copy link
Member

@eyJhb eyJhb commented Oct 31, 2020

I think the domain should be able to change, as far as I know you can host your own instance.

Besides that, we need to get the base approved, with how fetchFromSourcehut works, before any more are changed (I would say. Maybe @worldofpeace can help on this?

Also tagging @SuperSandro2000 and @mweinelt as they seemed interested in this!

@luc65r
Copy link
Contributor Author

@luc65r luc65r commented Oct 31, 2020

is the sourcehut url format stable

I hope!

@tsujp
Copy link

@tsujp tsujp commented Nov 1, 2020

IIRC Drew has said that the ~ is not going to change so the URL is stable (if that's what that refers to?) but as mentioned before you can host SourceHut yourself and so it could be git.your.domain. Perhaps an FQDN override (or something better named since your.domain isn't fully qualified, it's missing the git giving git.your.domain).

One more thing to note, whereas with GitHub the owner can refer to a person or org on SourceHut the discussion is ongoing on what the organisation (user group) prefix will be. Examples in the air are say ^. So there'd need to be some distinction between an organisation-equivalent (user group) and a person (single user). Here is the discussion thread relevant to that: https://lists.sr.ht/~sircmpwn/sr.ht-discuss/%3CC0L8LGIM0C2I.3O209D1TSO6M3%40homura%3E

@luc65r
Copy link
Contributor Author

@luc65r luc65r commented Nov 1, 2020

Perhaps an FQDN override

This is already implemented: domain ? "sr.ht". domain may not be the best name for it, but it's called like that in the gitlab fetcher.

on SourceHut the discussion is ongoing on what the organisation (user group) prefix will be

Thanks for the information!
I think there are two solutions to that.

  • we add group ? false to the args and swap ~ to whatever it will be (like + or ^) if group is true.
    Usage example with git.sr.ht/^sourcehut/core.sr.ht (or git.sr.ht/+sourcehut/core.sr.ht):
src = fetchFromSourcehut {
  owner = "sourcehut";
  group = true;
  repo = "core.sr.ht";
  rev = ...;
  sha256 = ...;
};
  • we default owner and group to null and use the one that is not null with the correct prefix.
src = fetchFromSourcehut {
  group = "sourcehut";
  repo = "core.sr.ht";
  rev = ...;
  sha256 = ...;
};

Usage example with git.sr.ht/~sourcehut/core.sr.ht in both cases:

src = fetchFromSourcehut {
  owner = "sourcehut";
  repo = "core.sr.ht";
  rev = ...;
  sha256 = ...;
};

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Nov 1, 2020

Wouldn't it be easer to use one attribute for users and one for groups?

src = fetchFromSourcehut {
  user = "sourcehut";
  repo = "core.sr.ht";
  rev = ...;
  sha256 = ...;
};
src = fetchFromSourcehut {
  group = "sourcehuters";
  repo = "core.sr.ht";
  rev = ...;
  sha256 = ...;
};

@matthiasbeyer
Copy link
Contributor

@matthiasbeyer matthiasbeyer commented Nov 1, 2020

Wouldn't it be easer to use one attribute for users and one for groups?

Might be a better option, although it would make the code more complicated. And also it might end up more confusing if people read the code of the fetchFromSourceHut function.
A flag is less complex to understand and adds little overhead IMO.

@luc65r
Copy link
Contributor Author

@luc65r luc65r commented Nov 1, 2020

Wouldn't it be easer to use one attribute for users and one for groups?

I think we should keep the owner naming for what you are referring as user, even if a group is technically an owner, because that's how the other fetchers are naming it.

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Nov 1, 2020

A flag is less complex to understand and adds little overhead IMO.

having to set a flag if I want to download things from a group is not very intuitiv for people coming from GitHub, GitLab or Gitea because they all don't have that difference.

I think we should keep the owner naming for what you are referring as user, even if a group is technically an owner, because that's how the other fetchers are naming it.

Didn't you come to the conclusion that groups on sourcehut are a bit different from all other platforms? Than the fetcher can also be a bit different.

The fetcher is new and we are not breaking anything so we can do it right the first time.

And if users are confused we can throw an explanation if they pass owner into the fetcher.

@luc65r
Copy link
Contributor Author

@luc65r luc65r commented Nov 1, 2020

Didn't you come to the conclusion that groups on sourcehut are a bit different from all other platforms?

I don't know how they work on github and gitlab, so I can't tell if it is different.

And if users are confused we can throw an explanation if they pass owner into the fetcher.

That could be a solution, but we are all used to give owner, repo, rev and sha256 to fetchers. I think it would be annoying to have a fetcher that uses a different naming for one thing, and I would see myself mixing up owner and user.
It would not be a big deal, especially if it throws a good error, but I see it being annoying.

But if other people think too it should be user and group, I'm not against it, I just think there are drawbacks.

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Nov 1, 2020

I don't know how they work on github and gitlab, so I can't tell if it is different.

the URL scheme is just the same. github.com/NixOS could be a group or a user.

But if other people think too it should be user and group, I'm not against it, I just think there are drawbacks.

Or the owner is prefixed with ~ or something else but adding a flag isGroup = true is the most inconvenient solution.

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Nov 25, 2020

Can you please fix the merge conflict?

@duairc
Copy link
Contributor

@duairc duairc commented Dec 25, 2020

I don't want to bikeshed, but by analogy with fetchFromGitHub, I would have expected this to be called fetchFromSourceHut.

@luc65r
Copy link
Contributor Author

@luc65r luc65r commented Dec 25, 2020

I don't want to bikeshed

We are already in a bikeshed problem :P

by analogy with fetchFromGitHub, I would have expected this to be called fetchFromSourceHut.

IDK, because it's GitHub, GitLab and SourceForge, but it's sourcehut or Sourcehut, they chose to not capitalize it for some reason.

We still need someone to decide the best solution for the group/user/owner/flag problem.

@duairc
Copy link
Contributor

@duairc duairc commented Dec 25, 2020

That's fair, I'm just catching up with the rest of the thread now. What if we just sidestep the owner/group thing altogether, and just say owner = "~sircmpwn";? And if group support is added further down the line with some other sigil, we can just do owner = "^group", without having to change the fetcher code? You could still have a shorthand as follows:

{ user ? null
, owner ? "~${user}"
}

And if/when they make up their mind about what sigil to use, you could update that to:

{ user ? null
, group ? null
, owner ? if !isNull group then "^${group}" else "~${user}"
}

But in any case there's no reason for this PR to be blocked on that.

@luc65r
Copy link
Contributor Author

@luc65r luc65r commented Dec 25, 2020

Thanks, this is a great solution!

@SuperSandro2000 what do you think?

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Dec 27, 2020

Thanks, this is a great solution!

@SuperSandro2000 what do you think?

sounds go to me.

@sikmir
Copy link
Member

@sikmir sikmir commented Jan 13, 2021

I start to see programs hosted on sourcehut, so I think adding a fetcher will be helpful.

Just in case:

$ egrep -ir --include="*.nix" "url.*=.*sr.ht" nixpkgs/ | wc -l
29

@ofborg ofborg bot requested a review from ccellado Jan 24, 2021
@ofborg ofborg bot requested a review from ccellado Jan 27, 2021
@ofborg ofborg bot requested a review from ccellado Feb 10, 2021
@ofborg ofborg bot requested a review from ccellado Feb 27, 2021
Copy link
Member

@sternenseemann sternenseemann left a comment

If you'd add an appropriate section to the nixpkgs manual, that'd be great!

Overall I think this is worth it, for using fetchzip over fetchgit and the .hg_archival.txt deletion.
Also I think it's wise to leave out a fetchFromGitHub-style fallback to fetchgit (as has been done here) because that fetcher is quite a mess in terms of internal logic.

pkgs/build-support/fetchsrht/default.nix Outdated Show resolved Hide resolved
Mic92
Mic92 approved these changes Feb 28, 2021
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested a review from ccellado Mar 8, 2021
@primeos
Copy link
Member

@primeos primeos commented Mar 10, 2021

I lost track of this since my last comment but I intend to give it a final review tomorrow and hopefully merge it (so that we can also properly fix #115397), so please let me know if there are any important objections (likely not but just to be sure). And if someone else reviews/merges it before that that's obviously fine as well :)

Copy link
Member

@primeos primeos left a comment

Tested it on nixos-unstable and everything worked as expected :)
Huge thanks to you @luc65r and also to all the reviewers!

in fetchzip (recursiveUpdate {
inherit name;
url = "${baseUrl}/archive/${rev}.tar.gz";
meta.homepage = "${baseUrl}/";
Copy link
Member

@primeos primeos Mar 11, 2021

Suggested change
meta.homepage = "${baseUrl}/";
meta.homepage = baseUrl;

On sr.ht the "/" isn't appended and e.g. scdoc's man page contains "Up-to-date sources can be found at https://git.sr.ht/~sircmpwn/scdoc [...]". So we might want to change this later but it shouldn't really matter much and we could do it at any time.

@@ -475,6 +475,8 @@ in

fetchFromSavannah = callPackage ../build-support/fetchsavannah {};

fetchFromSourcehut = callPackage ../build-support/fetchsourcehut { };
Copy link
Member

@primeos primeos Mar 11, 2021

It's a bit unfortunate that upstream uses (at least) sourcehut, Sourcehut, and SourceHut.
Picking fetchFromSourcehut should be fine though :)

@primeos primeos merged commit 938453e into NixOS:master Mar 11, 2021
19 checks passed
@rmcgibbo rmcgibbo mentioned this pull request Mar 16, 2021
10 tasks
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