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

RPM: cleanup gobuild macro for CentOS Stream #2501

Merged
merged 2 commits into from
Jan 31, 2025

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented Jan 23, 2025

The default gobuild macro on CentOS Stream now accounts for BUILDTAGS, so we don't need to redefine the macro in rpm spec.

@lsm5
Copy link
Member Author

lsm5 commented Jan 23, 2025

The builder-live.log.gz files in the copr builds show BUILDTAGS set in spec are used by gobuild.

@lsm5
Copy link
Member Author

lsm5 commented Jan 23, 2025

@mtrmac PTAL.

Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks! I love the cleanup, but there’s one part I’m unsure about.

# c9s bz: https://bugzilla.redhat.com/show_bug.cgi?id=2227328
# c8s bz: https://bugzilla.redhat.com/show_bug.cgi?id=2227331
%if %{defined rhel}
%define gobuild(o:) go build -buildmode pie -compiler gc -tags="rpm_crashtraceback libtrust_openssl ${BUILDTAGS:-}" -ldflags "-linkmode=external -compressdwarf=false ${LDFLAGS:-} -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \\n') -extldflags '%__global_ldflags'" -a -v -x %{?**};
Copy link
Contributor

Choose a reason for hiding this comment

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

libtrust_openssl is not a global RHEL tag, it is specific to our github.com/containers/libtrust . Should we really just drop it?

I see that the c9s builds still include the tag (apparently it was hard-coded in go-srpm-macros in some versions/patches?) but the c10s builds don’t.

Should we add that tag back into our BUILDTAGS in skopeo.spec? Or is it truly OK to drop it?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch. You're right and my bad for missing that. I'll fix that in spec in this PR itself. Putting back on draft.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, the latest build does show libtrust_openssl tag for centos stream builds: https://copr.fedorainfracloud.org/coprs/packit/containers-skopeo-2501/build/8566716/ . It will be set for rhel too.

I also included another commit to silence rpmlint.

@lsm5 lsm5 marked this pull request as draft January 24, 2025 07:18
@lsm5 lsm5 force-pushed the rpm-go-macro-cleanup branch from d485a87 to 409a359 Compare January 24, 2025 10:56
@lsm5 lsm5 marked this pull request as ready for review January 24, 2025 11:34
Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -146,6 +145,10 @@ cp -pav systemtest/* %{buildroot}/%{_datadir}/%{name}/test/system/
#define license tag if not already defined
%{!?_licensedir:%global license %doc}

# Include this to silence rpmlint.
# Especially annoying if you use syntastic vim plugin.
%check
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we consider at least a smoke-test, like skopeo --version? Anyway, that’s not this PR.

lsm5 added 2 commits January 31, 2025 20:20
The default gobuild macro on CentOS Stream now accounts for `BUILDTAGS`,
so we don't need to redefine the macro in rpm spec.

The `libtrust_openssl` has been set in the spec for RHEL
environments.

Signed-off-by: Lokesh Mandvekar <[email protected]>
We're not running any tests in the check section.

Signed-off-by: Lokesh Mandvekar <[email protected]>
@mtrmac mtrmac force-pushed the rpm-go-macro-cleanup branch from 409a359 to 5eba061 Compare January 31, 2025 19:20
@mtrmac mtrmac merged commit 0eb4ad2 into containers:main Jan 31, 2025
23 checks passed
@lsm5 lsm5 deleted the rpm-go-macro-cleanup branch February 10, 2025 11:35
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.

2 participants