Add USE_GITLAB support

[ultima]

Summary

• Add USE_GITLAB support for downloading from gitlab sites

GitLab is widely used and it is long due for added support to simplify.
This implementation has a similar format as the USE_GITHUB and should
be easy to adopt.

Sample:

USE_GITLAB= yes | nodefault
GL_SITE= protocol://domain:443/webroot   (Default: https://gitlab.com)
GL_ACCOUNT= accountname                  (Default: ${PORTNAME})
GL_PROJECT= projectname                  (Default: ${PORTNAME})
GL_COMMIT= 40charactercommithash         (Default: None, required variable)
GL_TUPLE= [site[:port][/webroot]:]account:project:commit[:group][/subdir]
          https://foobar.com:9838/gitlab:accountname:projectname:160bitcommithash:group/subdir

[ultima]

I can't take full credit for this added feature. Most of the code was a copied from the USE_GITHUB feature then modified as appropriate. Most of the credit should go to the creator of the USE_GITHUB code and this would have taken me much longer to write otherwise.

Mk/bsd.port.mk

2867–2872

Was not sure where the proper area for this would this should be.

[matthew]

This is great. There will need to be updates to the PHB to describe this. Also, are there any specimen ports that can
be converted to use this?

[mat]

Mk/bsd.sites.mk

543–544

Is there a reason not to always get the commit? Because if there is, it feels it should be on a per-tuple configuration variable and not global.

Also, I think the directory in the distfile always has the commit hash, which is why it is mandatory.

549

The :group should not be optional. I know it is in USE_GITHUB, and it was a bad design idea to begin with.

569

As there can be more than one GL_SITE, this feels somewhat wrong.
Same for the GL and GITLAB in the abbrevs things later, I do not think they should be there. It would allow removing all the :MGL checks and always set them.

The only reason there is one for USE_GITHUB is because of the GHC thing that piggybacks on USE_GITHUB but should not.

634–636

I don't really understand why this is not simply:

​GL_SITE_${_group}?= ${GL_SITE_DEFAULT}

639

I thought it was mandatory.

661–662

I'm not sure there is a point to having the conversion to GL_TUPLE. The only reason I added it to USE_GITHUB is because GH_TUPLE came late, and I was tired of having to convert stuff manually.

[se]

I have tested an uncommitted port of mine, and found, that I needed to use the full hash (not only 40 bits) and set (the undocumented) GL_TAGNAME due to the name of the work directory in the TAR file created by GitLab (which included the tag and the full hash).

My (working) test parameters are:

GL_ACCOUNT= esr
GL_PROJECT= open-adventure
GL_TAGNAME= 1.4
GL_COMMIT= 2d575dd0e37a5e03f5bfc9b23810f24778e3a3c3

[ultima]

In D12162#252562, @se wrote:

I have tested an uncommitted port of mine, and found, that I needed to use the full hash (not only 40 bits) and set (the undocumented) GL_TAGNAME due to the name of the work directory in the TAR file created by GitLab (which included the tag and the full hash).

My (working) test parameters are:

GL_ACCOUNT= esr
GL_PROJECT= open-adventure
GL_TAGNAME= 1.4
GL_COMMIT= 2d575dd0e37a5e03f5bfc9b23810f24778e3a3c3

Typo, should be 40 character hash, it was just an example in summary, will fix. GL_TAGNAME is not being added for USE_GITLAB and that variable is not being used in your setup. By default DISTVERSIONFULL is the GL_TAGNAME, alternatively USE_GITLAB= commit will change this behavior but the distname is not change which means there will be problems with WRKSRC being changed from ${GL_PROJECT}-${DISTVERSIONFULL}-${GL_COMMIT} to ${GL_PROJECT}-${GL_COMMIT}-${GL_COMMIT}.

In D12162#252541, @matthew wrote:

This is great. There will need to be updates to the PHB to describe this. Also, are there any specimen ports that can
be converted to use this?

Will do, I wanted to get this reviewed and tested to find some items I may not have seen or should not be added. Any GitLab site used in MASTER_SITES= should be able to use it, I was using audio/vm-lv2 port to test with. If switching from USE_GITLAB= yes to USE_GITLAB= commit, the dist needs to be redownloaded. I left as this format because it is likely one would start with the default behavior, if it isn't downloading as expected change to = commit and it will download using commit. This will give different results when testing.

Mk/bsd.sites.mk

543–544

Pondered this for awhile. The commit is only needed, but the WRKSRC is generated like so. ${GL_PROJECT}-(download method)-${GL_COMMIT}. downloadmethod can be commit or tag so using commit end up being the commit twice and my ocd spikes. I think in most cases the USE_GITLAB= commit won't be used.

On the other hand for the tuple it would make the string slightly more complicated than it should be and not being a required variable should be omitted in the design.

If a change should occur removing the = commit is probably the better route. Do you think this should be done?

549

Yeah you are right. This is something that tripped me up a few times when using GH_TUPLE. Will rework.

569

Will be addressed, there were a few comments I ran out of time to make that I wanted looked at and this was one.

634–636

The issue here is this variable is being defined in the for loop and ?= only defined with unassigned variables. There are three other places this can be addressed and this one has the least cost.

639

Forgot to remove this during cleanup will be addressed.

661–662

Ah okay. Will remove.

[ultima]

Mk/bsd.sites.mk

532–533

One thing also on my mind, should we allow portnumber in the custom site?

[ultima]

• Slight modification to filter for GL_SITE tuple
• Using group in tuple is no longer optional
• Changed MASTER_SITES area
• Cleaned up var
• Removed convert to tuple, not needed

[ultima]

Mk/bsd.sites.mk

569

I changed this, please check it.

[mat]

Mk/bsd.sites.mk

543–544

I can't parse your first paragraph.
From what I understand, when you download a file from gitlab, the commit will always be present in the directory in the tarball.
So it is mandatory to have it.
So having two possible cases for stuff, one where the commit is used and one where it is not used makes little sense, as we always have the commit available, so we can always use it.
I mean, it adds absolutely nothing, except complexity in the code here, and in the mind of our users who will wonder when they should, or not, use it.

612

Would be nice to have a _GL${_GITLAB_REV} appended to it, for the same reason we have it for github, so that in case they change the way they do things, we can have two different flavors of it.
Especially as gitlab is not like github where there is only one version of it around.

634–636

I do not understand your explanation.

All of GL_SITE_foo, like GL_ACCOUNT_foo or GL_PROJECT_foo can be undefined. Why would GL_SITE_foo get a special consideration ?

[mat]

Mk/bsd.sites.mk

532–533

Yes, please :-)

[ultima]

Mk/bsd.sites.mk

543–544

Will remove it and keep it simple.

612

Hmm, I'm still not positive I understand the use case but will add it. Would it be if GitLab/GitHub decides to change the url for downloading it is similar to how PORTREVISION force downloading at the updated url? Didn't even consider this that would be nice to have.

634–636

They can only be undefined when not using tuple. The first parse with tuple will define the empty variable like so GL_SITE+= :GROUP(example of when empty). Because the value is not empty, but the group is not, the for loop will then define GL_SITE_GROUP= . Due to being defined with an empty value the define when undefined(?=) variable assignment will fail.

There are two other places this could be fixed. First would be adding a check during the initial tuple parse, this would be more costly due to using a complicated regex string to check if the variable should be defined.

Second, modifying the for loop to verify strings are not empty before defining, this would be better but is still more costly because it will occur way more than we want.

The one I used, only checked if it is empty once on the defined group and is a simple empty check. Therefore costing the least out of the solutions.

The only time the empty check is not needed is when tuple is not used and instead everything is defined like so GL_VARIABLE= foobar[,:]GROUP. Otherwise they will be defined. I was on this problem for awhile trying to figure out why I kept getting errors while fetching.

It just occurred to me a third option, changing _GL_GROUPS to check for anything before the ":". This would probably be a better solution.

[ultima]

• Added the ability to set port in GL_SITE
• Fixed a bug where setting tuple causes default values for DEFAULT group to not set, will update the github patch to include this fix
• Added GL revision for DISTNAME
• Removed USE_GITLAB= commit, this just add complexity that isn't needed

[ultima]

Mk/bsd.sites.mk

634–636

Tested the last bit, it isn't a good solution. Hope the previous explanation makes sense. =]

[ultima]

Mk/bsd.sites.mk

560–564

To explain a bit more, These variables have to be different from the user variable or the defaults do not get set. We also need to add the user defined variable in the for below and add it to this temp variable. This will allow the default values to be applied to the DEFAULT group.

[ultima]

Mk/bsd.sites.mk

560–564

Disregard this bit I was having issues because when I added it I had a test case in place. It will be reverted.

[ultima]

This comment was removed by ultima.

[mat]

Mk/bsd.sites.mk

612

We added it when Github changed their API For downloading files, before, they did like gitlab does and included the commit hash in the directory in the distfile, and one day, they changed to what they do now, use whatever "tag/commit" you ask for.
The idea si that gitlab changes on day, we will need to support two (or more) schemes are not every gitlab instances will be updated at the same time.

634–636

Mmmm, ok, I get what the problem is.

Maybe it would be better to skip empty values and not create variables that are empty.

Replacing

​${_gl_v}_${_group}= ${_v_ex:C@^(.*):[^/:]+$@\1@}

with

.if !empty(_v_ex:C@^(.*):[^/:]+$@\1@)
​${_gl_v}_${_group}= ${_v_ex:C@^(.*):[^/:]+$@\1@}
.endif

or something.

[ultima]

Mk/bsd.sites.mk

634–636

Ok, will change. The main reason I made the check after instead of before is because the check would run significantly less and be a simpler check.

[ultima]

Mk/bsd.sites.mk

634–636

I can't figure out why but for some reason .if !empty(_v_ex:C@^(.*):[^/:]+$@\1@) is always evaluating as false.

[ultima]

Mk/bsd.sites.mk

612

Reflecting a bit, this should probably be user editable because the API could be different per site. Do you think the GL_SITEREV should be added? Here are my thoughts.

GL_SITEREV= 0 (0 will be the default and increase with https://gitlab.com.)
GL_TUPLE= [site[:port][/webroot][:_siterev]:]account:project:commit[:group][/subdir]

Although it would be an unused until an API change is required. Should this be left out until a valid issue occurrence?

[ultima]

• Added ability to add webroot

[mat]

As an example, you give:

https://foobar.com:9838/gitlab:accountname:projectname:160bitcommithash:group/subdir

Have you tried it works with a port but no webroot:

https://foobar.com:9838:accountname:projectname:160bitcommithash:group/subdir

And without a port, but with a username that only has digits:

https://foobar.com:42:projectname:160bitcommithash:group/subdir

(corner case, I know, but I would rather think about it now, than a very long wtf moment later)

[ultima]

In D12162#257817, @mat wrote:

As an example, you give:

https://foobar.com:9838/gitlab:accountname:projectname:160bitcommithash:group/subdir

Have you tried it works with a port but no webroot:

https://foobar.com:9838:accountname:projectname:160bitcommithash:group/subdir

And without a port, but with a username that only has digits:

https://foobar.com:42:projectname:160bitcommithash:group/subdir

(corner case, I know, but I would rather think about it now, than a very long wtf moment later)

I know I have tested every case possible at one point, but I may have made a small change that broke some other cases and forgot to retest. I'll have to create a test suite and really get this corrected. I want to also add the GL_REVISION to the string as well, but this is becoming extremely complex to parse correctly. I am starting to think it maybe necessary to change the approach a bit. Maybe.

[ultima]

• Fixed regex string for TUPLE, The TUPLE string should now be near bullet-proof.

[ultima]

I'm going to leave out the GL_REVISION variable until it is needed in the future unless you want it in now. I just don't see a good reason to add a test for valid revisions when there currently is only one present.

[ultima]

ping portmgr

I haven't actually looked at this in awhile, but this should be ready for commit. The last item that needed to be worked on was the regex string for GL_TUPLE, which was resolved in the latest patch. To test I created 20-25 test cases and they all passed.

[mat]

I completely forgot about this.

[ultima]

In D12162#302561, @mat wrote:

I completely forgot about this.

Oh I didn't even notice it was accepted until now. I want to run some tests to make sure I didn't miss anything a few months ago and if it looks good I'll commit.

[ultima]

Replying to myself, I thought I had created a possible issue in the regex rules for TUPLE, but I forgot that I actually fixed it months ago. It is ready and will be committed soon!

[mat]

It's only taken me a year to see an obvious bug in the code. (I'm working on cleaning things up a bit in D20140, and it'll be fixed there.)

head/Mk/bsd.sites.mk

619 ↗

(On Diff #39907)

This here, has ${GL_ACCOUNT}, I think it should be ${GL_ACCOUNT_${_group}}.