enable building without `varlink` tag #6353

lsm5 · 2020-05-22T17:57:12Z

default build without varlink tag

Issue gh#6286 was already fixed in a prior commit but the Makefile still
ran some varlink steps by default.

This commit makes any varlink build steps dependent on the varlink
build tag and also makes the contrib rpm spec file independent of
varlink.

Signed-off-by: Lokesh Mandvekar lsm5@fedoraproject.org

rh-atomic-bot · 2020-05-22T18:14:50Z

☔ The latest upstream changes (presumably #6341) made this pull request unmergeable. Please resolve the merge conflicts.

edsantiago

Really nice work. Just one question.

edsantiago · 2020-05-28T18:13:49Z

Makefile

 .PHONY: varlink_generate
+ifneq (or $(findstring varlink,$(BUILDTAGS)),$(findstring varlink,$(BUILD_TAGS)))


I'm super rusty on Makefile magic, but this doesn't look right to me. Are you looking for varlink in either of those two variables? If so would this work?

ifneq (,$(findstring varlink,$(BUILDTAGS) $(BUILD_TAGS)))

yes, I'm looking for varlink in those two. I'll give that a try, thanks!

there seems to be some integration test issues that I'm looking into as well.

make lint fails with:

# $BUILD_TAGS variable is used in hack/golangci-lint.sh .PHONY: varlink_generate -ifneq (or $(findstring varlink,$(BUILDTAGS)),$(findstring varlink,$(BUILD_TAGS))) +ifneq (,$(findstring varlink,$(BUILD_TAGS) $(BUILDTAGS))) varlink_generate: .gopathok pkg/varlink/iopodman.go ## Generate varlink else varlink_generate:

Probably some magic syntax detail I'm missing.

Well, does the or format work? If you run make with varlink in either variable, does it trigger the desired varlink_generate?

The or format works well for make lint. I don't need to specify varlink buildtag for tunnel and abi as it will automatically use it from the golangci-lint.sh file. Both make and `make BUILDTAGS='varlink' work correctly as well.

OK - sorry to have muddled things. LGTM, thanks.

btw, what you suggested I think should work as well, which is what seems to be suggested in the official make doc, but for whatever reason it doesn't seem to work in this case. Maybe more eyes would help..

lsm5 · 2020-05-28T18:51:05Z

Btw, I guess in the default case, it will now skip varlink build and tests. Let me know if CI needs to continue testing for the varlink case as well.

edsantiago · 2020-05-28T19:22:23Z

Hmmm. CI is not happy, and it doesn't look (at first glance) like a flake:

[+0179s] # github.com/containers/libpod/test/endpoint [github.com/containers/libpod/test/endpoint.test]
[+0179s] test/endpoint/config.go:10: undefined: ALPINE
[+0179s] test/endpoint/setup.go:19: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:102: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:120: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:129: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:136: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:144: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:176: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:185: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:195: undefined: EndpointTestIntegration
[+0179s] test/endpoint/config.go:10: too many errors

lsm5 · 2020-05-28T19:50:03Z

Hmmm. CI is not happy, and it doesn't look (at first glance) like a flake:

[+0179s] # github.com/containers/libpod/test/endpoint [github.com/containers/libpod/test/endpoint.test]
[+0179s] test/endpoint/config.go:10: undefined: ALPINE
[+0179s] test/endpoint/setup.go:19: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:102: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:120: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:129: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:136: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:144: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:176: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:185: undefined: EndpointTestIntegration
[+0179s] test/endpoint/setup.go:195: undefined: EndpointTestIntegration
[+0179s] test/endpoint/config.go:10: too many errors

err, yes that's a consequence of me wrapping test/endpoint/endpoint.go inside varlink buildtag.

Could someone please let me know if test/endpoint is a varlink-specific thing? Or if I should split out the varlink specific stuff in separate files?

edsantiago · 2020-05-28T20:55:48Z

if test/endpoint is a varlink-specific thing?

There is no way I can answer that but I'm going to jump in anyway: a quick ack -i varlink test/endpoint/ produces so many hits that I have to assume it is.

lsm5 · 2020-05-29T14:19:23Z

I added conditional to skip endpoint tests if no varlink buildtag found.

Issue gh#6286 was already fixed in a prior commit but the Makefile still ran some varlink steps by default. This commit makes any varlink build steps dependent on the varlink build tag and also makes the contrib rpm spec file independent of varlink. Endpoint tests will be run only if BUILDTAGS contains varlink. Signed-off-by: Lokesh Mandvekar <lsm5@fedoraproject.org>

edsantiago · 2020-06-01T12:36:04Z

LGTM

rhatdan · 2020-06-01T12:44:42Z

It would be nice to split out the varlink code into a separate Makefile.Varlink. And then optionally include it if necessary. That might cleanup the Makefile.

/lgtm

rhatdan · 2020-06-01T12:44:51Z

/approve

openshift-ci-robot · 2020-06-01T12:44:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lsm5, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot requested review from giuseppe and rhatdan May 22, 2020 17:57

lsm5 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 22, 2020

lsm5 linked an issue May 22, 2020 that may be closed by this pull request

make error - undefined: varlinkapi.New #6286

Closed

lsm5 force-pushed the build-without-varlink branch 3 times, most recently from e4e49cb to 312d2b6 Compare May 24, 2020 09:44

lsm5 changed the title ~~[do-not-merge] WIP - enable building without varlink tag~~ enable building without varlink tag May 24, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 24, 2020

lsm5 force-pushed the build-without-varlink branch 5 times, most recently from 06f289d to de29580 Compare May 24, 2020 13:03

lsm5 changed the title ~~enable building without varlink tag~~ [DO NOT MERGE] enable building without varlink tag May 24, 2020

lsm5 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 24, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 24, 2020

lsm5 force-pushed the build-without-varlink branch 4 times, most recently from cf63127 to 1f802a2 Compare May 28, 2020 17:56

lsm5 changed the title ~~[DO NOT MERGE] enable building without varlink tag~~ enable building without varlink tag May 28, 2020

lsm5 requested review from edsantiago, vrothberg and jwhonce May 28, 2020 17:58

edsantiago reviewed May 28, 2020

View reviewed changes

lsm5 force-pushed the build-without-varlink branch from 1f802a2 to 12302dc Compare May 28, 2020 18:44

lsm5 force-pushed the build-without-varlink branch 4 times, most recently from 68cfda3 to e385635 Compare May 29, 2020 14:18

lsm5 force-pushed the build-without-varlink branch 2 times, most recently from 554652b to cde93cd Compare May 29, 2020 14:44

lsm5 force-pushed the build-without-varlink branch from cde93cd to 8a914e8 Compare May 29, 2020 18:38

openshift-ci-robot assigned rhatdan Jun 1, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 1, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2020

openshift-merge-robot merged commit 22713d6 into containers:master Jun 1, 2020

edsantiago mentioned this pull request Jun 1, 2020

rawhide: podman build is completely broken #6451

Closed

lsm5 deleted the build-without-varlink branch June 1, 2020 19:56

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable building without `varlink` tag #6353

enable building without `varlink` tag #6353

lsm5 commented May 22, 2020 •

edited

rh-atomic-bot commented May 22, 2020

edsantiago left a comment

edsantiago May 28, 2020

lsm5 May 28, 2020

lsm5 May 28, 2020

lsm5 May 28, 2020

edsantiago May 28, 2020

lsm5 May 28, 2020

edsantiago May 28, 2020

lsm5 May 28, 2020

lsm5 commented May 28, 2020

edsantiago commented May 28, 2020

lsm5 commented May 28, 2020

edsantiago commented May 28, 2020

lsm5 commented May 29, 2020

edsantiago commented Jun 1, 2020

rhatdan commented Jun 1, 2020

rhatdan commented Jun 1, 2020

openshift-ci-robot commented Jun 1, 2020

		.PHONY: varlink_generate
		ifneq (or $(findstring varlink,$(BUILDTAGS)),$(findstring varlink,$(BUILD_TAGS)))

enable building without varlink tag #6353

enable building without varlink tag #6353

Conversation

lsm5 commented May 22, 2020 • edited

rh-atomic-bot commented May 22, 2020

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsm5 commented May 28, 2020

edsantiago commented May 28, 2020

lsm5 commented May 28, 2020

edsantiago commented May 28, 2020

lsm5 commented May 29, 2020

edsantiago commented Jun 1, 2020

rhatdan commented Jun 1, 2020

rhatdan commented Jun 1, 2020

openshift-ci-robot commented Jun 1, 2020

enable building without `varlink` tag #6353

enable building without `varlink` tag #6353

lsm5 commented May 22, 2020 •

edited