Conversation
Makefile
Outdated
There was a problem hiding this comment.
Do we want to hard-code varlink here? I'd have expected the default to depend on whether or not some system library or similar was available, and that we'd have a hack/ script to detect that dependency.
Makefile
Outdated
There was a problem hiding this comment.
You have inconsistent indenting with spaces here and a tab two lines above.
There was a problem hiding this comment.
not sure what gives there; should be fixed now
Makefile
Outdated
There was a problem hiding this comment.
Are you intentionally removing this blank line? I think the blank line helps to more clearly associate the following comment with its block of Make code.
Makefile
Outdated
There was a problem hiding this comment.
Instead of gutting this recipe when WANTVARLINK is false, I'd rather adjust the consumers to remove the cmd/podman/varlink/ioprojectatomicpodman.go dependency if WANTVARLINK is false.
For example, we could have a VARLINK_GO_DEPS variable (or some such) that we setup where you're currently setting up WANTVARLINK, and then use VARLINK_GO_DEPS where we currently use cmd/podman/varlink/ioprojectatomicpodman.go as a dependency.
There was a problem hiding this comment.
can you provide an example? i like where you are headed but my Make-fu sucks ... alternatively and perhaps even better, how would you feel about pushing this PR through (it does in fact work) and then having you make a superior PR that contains some of the great stuff you put here.
There was a problem hiding this comment.
can you provide an example?
I've filed a squash commit as baude#1. Squash that commit in if you like it, and feel free to mess with the commit message suggestions.
cmd/podman/cmd_varlink.go
Outdated
There was a problem hiding this comment.
This approach won't scale well with additional optional commands behind different tags. Perhaps we could have a varlinkCommand pointer that we setup in varlink.go in the varlink case and set nil in varlink_dummy.go (or some such) in the !varlink case?
There was a problem hiding this comment.
This approach won't scale well with additional optional commands behind different tags.
I've filed baude#2 with a squash commit for this suggestion.
some platforms and operating systems do not have varlink. in those cases, we need to be able to turn off enablement of varlink in podman. this can now be done with BUILDTAGS passed to the build though perhaps in the future will be better. the default is to build with varlink Signed-off-by: baude <[email protected]>
The API.md and cmd/podman/varlink/ioprojectatomicpodman.go targets will continue to work regardless of the presence (or not) of 'varlink' is in BUILDTAGS. However, cmd/podman/varlink/ioprojectatomicpodman.go is now only required by the podman target when BUILDTAGS contains 'varlink'. API.md had also been an podman dependency since 5b2627d (Makefile: Drop find-godeps.sh for podman target, 2018-05-15, containers#776) when I expanded varlink_api_generate. It had been an indirect podman dependency (via varlink_api_generate) since 2526355 (Generate varlink API documentation automatically, 2018-05-07, containers#734). But the podman executable obviously doesn't depend on the Markdown file, so I'm removing that dependency here. Signed-off-by: baude <[email protected]>
The command-pointer approach will scale well if/when we add additional optional commands behind their own build tags, because those tags won't all be competing for the same getOptionalCommands namespace. Signed-off-by: W. Trevor King <[email protected]>
|
comments from @wking addressed...ready for merge please |
|
LGTM |
TomSweeneyRedHat
left a comment
TomSweeneyRedHat
left a comment
There was a problem hiding this comment.
LGTM, @wking 's concerns can be addressed in a follow up.
|
📌 Commit 6a86956 has been approved by |
|
⚡ Test exempted: merge already tested. |
|
Looks like @rh-atomic-bot squashed these for us. |
github-actions
bot
added
the
locked - please file new issue/PR
5 participants
some platforms and operating systems do not have varlink. in those cases,
we need to be able to turn off enablement of varlink in podman. this can now
be done with BUILDTAGS passed to the build though perhaps in the future
will be better.
the default is to build with varlink
Signed-off-by: baude [email protected]