Page MenuHomeFreeBSD

USES=go ports: cleanup, make use of the new GO_TARGET tuple syntax
ClosedPublic

Authored by dmgk on Sep 19 2019, 5:02 PM.

Details

Summary

Cleanup USES=go ports:

  • Remove custom build/install targets left in place after r505321
  • Switch to the new GO_TARGET tuple syntax introduced in r512001
Test Plan

poudriere bulk: OK (113a, 120a)

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

dmgk created this revision.Sep 19 2019, 5:02 PM
tz added a comment.Sep 19 2019, 8:48 PM

Since i am very unfamiliar with the go ports, i have problems to understand this diff.
In devel/awless only do-build and do-install is removed, but i do not see any usage of GO_TARGET. Same for some other ports.

Is there any reason why removing the unneeded build-steps and the switch to GO_TARGET are in the same commit? With my limited understanding i would say this are two different changes with two different reasons to do so. So, can you explain me what you want to archive and why this is a single commit? This should be also reflected later in the commit message, because i am not the only one with limited go experience ;)

dmgk added a comment.Sep 19 2019, 9:34 PM
In D21721#473870, @tz wrote:

Since i am very unfamiliar with the go ports, i have problems to understand this diff.
In devel/awless only do-build and do-install is removed, but i do not see any usage of GO_TARGET. Same for some other ports.

r505321 converted all ports to USES=go which already provides do-build and do-install targets. During that conversion, ports that were defining their own build/install targets were left alone to be cleaned up later. This commit does this cleanup for the ports that I maintain.

Is there any reason why removing the unneeded build-steps and the switch to GO_TARGET are in the same commit?

Removing targets doesn't remove any build steps, just switches the port to use default targets that go.mk provides.

GO_TARGET is used by go.mk in do-build and do-install, it doesn't make sense to set it with custom targets and removing custom targets w/o setting GO_TARGET would break the build.

dmgk edited the summary of this revision. (Show Details)Sep 19 2019, 10:49 PM

Extended commit message a bit.

araujo accepted this revision.Sep 20 2019, 2:05 AM
In D21721#473887, @dmgk wrote:
In D21721#473870, @tz wrote:

Since i am very unfamiliar with the go ports, i have problems to understand this diff.
In devel/awless only do-build and do-install is removed, but i do not see any usage of GO_TARGET. Same for some other ports.

r505321 converted all ports to USES=go which already provides do-build and do-install targets. During that conversion, ports that were defining their own build/install targets were left alone to be cleaned up later. This commit does this cleanup for the ports that I maintain.

As you are working on this already for a while, how about ports that you are not a maintainer? Maybe an exp-run following with a portmgr@ approval would be interesting to clean up the rest of the ports.

What do you think? Is it something worth to do?

Is there any reason why removing the unneeded build-steps and the switch to GO_TARGET are in the same commit?

Removing targets doesn't remove any build steps, just switches the port to use default targets that go.mk provides.
GO_TARGET is used by go.mk in do-build and do-install, it doesn't make sense to set it with custom targets and removing custom targets w/o setting GO_TARGET would break the build.

This revision is now accepted and ready to land.Sep 20 2019, 2:05 AM
tz added a comment.Sep 20 2019, 9:05 AM
In D21721#473887, @dmgk wrote:
In D21721#473870, @tz wrote:

Since i am very unfamiliar with the go ports, i have problems to understand this diff.
In devel/awless only do-build and do-install is removed, but i do not see any usage of GO_TARGET. Same for some other ports.

r505321 converted all ports to USES=go which already provides do-build and do-install targets. During that conversion, ports that were defining their own build/install targets were left alone to be cleaned up later. This commit does this cleanup for the ports that I maintain.

Is there any reason why removing the unneeded build-steps and the switch to GO_TARGET are in the same commit?

Removing targets doesn't remove any build steps, just switches the port to use default targets that go.mk provides.
GO_TARGET is used by go.mk in do-build and do-install, it doesn't make sense to set it with custom targets and removing custom targets w/o setting GO_TARGET would break the build.

That is a great explanation. Without any knowledge of the GO ports i was able to understand the changes. Thank you!
Marcelo already approved it. Please go ahead :)

This revision was automatically updated to reflect the committed changes.
dmgk added a comment.Sep 20 2019, 11:43 AM

As you are working on this already for a while, how about ports that you are not a maintainer? Maybe an exp-run following with a portmgr@ approval would be interesting to clean up the rest of the ports.

That was my plan actually, I was just not sure what would be the best way to get something like that reviewed/committed. The complete diff is about 250KB, I was thinking about maybe breaking it to 3-4 parts for easier review. Would that make sense or it would be better to post it in a single review?

In D21721#474028, @dmgk wrote:

As you are working on this already for a while, how about ports that you are not a maintainer? Maybe an exp-run following with a portmgr@ approval would be interesting to clean up the rest of the ports.

That was my plan actually, I was just not sure what would be the best way to get something like that reviewed/committed. The complete diff is about 250KB, I was thinking about maybe breaking it to 3-4 parts for easier review. Would that make sense or it would be better to post it in a single review?

Maybe you can break it down by port category, or by combining group of categories in case you have few ports in some of them. So, makes sense break it in 3-4 parts as you mentioned!

dmgk added a comment.Sep 20 2019, 1:10 PM

Maybe you can break it down by port category, or by combining group of categories in case you have few ports in some of them. So, makes sense break it in 3-4 parts as you mentioned!

Will do that, thanks.

tz added a comment.Sep 20 2019, 7:27 PM
In D21721#474028, @dmgk wrote:

As you are working on this already for a while, how about ports that you are not a maintainer? Maybe an exp-run following with a portmgr@ approval would be interesting to clean up the rest of the ports.

That was my plan actually, I was just not sure what would be the best way to get something like that reviewed/committed. The complete diff is about 250KB, I was thinking about maybe breaking it to 3-4 parts for easier review. Would that make sense or it would be better to post it in a single review?

It depends on the diff. If it is the same line changed over 1.000 ports - that is fine as a single review. If we need to check various things, its better to break it down. Beside the good suggestion "by category" you can also use "by type of change" or "by not-maintained and maintained" and any combination of this.

dmgk added a comment.Sep 21 2019, 12:35 PM
In D21721#474176, @tz wrote:

It depends on the diff. If it is the same line changed over 1.000 ports - that is fine as a single review. If we need to check various things, its better to break it down. Beside the good suggestion "by category" you can also use "by type of change" or "by not-maintained and maintained" and any combination of this.

The diff I plan to submit has the same type of changes as in this one - removing custom targets, adding GO_TARGET and GO_PKGNAME instead, e.g. cleanup and finalizing migration to go.mk. I think splitting it to a few parts by category is a good plan because all affected ports have the same scope of changes.