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

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.

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

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.

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

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

I thought it was mandatory.

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.

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?

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

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

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.

Forgot to remove this during cleanup will be addressed.

Ah okay. Will remove.

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

I changed this, please check it.

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.

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.

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 ?

Yes, please :-)

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.

Will remove it and keep it simple.

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.

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

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.

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

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.

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.

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.

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

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?

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