ON-15949: Allow user to choose debugness of build #38

tcrawley-xilinx · 2024-09-20T10:36:07Z

Also includes various improvements and changes to build system and packaging scripts.

The main commit is probably ON-15949: Configure build output of zf_mkdist so that it no longer builds both release and debug versions of the library.

[1/6] ON-15695: Stop stripping tcpdirect binaries

I believe this was only done because tcpdirect was proprietary, now that we are open source this should no longer be necessary.

[2/6] ON-15695: Remove separate socket shim build in CI pipeline

The stashed .so file isn't used anymore, so don't build it in the first
place.

Maybe we want to keep this to verify the builds work?

[3/6] ON-15695: Shellcheck improvements to zf_make_tarball

[4/6] ON-15949: Configure build output of zf_mkdist

Previously zf_mkdist would build: release binaries, debug binaries,
and the socket shim. Now that tcpdirect is open-source we don't have to
do these "all in one" builds anymore.

This commit adds a new --debug option to zf_mkdist as well as
slightly changing the behaviour of --shim. Now, by default, calling
zf_mkdist will result in building tcpdirect in release mode. Adding
--debug will change this to become a debug build. --shim now only
builds the socket shim, it will inherit the debugness of the overall
library.

[5/6] ON-15949: Allow optional build of debug binaries in rpm spec

[6/6] Cleanup: Archive tcpdirect srpm in CI pipeline

I think this is a regression from 2295845 where our jenkins pipeline was no longer picking the tarball.

Testing done

Various manual tests including build and install from tarball and rpm (deb packages are TODO when I've set up a dut properly)
Jenkins pipeline passes and actually has the srpm now
I've had to make some changes to runbench build process for tcpdirect. I've got a version working and it can build tcpdirect as well as find and use the socket shim. runbench PR coming soon.

I haven't included links to private things, but feel free to ask for them if curious

abower-amd · 2024-09-20T10:45:34Z

scripts/tcpdirect_misc/tcpdirect.spec

@@ -8,13 +8,16 @@
 #
 # To build a binary RPM from a source RPM:
 #   rpmbuild --define "onload_tarball <onload_tarball>" --rebuild ~/rpmbuild/SRPMS/tcpdirect-<version>-1.src.rpm
+#
+# If you want debug binary packages add:
+#   --define "debug true"


I don't think debug packages should be an rpm build option - these are highly undesirable.

Better would be for the debug versions either to exist in parallel within the package, or, even better, in a debug sub-package. (Not to be confused with the debuginfo and debugsource packages which are different again and we should also make sure we are not inhibiting their automatic creation.)

abower-amd · 2024-09-20T10:47:04Z

scripts/zf_mkdist

+
+    # Copy binaries
+    cp -a "${build_dir}"/bin/zf_stackdump "${bin_dir}"
+    cp -a "${build_dir}"/lib/libonload_zf_static.a "${lib_dir}"


ideally there'd be a separate -static subpackage too! Although that can instead form part of a -devel subpackage.

abower-amd

So I think the ideas here are generally right and any further development could go on top.

I think for packages we should start from the principle of do what you would do if your product were part of the distro (following distro packaging guidelines) and then only deviate from that for good reason.

So I would try to gravitate away from build options towards helping the user make the choice for whether to use the debug libraries at a post-installation stage: is it easy to select debug or static versions?

And the debuginfo/debugsource stuff is entirely orthogonal and we just need to make sure we don't inhibit their autocreation.

sianj-xilinx · 2024-09-20T12:49:08Z

I agree that debug vs release shouldn't be a package build option. I think that installing both debug and release binaries by default is my preferred option. I fear that having a separate -debug package will leave us in the situation where the support team want to ask a customer to run in debug mode and find they're unable to because they're at a point where they can't install new packages.

abower-amd · 2024-09-20T12:59:59Z

I agree that debug vs release shouldn't be a package build option. I think that installing both debug and release binaries by default is my preferred option. I fear that having a separate -debug package will leave us in the situation where the support team want to ask a customer to run in debug mode and find they're unable to because they're at a point where they can't install new packages.

A good case.

One thing one could to is Provides: ....-debug or similar.

pcolledg-amd

Liking general approach.

On separate debug packages vs combined package:

Package maintainers typically just take an executive decision on the build options and distribute just that. Debug-named variant packages only really exist on EL for Kernel, Java, and Python. If the user wants different build options, they're on their own for configuring the SPEC and choosing a unique RPM NVR.

Personally, I'm in favour of one package with /bin/foo & /bin/foo-debug, even if it doubles build time, and selection of which is performed within system config (eg. alternatives) or application configs. It just seems simpler and easier for users.

pcolledg-amd · 2024-09-20T12:44:32Z

scripts/tcpdirect_misc/tcpdirect.spec


 %define _unpackaged_files_terminate_build 0
 %{!?pkgversion: %global pkgversion 9.0.0}

 Name:           tcpdirect
 Version:        %{pkgversion}
-Release:        1
+Release:        1%{?debug:DEBUG}


Commenting purely on packaging guidelines compliance: This does replicate openonload.spec but in a forthcoming spec cleanup PR I'll be proposing appending ~debug to the Version instead of Release, as can be seen in the split spec PR. (Strictly, I think it should be in the package Name, as that avoids yum update unexpectedly selecting a debug variant version.)

pcolledg-amd · 2024-09-20T12:44:38Z

scripts/zf_make_official_srpm

-try rpmbuild -ts "${zf_tgz}"
+rpmbuild_args=()
+if [ -n "$outdir" ]; then
+  rpmbuild_args=(--define "_topdir $outdir");


If you're aiming for a consistent arg with zf_make_official_deb --out then I'd aim for consistent behaviour too. Currently, this rpmbuild command will output to $outdir/SRPMS/. Also, as it stands this arg is more about setting the build environment than the output but is not named as such, which might confuse a user who has customised their ~/rpmbuild.

I was just aiming to get something that works for both jenkins and manual creation workflows. This was added so that the jenkins pipeline could pick up the packages relatively simply, I was having difficulties getting jenkins to find the packages using shell variables or absolute addresses so this was the solution I came up with. I think with a manual workflow it should be fine to ignore --outdir and configure where rpmbuild puts the packages.

I mostly just find the naming of "out" unintuitive, so minor.

In spec split PR I declared the array in an overridable way:

read -r -a rpmbuild_args <<< "$rpmbuild_args"

Which would allow for arbitrary hacks in the groovy:

sh "rpmbuild_args='--define \"_srcrpmdir ${env.WORKSPACE}\"' tcpdirect/scripts/zf_make_official_srpm"

Agreed that outdir isn't the best name. I like you idea of the overridable array, when I come back this after the release I'll incorporate it.

pcolledg-amd · 2024-09-20T13:40:48Z

scripts/zf_mkdist

+    echo "  --shim                  - Build the socket shim library. For testing only."
+    echo "                            Requires onload_tarball"
+    echo "  --debug                 - Build debug version of tcpdirect."
+    echo "  --name <tarball_prefix> - Specify prefix for generated tarball. Default is \"zf\"."


Presumably there is a reason for

name_prefix="tcpdirect" in zf_make_tarball and

name="zf" in zf_mkdist.

Yeah, runbench uses zf_mkdist to build tcpdirect and assumes that the tarball of binaries begins with "zf". zf_make_tarball is more designed for the packaging workflow where the default prefix is "tcpdirect" rather than "zf"

tcrawley-xilinx · 2024-09-20T14:32:32Z

Dropped the separate debug build bits, packages will have the same stuff as they had before (and this PR should affect runbench's ability to build tcpdirect)
I'll come back to the debug bits, but I am conscious of time constraints so this should be more mergeable now.

pcolledg-amd · 2024-09-20T15:29:15Z

scripts/zf_make_official_srpm

-try rpmbuild -ts "${zf_tgz}"
+rpmbuild_args=()
+if [ -n "$outdir" ]; then
+  rpmbuild_args=(--define "_topdir $outdir");


I mostly just find the naming of "out" unintuitive, so minor.

In spec split PR I declared the array in an overridable way:

read -r -a rpmbuild_args <<< "$rpmbuild_args"

Which would allow for arbitrary hacks in the groovy:

sh "rpmbuild_args='--define \"_srcrpmdir ${env.WORKSPACE}\"' tcpdirect/scripts/zf_make_official_srpm"

tcrawley-xilinx requested a review from a team as a code owner September 20, 2024 10:36

abower-amd reviewed Sep 20, 2024

View reviewed changes

pcolledg-amd reviewed Sep 20, 2024

View reviewed changes

tcrawley-xilinx added 4 commits September 20, 2024 15:09

ON-15695: Stop stripping tcpdirect binaries

25b0387

ON-15695: Don't stash socket shim library in CI pipeline

0f17044

ON-15695: Shellcheck improvements to zf_make_tarball

0db92fa

Cleanup: Archive tcpdirect srpm in CI pipeline

133f50f

tcrawley-xilinx force-pushed the reviews/tcrawley/ON-15949 branch 2 times, most recently from 5b9d8c0 to 133f50f Compare September 20, 2024 14:30

pcolledg-amd approved these changes Sep 20, 2024

View reviewed changes

tcrawley-xilinx merged commit 70e1460 into master Sep 20, 2024
2 checks passed

tcrawley-xilinx deleted the reviews/tcrawley/ON-15949 branch September 20, 2024 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ON-15949: Allow user to choose debugness of build #38

ON-15949: Allow user to choose debugness of build #38

tcrawley-xilinx commented Sep 20, 2024 •

edited

Loading

abower-amd Sep 20, 2024 •

edited

Loading

abower-amd Sep 20, 2024 •

edited

Loading

abower-amd left a comment

sianj-xilinx commented Sep 20, 2024

abower-amd commented Sep 20, 2024

pcolledg-amd left a comment

pcolledg-amd Sep 20, 2024

pcolledg-amd Sep 20, 2024

tcrawley-xilinx Sep 20, 2024

pcolledg-amd Sep 20, 2024

tcrawley-xilinx Sep 20, 2024

pcolledg-amd Sep 20, 2024

tcrawley-xilinx Sep 20, 2024

tcrawley-xilinx commented Sep 20, 2024

pcolledg-amd Sep 20, 2024

ON-15949: Allow user to choose debugness of build #38

ON-15949: Allow user to choose debugness of build #38

Conversation

tcrawley-xilinx commented Sep 20, 2024 • edited Loading

[1/6] ON-15695: Stop stripping tcpdirect binaries

[2/6] ON-15695: Remove separate socket shim build in CI pipeline

[3/6] ON-15695: Shellcheck improvements to zf_make_tarball

[4/6] ON-15949: Configure build output of zf_mkdist

[5/6] ON-15949: Allow optional build of debug binaries in rpm spec

[6/6] Cleanup: Archive tcpdirect srpm in CI pipeline

Testing done

abower-amd Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

abower-amd Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

abower-amd left a comment

Choose a reason for hiding this comment

sianj-xilinx commented Sep 20, 2024

abower-amd commented Sep 20, 2024

pcolledg-amd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcrawley-xilinx commented Sep 20, 2024

Choose a reason for hiding this comment

tcrawley-xilinx commented Sep 20, 2024 •

edited

Loading

abower-amd Sep 20, 2024 •

edited

Loading

abower-amd Sep 20, 2024 •

edited

Loading