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

build apiclient as static binary #552

Merged
merged 2 commits into from
Nov 21, 2019
Merged

build apiclient as static binary #552

merged 2 commits into from
Nov 21, 2019

Conversation

bcressey
Copy link
Contributor

@bcressey bcressey commented Nov 21, 2019

Issue #, if available:
N/A

Description of changes:
apiclient is mapped into host containers, so we need to build it as a static binary to minimize its dependencies.

Testing done:
Built an AMI, confirmed that apiclient worked inside control container.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

The apiclient binary is copied into host containers, and can't rely
on its shared libraries to exist or to be compatible versions.

Signed-off-by: Ben Cressey <[email protected]>
@iliana iliana added this to the v0.2.0 milestone Nov 21, 2019
@@ -171,6 +174,11 @@ for p in \
do
install -p -m 0755 ${HOME}/.cache/%{__cargo_target}/release/${p} %{buildroot}%{_cross_bindir}
done

for p in apiclient ; do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for p in apiclient ; do
for static_binary in apiclient ; do

%cargo_build_static --manifest-path %{_builddir}/workspaces/Cargo.toml \
-p apiclient \
%{nil}

%install
install -d %{buildroot}%{_cross_bindir}
for p in \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for p in \
for dynamic_binary in \

%__global_rustflags_toml [%{lua:
for arg in string.gmatch(rpm.expand("%{__global_rustflags}"), "%S+") do
# Enable dynamic linking.
%__global_rustflags_shared -Cprefer-dynamic %__global_rustflags
Copy link
Contributor

Choose a reason for hiding this comment

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

nit/confusion: shouldn't the overrides for this more specific variable come after the more general variable expands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I preferred it this way so the relevant changes were the first part of the definition, but I don't feel strongly.

@@ -7,23 +7,33 @@

%__cargo %{_bindir}/cargo
%__cargo_common_opts --offline --locked --verbose
%__cargo_target %{_cross_arch}-unknown-linux-%{_cross_libc}
%__cargo_target %{_cross_arch}-unknown-linux-gnu
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the change from _cross_libc to hardcoded "gnu" a bug fix, or is it just asserting that we'll always link to musl if static, and glibc for dynamic?

nit: should this (and cross_opts/pkg_config/env) be suffixed _shared?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly the latter. There are only a couple of %_cross_libc uses now and I may clean it all the way up at some point.

I left out the _shared suffix here since I wanted that to be the default and wanted _static to stand out when used in a spec.

@bcressey bcressey merged commit 711a7b1 into develop Nov 21, 2019
@iliana iliana deleted the apiclient-fix branch December 10, 2019 21:45
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