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

🐛 enable the use of CGO #246

Closed
wants to merge 1 commit into from

Conversation

elgnay
Copy link
Contributor

@elgnay elgnay commented Aug 4, 2023

Summary

Related issue(s)

Fixes #

Signed-off-by: Yang Le <yangle@redhat.com>
@openshift-ci openshift-ci bot requested review from deads2k and qiujian16 August 4, 2023 08:44
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 4, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: elgnay
Once this PR has been reviewed and has the lgtm label, please assign qiujian16 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.02% 🎉

Comparison is base (6d6a6f1) 60.35% compared to head (7d324c6) 60.37%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #246      +/-   ##
==========================================
+ Coverage   60.35%   60.37%   +0.02%     
==========================================
  Files         132      132              
  Lines       13590    13590              
==========================================
+ Hits         8202     8205       +3     
+ Misses       4633     4631       -2     
+ Partials      755      754       -1     
Flag Coverage Δ
unit 60.37% <ø> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@qiujian16
Copy link
Member

qiujian16 commented Aug 4, 2023

do we know if it works with ARM? cc @haoqing0110

@haoqing0110
Copy link
Member

Verified on arm64, all the images work fine when cgo enabled. Below are the output of registration-operator as an example:

[root@ip-172-31-16-126 ocm]# uname -a
Linux ip-172-31-16-126.ec2.internal 6.1.49-69.116.amzn2023.aarch64 #1 SMP Wed Aug 30 18:08:57 UTC 2023 aarch64 aarch64 aarch64 GNU/Linux
[root@ip-172-31-16-126 ocm]#

[root@ip-172-31-16-126 ocm]# docker run -it quay.io/open-cluster-management/registration-operator:latest-arm64 sh
sh-4.4$ ldd registration-operator <== confirm the cgo take effect
	linux-vdso.so.1 (0x0000ffff8e51c000)
	libresolv.so.2 => /lib64/libresolv.so.2 (0x0000ffff8e4ab000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x0000ffff8e476000)
	libc.so.6 => /lib64/libc.so.6 (0x0000ffff8e300000)
	/lib/ld-linux-aarch64.so.1 (0x0000ffff8e4de000)
sh-4.4$ ./registration-operator
Nucleus Operator

Usage:
  registration-operator [flags]
  registration-operator [command]

Available Commands:
  agent       Start the klusterlet agent
  completion  Generate the autocompletion script for the specified shell
  help        Help about any command
  hub         Start the cluster manager operator
  klusterlet  Start the klusterlet operator

Flags:
  -h, --help                           help for registration-operator
      --log-flush-frequency duration   Maximum number of seconds between log flushes (default 5s)
  -v, --v Level                        number for the log level verbosity
      --version                        version for registration-operator
      --vmodule moduleSpec             comma-separated list of pattern=N settings for file-filtered logging (only works for the default text log format)

Use "registration-operator [command] --help" for more information about a command.


@qiujian16
Copy link
Member

qiujian16 commented Sep 11, 2023

@elgnay could we set this as an optional env var, so we can choose to build with or without CGO (disabled by default)

@elgnay
Copy link
Contributor Author

elgnay commented Oct 18, 2023

As we can see from the golang doc: https://tip.golang.org/doc/go1.20. The default value of CGO_ENABLED depends on the build environment. It's hard to set a default value in the Makefile. So I'm closing this PR. If anyone wants to enable the use of CGO, a custom Dockerfile is required with CGO_ENABLED=1 in it.

The go command now disables cgo by default on systems without a C toolchain. More specifically, when the CGO_ENABLED environment variable is unset, the CC environment variable is unset, and the default C compiler (typically clang or gcc) is not found in the path, CGO_ENABLED defaults to 0. As always, you can override the default by setting CGO_ENABLED explicitly.

The most important effect of the default change is that when Go is installed on a system without a C compiler, it will now use pure Go builds for packages in the standard library that use cgo, instead of using pre-distributed package archives (which have been removed, as noted above) or attempting to use cgo and failing. This makes Go work better in some minimal container environments as well as on macOS, where pre-distributed package archives have not been used for cgo-based packages since Go 1.16.

@elgnay elgnay closed this Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants