Improve Flakeref for Gitlab repos. by Silver-Golden · Pull Request #8773 · NixOS/nix

Silver-Golden · 2023-08-02T19:34:07Z

Motivation

Gitlab has the option to have repos inside folders for example: https://gitlab.com/gitlab-org/build/omnibus-mirror/alertmanager

Under the current system (docs) we would have to use

example = {
  type = "gitlab";
  owner = "gitlab-org";
  repo = "build%2Fomnibus-mirror%2Falertmanager";
};

or

example.url = "gitlab:gitlab-org/build%2Fomnibus-mirror%2Falertmanager";

Context

I first encountered the problem myself in #6435 where I learnt about teh work around and that got me going for that time.
My university's computer society is now using Gitlab and NixOS for our servers and I can foresee that using %2F will cause some issues for folks being trained/taught NixOS.
Also I think it looks ugly AF (that alone should be enough reason to fix it).

My ideal solution would have been to allow slashes in the example.url above, however due to #4061 it could cause breaking changes for any flake using the current implementation.

So as a result I am targeting the attribute directly.

example = {
  type = "gitlab";
  owner = "gitlab-org";
  repo = "build/omnibus-mirror/alertmanager";
};

In the github.cc every / in the repo field will get replaced with a %2f, maybe not the best but it is functional
If there is a better way for doing it please let me know.
I added to the docs as well, not 100% sure its the ebst placement but I figured it was best to keep it close to where teh original mentioned the workaround.

I built nix and tested the changes locally (in wsl) and it works for my example above.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
documentation in the internal API docs
code and comments are self-explanatory
commit message explains why the change was made
new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

Relates to NixOS#6435

src/libfetchers/github.cc

roberth

The change in behavior is good. Arguably the repo attribute should be in terms of the gitlab "domain", which means it's a path with /s.

I'm not so sure about the overall implementation, but that's a pre-existing tech debt that we don't have to resolve right away - ie use a URL library or such. Afaic, we can merge this after resolving the std vs boost question.

roberth · 2023-08-11T11:00:34Z

src/libfetchers/github.cc

        auto host = maybeGetStrAttr(input.attrs, "host").value_or("gitlab.com");
+        auto repo = boost::replace_all_copy(getStrAttr(input.attrs, "repo"), "/", "%2F");
        // See rate limiting note below
        auto url = fmt("https://%s/api/v4/projects/%s%%2F%s/repository/commits?ref_name=%s",


Why are we constructing urls with string concatenation 🤦

The value of repo has to go into the url encoded, one way or another.
Currently people are manually encoding it for example: build%2Fomnibus-mirror%2Falertmanager.

The goal of this change is to allow both build%2Fomnibus-mirror%2Falertmanager and build/omnibus-mirror/alertmanager

roberth · 2023-08-11T11:07:47Z

src/libfetchers/github.cc

 #include <optional>
 #include <nlohmann/json.hpp>
 #include <fstream>
+#include <boost/algorithm/string/replace.hpp>


Why not just std::string's replace?

I did try using that initially.
The main problem i hit there was it can only replace a char with another char.
It does not work when you try to replace a char with a string.

Silver-Golden · 2023-08-11T12:22:42Z

Arguably the repo attribute should be in terms of the gitlab "domain", which means it's a path with /s.

Should I adjust it so that both the owner and repo can take slashes?

So that would give users the flexability of all these:

example1 = {
  type = "gitlab";
  owner = "gitlab-org";
  repo = "build/omnibus-mirror/alertmanager";
};

example2 = {
  type = "gitlab";
  owner = "gitlab-org/build";
  repo = "omnibus-mirror/alertmanager";
};

example3 = {
  type = "gitlab";
  owner = "gitlab-org/build/omnibus-mirror";
  repo = "alertmanager";
};

Silver-Golden · 2023-08-11T13:22:04Z

Just viewed yer profile and realised I should have mentioned ye @roberth

roberth · 2023-08-11T13:32:52Z

Should I adjust it so that both the owner and repo can take slashes?

Right, I think framing it as "repo has a path" was a bit wrong.

To be correct about it, in the GitLab data model we have:
A repo is 1:1 with a project.
A project has a name, which is the final part of its path.
A project has a namespace, which is a user or group.
A groups can be nested.

In Nix we have:
A namespace is called an owner, for consistency with GitHub.

So a slash in a repo is not actually possible; any such prefix to the repo should be specified in the owner instead.
Allowing a slash in the repo would therefore make the values in owner unreliable, which seems kinda bad.

I guess for gitlab it would have made more sense to have a single attribute for the whole path to the repo including the "owner" - whatever that means in a gitlab context. We shouldn't be making up such terms.

Silver-Golden · 2023-08-11T14:06:03Z

I see what ye mean, you would be thinking more of example3 above right?

Where things get tricky is ownship in Gitlab is a lot more nuanced than the flat structure of Github.
For example:

We have https://gitlab.skynet.ie/compsoc1 which as two groups, the computer society and skynet (social vs technical).
There is some overlap in membership between the two subgroups.

For https://gitlab.skynet.ie/compsoc1/skynet/nixos I would consider this the attribute

nixos = {
  type  = "gitlab";
  owner = "compsoc1/skynet";
  repo  = "nixos";
};

Which is in line of what ye are thinking of, however I would consider https://gitlab.skynet.ie/compsoc1/skynet/website/2023 to be like so:

website = {
  type  = "gitlab";
  owner = "compsoc1/skynet";
  repo  = "website/2023";
};

In both cases I consider compsoc1/skynet to be the owner because its teh same group of people managing both.
The closest ye would get on Github is something like this https://github.com/Byron/gitoxide/tree/main/cargo-smart-release where tehre are multiple related projects in a single repo.

(Semi serious, ~~pockets~~ folders is the quiet game changing feature of Gitlab)

roberth · 2023-08-11T14:51:20Z

~~pockets~~

As in something useful that we tend to take for granted? Interesting.

ownership in Gitlab is a lot more nuanced

Right! And this is not really represented in their data model. That means we won't agree on what the split should be and we should simplify the interface so that we don't burden users with coming up with their own ownership ideas where it doesn't really apply unambiguously.

But seriously, we should probably get rid of owner. Maybe path could be a replacement for both owner and repo?

@edolstra, would it be ok to have an attribute called path in the fetchTree arguments? There's some potential for confusion with the subflake dir parameter I guess. wdyt?

edolstra · 2023-08-11T15:18:06Z

Yes, path is a bit ambiguous. Maybe there is a better name?

K900 · 2023-08-11T15:20:14Z

Maybe we should just keep repo, and let owner be unset?

Silver-Golden · 2023-08-11T15:23:49Z

It wasnt the intention of mine earlier but I am more in favor of allowing slashes in both owner and repo, like in #8773 (comment) , gives flexability.
Also allowing owner to be unset would be grand as well

Silver-Golden · 2023-08-11T15:29:07Z

Ye, I would be quite happy with this

# top level group is "owner"
nixos = {
  type  = "gitlab";
  owner = "compsoc1";
  repo  = "skynet/nixos";
};
# subgroup is "owner"
nixos = {
  type  = "gitlab";
  owner = "compsoc1/skynet";
  repo  = "nixos";
};
# no owner
nixos = {
  type  = "gitlab";
  repo  = "compsoc1/skynet/nixos";
};

roberth · 2023-08-11T15:38:06Z

# no owner

That'd be my preferred syntax, and if we do keep the owner attr around as "user input" syntax, I think we should canonicalize to it before returning the "sourceInfo" to the user.

I don't think libfetchers is set up to normalize the attributes and remove owner from the returned attrset, is it?

Silver-Golden · 2023-08-11T15:52:14Z

Why lock it down to a specific one though?
Especially since all 3 can co-exist without any issue.

If its about making it easier for the user then any of those will be better than what we have now.
It took me about an hour last year to figure out why why "gitlab:c2842/misc/classbot"; wasnt getting accepted.

Especially easier to fit in than a new attribute.

roberth · 2023-08-11T18:38:30Z

I'm mostly ok with accepting multiple input syntaxes, but not happy about propagating that entropy into the rest of a Nix expression.
For example if in my flake I override the foo input of your flake, and your flake has a conditional based on foo.owner, I may unknowingly cause a problem by overriding foo (follows) despite specifying the exact same repository, just because we disagree about how long the owner prefix is.

So what I'm suggesting is for fetchTree to canonicalize the input attributes to prevent such a situation from arising. If inputs.foo.owner doesn't exist, we can't accidentally disagree about its value.

Silver-Golden · 2023-08-12T06:20:26Z

@roberth I can see where ye are coming from, however that is an already existing problem.

Currently I could have it like so:

nixos = {
  type  = "gitlab";
  owner = "compsoc1";
  repo  = "skynet%2Fnixos";
};
nixos = {
  type  = "gitlab";
  owner = "compsoc1%2Fskynet";
  repo  = "nixos";
};

So if you try to override the repo without also overriding the owner ye may run into issues.
I repeat, that is current behaviour, not introduced in this patch

And as far as I can tell there has been no issue raised in the three years since the lines were last touched.

Pushed up a commit to also allow slashes in owner.

@edolstra have you any thoughts on the convo above?

Ericson2314 · 2023-08-18T12:24:12Z

Triaged in the Nix team meeting

@edolstra: flakeref is tricky because extra slash means branch name
- roberth: could use double forward slash for branch name. Some other tool(s) do this (Haskell??)
@roberth: flakeref and structured data representation are distinct syntaxes and can be considered separately
@roberth: it's still experimental (but painful to change nonetheless)
Tricky problem: Gitlab doesn't have a owner/repo distinction, instead repos can be arbitrary paths. But switching to arbitrary paths would make refs/revs ambiguous.

Scratch space:

double slash: gitlab://group/subgroup/repo//branch
currently: gitlab://group%2Fsubgroup/repo/branch
gitlab://org/section/repo\sha
gitlab://org/section/repo|sha
gitlab://org/section/repo?ref=sha
gitlab://org/section/repo__________SOMETHING________sha

Decision: Will discuss further

Silver-Golden · 2023-08-18T12:33:13Z

@Ericson2314 thanks for discussing it in the meeting.
Are ye planning to expand the scope (in this MR/issue) from teh attribute to gitlab: url as well?
Or will it be seperate?

roberth · 2023-08-19T20:12:16Z

Are ye planning to expand the scope [...] to gitlab: url as well?

It's related but can be considered separately. The team hasn't made a decision about the URL yet, so I'd focus on just the structured data syntax.

Possible migration path for URLs

We could just break it, or do a migration. For a migration plan we only need to consider the user facing interface, but that's not to say that the implementation will be easy. libfetchers' architecture doesn't help with any of this, I believe.

current state
make the structured data work without owner (I don't think it already does)
add //ref support to all VCS fetchers
deprecate: warn when owner is set
deprecate: warn when the gitlab: URI contains a path. Tell users to pass ?repo=group/subgroup/repo
make the breaking change to the gitlab: URI syntax: third path component is not a ref anymore. Refs must now be ?ref= or //ref

1-3 are a good goal for 23.11
4-5 can be done for 24.05
the migration could be completed in 24.11.

So if we want to do a user friendly migration for the flakeref URIs it will take a while. Flakeref URIs are just a nice to have for the flakes feature, so we could stabilize flakes before stabilizing the URIs.

nixos-discourse · 2023-08-23T07:20:13Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-08-18-nix-team-meeting-minutes-80/32081/1

fricklerhandwerk · 2023-08-23T07:32:22Z

Reviewed in Nix team meeting 2023-08-21:

rehashed the state of discussion
discussed flake URIs more generally
considered making a breaking change for gitlab flakerefs
decision:
- structured representation: owner becomes optional, but is prepended to the repo if specified, and a warning is printed
- flake URI: options but no decision yet
  - add a special syntax such as //ref
    - even more special syntax
  - require escaping if there is more than one / in the path
    - not ergonomic
  - do more involved parsing, e.g. count path components
    - breaking change, but possible
  - resolve details with the service API
    - too slow
  - remove the special syntax and require people to use query parameters or the structured representation
    - cheap

nixos-discourse · 2023-08-23T07:33:58Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-08-21-nix-team-meeting-minutes-81/32082/1

Silver-Golden · 2023-10-09T21:24:52Z

@roberth @edolstra

Not sure if its intended but it seems the way the URL is handled has changed.
Best that I can tell is that something is replacing%2F with / in the path attribute before it gets to the fetcher because it is splitting off the part after the last / as if its the branch/ref, even if it is inputted in the form of %2F.

If ye want I could split this off into a new ticket.

replacing old 'nix-2.13.2'
installing 'nix-2.18.1'

This worked fine on 2.13.2 at least, not sure what was last version it worked on.

compsoc_public.url = "gitlab:compsoc1%2Fcompsoc/presentations?host=gitlab.skynet.ie";

silver@DESKTOP-7RB7S9E:/mnt/c/College/Compsoc/nixos$ nix develop
error:
       … while updating the lock file of flake 'git+file:///mnt/c/College/Compsoc/nixos?ref=refs/heads/main&rev=8ea737d57b224cdc8e5009ff47cb1e013ca71ac0'

       … while updating the flake input 'compsoc_public'

       … while fetching the input 'gitlab:compsoc1/compsoc/presentations'

       error: unable to download 'https://gitlab.skynet.ie/api/v4/projects/compsoc1%2Fcompsoc/repository/commits?ref_name=presentations': HTTP error 404

       response body:

       {"message":"404 Project Not Found"}

If I change the last slash in it to %2F result is the same

compsoc_public.url = "gitlab:compsoc1%2Fcompsoc%2Fpresentations?host=gitlab.skynet.ie";

silver@DESKTOP-7RB7S9E:/mnt/c/College/Compsoc/nixos$ nix develop
warning: Git tree '/mnt/c/College/Compsoc/nixos' is dirty
error:
       … while updating the lock file of flake 'git+file:///mnt/c/College/Compsoc/nixos'

       … while updating the flake input 'compsoc_public'

       … while fetching the input 'gitlab:compsoc1/compsoc/presentations'

       error: unable to download 'https://gitlab.skynet.ie/api/v4/projects/compsoc1%2Fcompsoc/repository/commits?ref_name=presentations': HTTP error 404

       response body:

       {"message":"404 Project Not Found"}

and for funsies use all /, still same error

compsoc_public.url = "gitlab:compsoc1/compsoc/presentations?host=gitlab.skynet.ie";

silver@DESKTOP-7RB7S9E:/mnt/c/College/Compsoc/nixos$ nix develop
warning: Git tree '/mnt/c/College/Compsoc/nixos' is dirty
error:
       … while updating the lock file of flake 'git+file:///mnt/c/College/Compsoc/nixos'

       … while updating the flake input 'compsoc_public'

       … while fetching the input 'gitlab:compsoc1/compsoc/presentations'

       error: unable to download 'https://gitlab.skynet.ie/api/v4/projects/compsoc1%2Fcompsoc/repository/commits?ref_name=presentations': HTTP error 404

       response body:

       {"message":"404 Project Not Found"}

fricklerhandwerk · 2023-10-09T22:10:51Z

@Silver-Golden yes, that's been an intentional change here #6614

Silver-Golden · 2023-10-09T22:24:32Z

Welp, once again it feels that gitlab is not even a second class citizen :(

This most likely means that it will require a rewrite of the src/libfetchers/github.cc logic to get it working, possibly #9031 in the longterm but as it currently stands I would see the current behaviour as a bug.
I'll do up an issue tomorrow and take a look at ways to solve it.

roberth · 2023-10-10T10:37:30Z

Best that I can tell is that something is replacing%2F with / in the path attribute before it gets to the fetcher because it is splitting off the part after the last / as if its the branch/ref, even if it is inputted in the form of %2F.

an intentional change here #6614

Still a bug though.

#6614 should be re-done by factoring out the parser, putting the freshly parsed strings into a struct and adding a bunch of unit tests.

I don't know how people can accept custom parsing code without a ton of tests. This is insane.

Another thing that needs to happen is that the gitlab code should stop inheriting any code that makes github assumptions.
Most of the logic in GitArchiveInputScheme should be moved down into a new GitHubLikeArchiveInputScheme.

Silver-Golden · 2023-10-14T14:24:57Z

@roberth I am planning to open a new issue ticket and merge request to fix the new bug introduced above, anything in particular from this one do ye want me to add?
(carved out some free time today and tomorrow)

roberth · 2023-10-15T10:00:21Z

@Silver-Golden I think that's a problem in the parsing/decoding logic, which wasn't really touched by this PR.

Silver-Golden · 2023-10-15T12:20:35Z

Switching over to #9160 for this PR.

libfetchers: allow gitlab repos to have slashes in them

d169123

Relates to NixOS#6435

Silver-Golden requested review from edolstra and fricklerhandwerk as code owners August 2, 2023 19:34

github-actions bot added documentation new-cli Relating to the "nix" command fetching Networking with the outside (non-Nix) world, input locking labels Aug 2, 2023

K900 suggested changes Aug 2, 2023

View reviewed changes

src/libfetchers/github.cc Show resolved Hide resolved

roberth reviewed Aug 11, 2023

View reviewed changes

roberth self-assigned this Aug 21, 2023

Silver-Golden force-pushed the master branch from d65e99a to d169123 Compare October 15, 2023 12:08

Silver-Golden mentioned this pull request Oct 15, 2023

Improve Flakeref for Gitlab repos. #9160

Closed

Silver-Golden closed this Oct 15, 2023

This was referenced Oct 15, 2023

#6614 broke flake URL decoding for Gitlab #9161

Open

Fix and Improve Flakeref for Gitlab repos. #9163

Open

Uh oh!

Conversation

Silver-Golden commented Aug 2, 2023

Motivation

Context

Checklist for maintainers

Priorities

Uh oh!

Uh oh!

roberth left a comment

Choose a reason for hiding this comment

Uh oh!

roberth Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

Silver-Golden Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

roberth Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

Silver-Golden Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

Silver-Golden commented Aug 11, 2023

Uh oh!

Silver-Golden commented Aug 11, 2023

Uh oh!

roberth commented Aug 11, 2023

Uh oh!

Silver-Golden commented Aug 11, 2023

Uh oh!

roberth commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edolstra commented Aug 11, 2023

Uh oh!

K900 commented Aug 11, 2023

Uh oh!

Silver-Golden commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Silver-Golden commented Aug 11, 2023

Uh oh!

roberth commented Aug 11, 2023

Uh oh!

Silver-Golden commented Aug 11, 2023

Uh oh!

roberth commented Aug 11, 2023

Uh oh!

Silver-Golden commented Aug 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Aug 18, 2023

Uh oh!

Silver-Golden commented Aug 18, 2023

Uh oh!

roberth commented Aug 19, 2023

Uh oh!

nixos-discourse commented Aug 23, 2023

Uh oh!

fricklerhandwerk commented Aug 23, 2023

Uh oh!

nixos-discourse commented Aug 23, 2023

Uh oh!

Silver-Golden commented Oct 9, 2023

Uh oh!

fricklerhandwerk commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Silver-Golden commented Oct 9, 2023

Uh oh!

roberth commented Oct 10, 2023

Uh oh!

Silver-Golden commented Oct 14, 2023

Uh oh!

roberth commented Oct 15, 2023

Uh oh!

Silver-Golden commented Oct 15, 2023

Uh oh!

roberth commented Aug 11, 2023 •

edited

Loading

Silver-Golden commented Aug 11, 2023 •

edited

Loading

Silver-Golden commented Aug 12, 2023 •

edited

Loading

fricklerhandwerk commented Oct 9, 2023 •

edited

Loading