redhat-developer / kam

GitOps Application Manager: An opinionated CLI that generates the Kubernetes resources for managing your Tekton-based CI manifests, ArgoCD-based CD manifests and Application manifests in Git.
Apache License 2.0
145 stars 83 forks source link

chore: add flags for external linker to satisfy RPMdiff bind-now test #298

Closed jaideepr97 closed 2 years ago

jaideepr97 commented 2 years ago

What type of PR is this?

Uncomment only one /kind line, and delete the rest. For example, > /kind bug would simply become: /kind bug

/kind bug /kind cleanup /kind failing-test /kind enhancement /kind documentation /kind code-refactoring

What does this PR do / why we need it: it seems like kam pulls in the net package as a dependency which I suspect is causing cgo to be used for compilation indirectly and maybe causing kam to be dynamically linked. (This might also be putting the KAM binary in a position to benefit from supplying the -buildmode=pie flag) The RPMDiff test in cpaas currently throws an error because we don't add the -Wl,-z,now flags for external linker while building the kam binary This PR fixes that

Have you updated the necessary documentation?

Which issue(s) this PR fixes:

Fixes https://issues.redhat.com/browse/GITOPS-1714 partially

How to test changes / Special notes to the reviewer:

jannfis commented 2 years ago

/lgtm

jannfis commented 2 years ago

I was wondering tho, if we could just not set CGO_ENABLED=0 for the build?

jaideepr97 commented 2 years ago

@jannfis Would that be a problem if we are using any packages that absolutely require cgo? My understanding is the net package that is pulled in by one of kam's dependencies maybe one such package but I don't have an in-depth understanding of this

jaideepr97 commented 2 years ago

I was thinking we could just try to build a new RPM with these extdflags in place, supply the buildmode=pie flag and then find a way to run the RPMDiff test again just on this newly built RPM to see if that addresses all the concerns

jannfis commented 2 years ago

Would that be a problem if we are using any packages that absolutely require cgo

Yes, probably

My understanding is the net package that is pulled in by one of kam's dependencies maybe one such package

I think it used to be due to the resolver, but meanwhile, Go also provides a Go-native resolver to be used with CGO_ENABLED=0 (https://pkg.go.dev/net#hdr-Name_Resolution)

kam compiles, links and runs fine when built with CGO_ENABLED=0. However, it produces a static binary. What's funny is, that it's even a little smaller than the dynamically one, which was built using cgo:

-rwxr-xr-x 1 foo foo 45056000 Mar  3 21:54 dist/kam_linux_amd64
-rwxr-xr-x 1 foo foo 45109248 Mar  3 21:53 dist/kam_linux_amd64-cgo

Anyway, I think for now, we should go for the linker flags.

jannfis commented 2 years ago

/retest

wtam2018 commented 2 years ago

/approve

openshift-ci[bot] commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wtam2018

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](https://github.com/redhat-developer/kam/blob/master/OWNERS)~~ [wtam2018] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment