Cleanup duplicate code in USE_GITHUB

[ultima]

Summary

Cleanup duplicate code in USE_GITHUB

PR:\ 221816
Reviewed by\: lifanov (mentor), matthew (mentor), ..., ...
Approved by\: lifanov (mentor), matthew (mentor), ..., ...
Differential Revision\: https://reviews.freebsd.org/DXXXXX

While adding GitLab into the USE_GITHUB code base, I noticed a lot of
duplicate code and decided to clean it up a bit.

Originally I intended to simply add USE_GITLAB into the USE_GITHUB
code base, because of the mostly similar bits it will use, but after
seeing how much this will reduce the readability, I decided to remove
it and create its own section. This is just leftover improvements for
the USE_GITHUB code. It should not change any functionality.

[lifanov]

This looks pretty awesome to me!

However, for this change you also need approval from someone on portmgr (like mat) and an exp-run.

[ultima]

In D12125#251704, @lifanov wrote:

This looks pretty awesome to me!

However, for this change you also need approval from someone on portmgr (like mat) and an exp-run.

How does one start an exp-run?

[lifanov]

You file a PR flagged exp-run with a patch attached to it and explaining what it's supposed to.

There are instructions for it here:
https://www.freebsd.org/doc/en/articles/committers-guide/ports.html#ports-exp-run

[mat]

This may be going meta one step too far, it is getting very hard to read.

One thing you could do too would be to remove the _TEMP variables, they became bad kludges since we dropped support for 9.

[ultima]

• Renamed the *_TEMP variables to make their purpose clear

[ultima]

In D12125#251712, @mat wrote:

This may be going meta one step too far, it is getting very hard to read.

One thing you could do too would be to remove the _TEMP variables, they became bad kludges since we dropped support for 9.

Err, you said drop them and I renamed them.... Let me rework this. Can groups be named all ALL and default, or is this the dropped support for 9 part you are talking about?

[ultima]

Removed one of the *_TEMP, because it is unnecessary, renamed the other.

[mat]

In D12125#251709, @ultima wrote:

How does one start an exp-run?

19.8.5. How can an experimental test build of the ports tree (exp-run) be requested?, though do not ask for one before the patch is ok.

Err, you said drop them and I renamed them.... Let me rework this. Can groups be named all ALL and default, or is this the dropped support for 9 part you are talking about?

No, I only talked about the variables, all, ALL and default are used in the framework, they cannot be used as groups. The checks for all/ALL/defauld is, technically, not needed here, as it is checked in the MASTER_SITES parsing code afterwards. On the other hand, if removed and one use one of those groups, the error message would be unhelpful, which is why I added it here too.

The _TEMP variables were created because of how make on 9 and before expanded the .for loop variable. It did not create a real variable, it only substituted the value wherever the variable was found.

Mk/bsd.sites.mk

425–431

Not sure _GH_VAR is needed.

​.  for _GH_V in GH_ACCOUNT GH_PROJECT GH_TAGNAME GH_SUBDIR

428

This is badly named, it can contain many groups.

Also, I am not sure moving the comma split up here is a good idea.

[ultima]

Got it, I think this will be satisfactory. _GH_V is for github variables, _V_EX is variable expanded, I could add _GH_V_EX if you think that would make more sense.

[mat]

could you make the .for variables lowercase? We usually do that, it helps differentiating local variables with other ones.

[ultima]

• Changged fors to lowercase to follow styling
• ln --> ${LN}

[ultima]

In D12125#251754, @mat wrote:

could you make the .for variables lowercase? We usually do that, it helps differentiating local variables with other ones.

Thought I updated this the other day but I guess I forgot to submit.

[mat]

The code looks ok. Now you can request the exp-run :-)

Mk/bsd.sites.mk

514

While this fix is right, please commit it separately. (like, right now is good.)

[lifanov]

Mk/bsd.sites.mk

514

I approve this specific change for a commit.

[ultima]

Removed ln change

[ultima]

Mk/bsd.sites.mk

394–396

I just noticed the _t_tmp could also be removed, should I add this to the patch or should this be separate as well?

[matthew]

Did the exp-run happen and if so, what was the outcome?

[ultima]

In D12125#253302, @matthew wrote:

Did the exp-run happen and if so, what was the outcome?

If it has I am not aware of it. No replies from the bugzilla PR.

[matthew]

I saw Antoine said the Exp-run looked fine, so I'm happy to approve this -- but you still need approval from
mat or other members of portmgr.

[lifanov]

You have my approval to commit once you get approval from portmgr.

[mat]

[ultima]

In D12125#253544, @mat wrote:

This photo makes me very confident. >.<