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.

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

fix build with with GO111MODULE=off #10010

Merged

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented Apr 13, 2021

Fix build with GO111MODULE=off

Distro builds on Fedora and Kubic projects use GO111MODULE=off
by default which are currently failing. This commit fixes it and
going forward, podman CI will also indicate failures in rpm builds.

conmon build has also been removed from podman.spec.in because the COPR
for which it was provided has been discontinued.

Fixes: #10009

Signed-off-by: Lokesh Mandvekar [email protected]

@mheon @vrothberg @rhatdan @baude @cevich PTAL

@lsm5 lsm5 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 13, 2021
@lsm5
Copy link
Member Author

lsm5 commented Apr 13, 2021

let me add a CI job for GO111MODULE=off

@lsm5 lsm5 force-pushed the GO111MODULE-OFF-fix-build branch from f29e6b4 to 9e9d737 Compare April 13, 2021 13:58
@lsm5 lsm5 removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 13, 2021
@lsm5 lsm5 force-pushed the GO111MODULE-OFF-fix-build branch from 9e9d737 to 03535db Compare April 13, 2021 13:59
@lsm5 lsm5 requested a review from rhatdan April 13, 2021 14:00
@lsm5 lsm5 force-pushed the GO111MODULE-OFF-fix-build branch from 03535db to c73d233 Compare April 13, 2021 14:00
@lsm5 lsm5 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 13, 2021
@lsm5 lsm5 force-pushed the GO111MODULE-OFF-fix-build branch 2 times, most recently from 8ebcba5 to c7d79a6 Compare April 13, 2021 14:31
@mheon
Copy link
Member

mheon commented Apr 13, 2021

This seems OK, LGTM

@lsm5 lsm5 force-pushed the GO111MODULE-OFF-fix-build branch from c7d79a6 to a8ec7d2 Compare April 13, 2021 14:43
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@vrothberg
Copy link
Member

/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 13, 2021
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2021
@lsm5 lsm5 force-pushed the GO111MODULE-OFF-fix-build branch from a8ec7d2 to 34e7bff Compare April 13, 2021 15:06
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2021
@lsm5
Copy link
Member Author

lsm5 commented Apr 13, 2021

Let's hold off on lgtm until we have a green CI.

@lsm5 lsm5 force-pushed the GO111MODULE-OFF-fix-build branch from 34e7bff to e6f51d4 Compare April 13, 2021 15:19
@cevich
Copy link
Member

cevich commented Apr 13, 2021

@lsm5 I'm curious why this is needed. My understanding is GO111MODULE=off is only necessary when using really, really, old versions of golang, no?


CI test-job wise, this sounds like something ripe for inclusion in alt_build_task. This is a kind of catch-all for "Confirm building works in *" scenarios. To add something new, just toss in a new matrix item to that task with a distinct $ALT_NAME value. Then pick that up with a new entry in the runner script case block.

@lsm5 lsm5 force-pushed the GO111MODULE-OFF-fix-build branch from e6f51d4 to 858f83d Compare April 13, 2021 15:29
@lsm5
Copy link
Member Author

lsm5 commented Apr 13, 2021

@lsm5 I'm curious why this is needed. My understanding is GO111MODULE=off is only necessary when using really, really, old versions of golang, no?

hmm, I never bothered to check honestly but I could dig in if necessary on that. FWIW, the default fedora %gobuild definition is:

$ rpm --eval %gobuild

  # https://bugzilla.redhat.com/show_bug.cgi?id=995136#c12
     GO111MODULE=off \
  go build -buildmode pie -compiler gc -tags="rpm_crashtraceback ${BUILDTAGS:-}" -ldflags "${LDFLAGS:-} -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \n') -extldflags '-Wl,-z,relro -Wl,--as-needed  -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld  '" -a -v -x ;

so I'm guessing there hasn't been any change on newer versions.

CI test-job wise, this sounds like something ripe for inclusion in alt_build_task. This is a kind of catch-all for "Confirm building works in *" scenarios. To add something new, just toss in a new matrix item to that task with a distinct $ALT_NAME value. Then pick that up with a new entry in the runner script case block.

So, given that the spec file is only used for checking if rpm builds fine, I changed the spec file directly without a specific CI job. Things should hopefully be changing very soon when we have the packit integration in place.

@lsm5 lsm5 force-pushed the GO111MODULE-OFF-fix-build branch from 858f83d to 84b3dc7 Compare April 13, 2021 15:48
@lsm5
Copy link
Member Author

lsm5 commented Apr 13, 2021

@cevich oh also, if we don't have GO111MODULE=off it ends up fetching all the vendored dependencies which is a no-go for non-networked koji buiilds.

@cevich
Copy link
Member

cevich commented Apr 13, 2021

https://bugzilla.redhat.com/show_bug.cgi?id=995136

That BZ is from 2016.

if we don't have GO111MODULE=off it ends up fetching all the vendored dependencies which is a no-go for non-networked koji buiilds.

Oh...right, yeah, golang really wants to be connected to the internet 😞

@cevich
Copy link
Member

cevich commented Apr 13, 2021

Things should hopefully be changing very soon when we have the packit integration in place.

Oh okay, yeah, feel free to replace that RPM-build job then. My understand is it's mainly there for package release benefit - noticing problems early. So if there's a better way to do that, go for it.

@lsm5 lsm5 force-pushed the GO111MODULE-OFF-fix-build branch 2 times, most recently from 2c7dbca to 1d92982 Compare April 13, 2021 17:09
@lsm5 lsm5 force-pushed the GO111MODULE-OFF-fix-build branch 2 times, most recently from 1c79a5a to f19a891 Compare April 14, 2021 18:37
@lsm5 lsm5 requested a review from vrothberg April 14, 2021 19:40
@lsm5
Copy link
Member Author

lsm5 commented Apr 14, 2021

ready for lgtm

@mheon
Copy link
Member

mheon commented Apr 15, 2021

I think the comment from @rhatdan is addressed
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2021
@lsm5
Copy link
Member Author

lsm5 commented Apr 15, 2021

we also need a /hold cancel i guess.

@rhatdan
Copy link
Member

rhatdan commented Apr 15, 2021

/approve

@openshift-ci-robot
Copy link
Collaborator

[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:

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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2021
@rhatdan
Copy link
Member

rhatdan commented Apr 15, 2021

I thought those were there to indicate where we copied the code from . Perhaps change the import to copied from?

@lsm5
Copy link
Member Author

lsm5 commented Apr 15, 2021

I thought those were there to indicate where we copied the code from . Perhaps change the import to copied from?

@rhatdan I can give that a try.

Distro builds on Fedora and Kubic projects use GO111MODULE=off
by default which are currently failing. This commit fixes it and
going forward, podman CI will also indicate failures in rpm builds.

The additional LDFLAGS have been removed  from the spec file
which is not ideal. But, currently we only use the spec file
to check if the rpm builds fine. We can fix the LDFLAGS in a
later commit when we're working on packit integration.

conmon build has also been removed from podman.spec.in because the COPR
for which it was provided has been discontinued.

[NO TESTS NEEDED]

Fixes: containers#10009

Signed-off-by: Lokesh Mandvekar <[email protected]>
@lsm5 lsm5 force-pushed the GO111MODULE-OFF-fix-build branch from f19a891 to 501b475 Compare April 15, 2021 18:25
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2021
@lsm5
Copy link
Member Author

lsm5 commented Apr 15, 2021

@rhatdan added a comment on top of the package line. I wish golang didn't act funny with comments.

@rhatdan
Copy link
Member

rhatdan commented Apr 15, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2021
@lsm5
Copy link
Member Author

lsm5 commented Apr 15, 2021

anybody know how to rerun the rdoproject.org/github-check job? It doesn't fail locally for me on centos8

@edsantiago
Copy link
Member

rdoproject always fails. We don't care: it doesn't block merging.

@lsm5
Copy link
Member Author

lsm5 commented Apr 15, 2021

rdoproject always fails. We don't care: it doesn't block merging.

thanks @edsantiago . If someone could please /hold cancel, this should be good for merge.

@lsm5
Copy link
Member Author

lsm5 commented Apr 15, 2021

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2021
@openshift-merge-robot openshift-merge-robot merged commit 373f15f into containers:master Apr 15, 2021
@lsm5 lsm5 deleted the GO111MODULE-OFF-fix-build branch April 15, 2021 20:03
@github-actions 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 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GO111MODULE=off go build failure
8 participants