Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix native go build #5799

Merged
merged 5 commits into from
Jun 22, 2021
Merged

Fix native go build #5799

merged 5 commits into from
Jun 22, 2021

Conversation

vasiliy-ul
Copy link
Contributor

@vasiliy-ul vasiliy-ul commented Jun 10, 2021 •
edited
Loading

What this PR does / why we need it:

The PR includes several fixes for native Go build (without Bazel):

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #5602

Special notes for your reviewer:

Release note:

NONE

vasiliy-ul added 3 commits June 9, 2021 10:08
@vasiliy-ul
Drop deprecated go build flag '-i'
710a6ef 
Suppressing Go 1.16 warning: 'go build: -i flag is deprecated'

Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
@vasiliy-ul
Fix inconsistent path for node-labeller.sh
650b1ab 
The installation path for node-labeller.sh is /usr/bin. Dont call it
from /bin assuming the directories are symlinked.

Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
@vasiliy-ul
Enable Linux-only native Go builds
196e55d 
Don't build virtctl for darwin and windows if LINUX_ONLY env var is
defined.

Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/S labels Jun 10, 2021
@kubevirt-bot kubevirt-bot requested review from dhiller and phoracek June 10, 2021 06:19
@vasiliy-ul
Copy link
Contributor Author

/retest

dhiller
dhiller approved these changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@dhiller dhiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes

/lgtm

One question I have still: what is the use case for injecting a custom manifest-templator?

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2021
@vasiliy-ul
Copy link
Contributor Author

One question I have still: what is the use case for injecting a custom manifest-templator?

@dhiller , its not for a 'custom' templator. The issue for us is that build-manifests.sh triggers the build of the templator using bazel:

kubevirt/hack/build-manifests.sh

Lines 35 to 37 in 37ec453

bazel run \
--config=${HOST_ARCHITECTURE} \
//:build-manifest-templator -- ${templator}

Unfortunately bazel does not work well in our build environemnt as it tends to pull many dependencies from the web (which we do not need in general since we provide our own containers). The idea to work it around is to prebuilt the default templator using native go ./hack/build-go.sh install tools/manifest-templator and then call

MANIFEST_TEMPLATOR=./_out/cmd/manifest-templator/manifest-templator ./hack/build-manifests.sh

@rmohr
Copy link
Member

rmohr commented Jun 14, 2021

Unfortunately bazel does not work well in our build environemnt as it tends to pull many dependencies from the web (which we do not need in general since we provide our own containers)

It may probably still not be the best option for you to build, but note that offline building would be possible by pre-fetching dependencies and pointing bazel to directory with them via --distdir.

@vasiliy-ul
Copy link
Contributor Author

It may probably still not be the best option for you to build, but note that offline building would be possible by pre-fetching dependencies and pointing bazel to directory with them via --distdir.

Thanks for the suggestion. I've already looked a bit into that direction. Indeed it is possible to setup offline builds with bazel. But in fact we do not need most of the dependencies like e.g. RPMs and exteranl modules since at the end we build our own container images. We really need only the binaries and currently the easiest way to get those is by using native go build.

rmohr
rmohr requested changes Jun 15, 2021
View reviewed changes
hack/build-manifests.sh Outdated
bazel run \
--config=${HOST_ARCHITECTURE} \
//:build-manifest-templator -- ${templator}
templator=${MANIFEST_TEMPLATOR}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A clear indicator if one wants to use bazel or go would be something I would prefer. It a little bit a mix-and-match with unclear fallback paths for some users. Could you improve that a little?

Copy link
Contributor Author

@vasiliy-ul vasiliy-ul Jun 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about moving the build of manifest-templator tool to a separate make target. Or even better: include it in the bazel-build / go-build. That way hack/build-manifests.sh can only do the manifests generation. What do you think? Is there any reason to keep bazel run ... //:build-manifest-templator ... here?

@rmohr , I refactored it a bit to make the bazel / go choice more obvious. Does it now look similar to what you would expect here? Hope I understood your comment correctly...

@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2021
@vasiliy-ul
Add missing virt-chroot binary to the build list
7877a8a 
Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
@vasiliy-ul
Allow building manifest-templator with native Go
fc120d8 
Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
@vasiliy-ul vasiliy-ul force-pushed the fix-native-go-build branch from 0bb6696 to fc120d8 Compare June 16, 2021 11:32
@rmohr
Copy link
Member

rmohr commented Jun 22, 2021

/approve
/retest

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rmohr

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:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 22, 2021
@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2021
@kubevirt-bot kubevirt-bot merged commit a10478c into kubevirt:main Jun 22, 2021
@vasiliy-ul vasiliy-ul deleted the fix-native-go-build branch June 22, 2021 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmohr rmohr rmohr requested changes

@dhiller dhiller dhiller approved these changes

@phoracek phoracek Awaiting requested review from phoracek

Assignees

@dhiller dhiller

@rmohr rmohr

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent path for node-labeller.sh in virt-handler image
4 participants
@vasiliy-ul @rmohr @kubevirt-bot @dhiller