USE_GITHUB cleanup
ClosedPublic
Actions

Authored by bdrewery on Sep 9 2014, 7:45 PM.

Details

Reviewers

swills
AMDmi3
antoine
bapt
bdrewery
mat

Group Reviewers

portmgr

Summary

Cleanup GITHUB master sites.

While we currently have 3 github mirrors:

MASTER_SITE_GITHUB+= https://codeload.github.com/%SUBDIR% \

http://codeload.github.com/%SUBDIR%

MASTER_SITE_GITHUB_CLOUD+= http://cloud.github.com/downloads/%SUBDIR%

only first one works. Second one doesn't reply and clound reply with 403.

Leave only first one.

Use new GITHUB fetching scheme.

In addition to the scheme which we currently use:
https://codeload.github.com/Bertram25/ValyriaTear/legacy.tar.gz/1.0.0?dummy=/valyriatear-1.0.0.tar.gz

GitHub allows other one:
https://codeload.github.com/Bertram25/ValyriaTear/tar.gz/1.0.0?dummy=/valyriatear-1.0.0.tar.gz

The difference is that root directory of tarball is named
${GH_ACCOUNT}-${GH_PROJECT}-${GH_COMMIT} in the first case and
${GH_PROJECT}-${GH_TAGNAME} in the second one.

The second scheme is more convenient, as it doesn't require maintainer to retrieve and specify commit hash.

I propose to make use of the new scheme while leaving support for the old one. To select scheme, GH_COMMIT is checked. If it's defined, the old scheme is used. If not, the new one. Since GH_COMMIT is mandatory in the old scheme, this change is fully backwards compatible. Now, ports may just omit GH_COMMIT to use the new scheme.

Diff Detail

Repository

rP FreeBSD ports repository

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

AMDmi3 updated this revision to Diff 1425.Sep 9 2014, 7:45 PM

AMDmi3 retitled this revision from to USE_GITHUB cleanup.

AMDmi3 updated this object.

AMDmi3 edited the test plan for this revision. (Show Details)

AMDmi3 added reviewers: bapt, bdrewery.

Herald added a reviewer: portmgr. · View Herald TranscriptSep 9 2014, 7:45 PM

mmmm GHC should still be kept in my opinion
See converters/lua-iconv for instance

antoine requested changes to this revision.Sep 9 2014, 8:01 PM

antoine added a reviewer: antoine.

This revision now requires changes to proceed.Sep 9 2014, 8:01 PM

This is close to where I was going as well. Please give me some time to review this first.

In D748#4, @antoine wrote:

mmmm GHC should still be kept in my opinion
See converters/lua-iconv for instance

Agreed, GITHUB_CLOUD is different than USE_GITHUB.

Do not remove GITHUB_CLOUD

antoine added inline comments.Sep 9 2014, 8:45 PM

Mk/bsd.sites.mk
529	With this, audio/lua51-mpd fails to fetch (it uses GHC but it's not explicit)

There's bunch of ports which do not set GH_COMMIT. I guess I need to process them first.

I've started testing this, should have more feedback soon.

@bdrewery any news on this?

I have some other changes related to GITHUB stuff, see D1340. Perhaps I could combine them, but that might require additional testing.

Mk/bsd.port.mk
1537	Perhaps this should also check that GH_TAGNAME is defined?

Sorry this fell off my radar. Will test it tomorrow.

antoine added inline comments.Dec 19 2014, 11:41 PM

Mk/bsd.sites.mk
529	my comment was wrong, audia/lua51-mpd fails to fetch because it uses GH legacy but doesn't have a GH_COMMIT

I believe i fixed all the ports using legacy github that were not setting GH_COMMIT, so the change should be safe now (I need to test a bit)

antoine added inline comments.Dec 20 2014, 7:22 PM

Mk/bsd.sites.mk
533	I think I would change this condition to: .if !defined(MASTER_SITES) \|\| !${MASTER_SITES:MGH} && !${MASTER_SITES:MGHC} && !${MASTER_SITES:MGHR} to not add a wrong master site to ports using GHC or GHR.

swills added inline comments.Dec 20 2014, 10:01 PM

Mk/bsd.sites.mk
533	Correct me if I'm wrong, but I think that's the same logic I added in rP375010 (D1340)

antoine added inline comments.Dec 20 2014, 10:03 PM

Mk/bsd.sites.mk
533	The result is not the same :-)

swills added inline comments.Dec 20 2014, 10:05 PM

Mk/bsd.port.mk
1540	GH_TAGNAME may not be defined if using a release, so a check that that's defined and using only ${GH_PROJECT}-${PORTVERSION} would be good.

swills added inline comments.Dec 20 2014, 10:38 PM

Mk/bsd.port.mk
1540	Nevermind, I was misunderstanding something.

FYI I have an updated patch at: https://people.freebsd.org/~antoine/ports/D748-updated.diff

Taking this over since it has sat for so long.

Updating with Antoine's updated patch, plus fixing a few rejects that crept in

Change to bitcoin no longer needed

Macro shipit:

I like it! Thanks for doing this.

Takeover

bdrewery accepted this revision.Mar 19 2015, 4:51 PM

bdrewery added a reviewer: bdrewery.

antoine accepted this revision.Mar 19 2015, 4:55 PM

antoine edited edge metadata.

This revision is now accepted and ready to land.Mar 19 2015, 4:55 PM

Committed in rP381618

Just curious, why was internal github URL used (https://codeload.github.com/${ACCOUNT}/${PROJECT}/tar.gz/${TAG}) instead of public one (https://github.com/${ACCOUNT}/${PROJECT}/archive/${TAG}.tar.gz) ?

See https://developer.github.com/v3/repos/contents/#get-archive-link

It turns out that we now allow redirects just fine, so I see no point for not using official public URL. The change should be trivial and not affect any ports.

Also, do we really need ${GH_ACCOUNT} at the beginning of ${DISTNAME}? This makes port and distfile matching harder for user. I'd go with ${PORTNAME}-${PORTVERSION}-${GH_ACCOUNT}_${GH_TAGNAME} or similar.

In D748#51, @AMDmi3 wrote:

Just curious, why was internal github URL used (https://codeload.github.com/${ACCOUNT}/${PROJECT}/tar.gz/${TAG}) instead of public one (https://github.com/${ACCOUNT}/${PROJECT}/archive/${TAG}.tar.gz) ?

See https://developer.github.com/v3/repos/contents/#get-archive-link

It turns out that we now allow redirects just fine, so I see no point for not using official public URL. The change should be trivial and not affect any ports.

Also, do we really need ${GH_ACCOUNT} at the beginning of ${DISTNAME}? This makes port and distfile matching harder for user. I'd go with ${PORTNAME}-${PORTVERSION}-${GH_ACCOUNT}_${GH_TAGNAME} or similar.

Multiple reasons. 1. The API link redirects to the LEGACY url. Which invalidates your entire change pointless. 2. Using the main site URL just redirects to codeload anyhow but it ends up expanding whatever tag/hash you give until the full name. So we can no longer use a 4 char hash or 8 char hash, it forces the full hash length. It is an inconvenience for porters and would not allow a simple old->new USE_GITHUB transition as porters would have to go lookup the full hash, which Github tends to hide from you. The API URL issue is discussed in Bug 194898.