Re: [PATCH 2/3] Makefile: Add support for legacy kernels

[Crystal Wood <crwood@redhat.com>]

On Wed, 2025-10-08 at 09:07 -0400, Derek Barbosa wrote:
> On Tue, Oct 07, 2025 at 01:50:57PM -0500, Crystal Wood wrote:
> > On Thu, 2025-10-02 at 16:55 -0400, Derek Barbosa wrote:
> > > Also, disable Intel CET -fcf-protection and BPF support, remove
> > > annocheck targets, and default to the c99 compiler (if present).
> > 
> > How are these related to the kernel version?  And I don't see any
> > changes to annocheck.
> > 
> 
> If I am not mistaken, CET [0][1] is fairly recent. Attempting to build with this
> flag on a RHEL-7 box (I need to check the gcc version again) was met with an
> "unknown option".

OK but that's a GCC version issue, not a kernel version issue.

I still don't get why we only use c99 for legacy builds... does
something in the non-legacy build need a newer standard that is now
default?

> > > Signed-off-by: Derek Barbosa <debarbos@redhat.com>
> > > ---
> > >  Makefile     | 40 +++++++++++++++++++++++++++++++++++++++-
> > >  src/stalld.c |  6 ++++--
> > >  2 files changed, 43 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index 7234a46..c09d574 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -9,6 +9,14 @@ ARCH=$(shell uname -m)
> > >  endif
> > >  $(info ARCH=$(ARCH))
> > >  
> > > +LEGACY_VER := 3
> > > +ifeq ($(strip $(KERNEL_VER)),)
> > > +KERNEL_VER=$(shell uname -r | cut -f 1 -d .)
> > > +endif
> > > +$(info Kernel Major Version is $(KERNEL_VER))
> > > +
> > > +IS_LEGACY:= $(shell test $(KERNEL_VER) -le $(LEGACY_VER) && echo "true" || echo "false")
> > 
> > What happens when we need to test for a feature that isn't even loosely
> > associated with 3.x versus 4.x+?  Why are all these things being lumped
> > under one vague "legacy" label?
> > 
> 
> What other features would we be testing for, outside of the ones disabled for
> compatability purposes?

I just meant that lumping all compatibility flags into one big "LEGACY"
made me twitch. :-)

If we're going to do that anyway (e.g. to simplify testing) I'd at
least have a comment indicating that this is being used as a heuristic
for older systems in general to avoid the need for a more complicated
build system (at least for now), to save readers some head scratching.

> > > +ifeq ($(IS_LEGACY),true)
> > > +$(info Compiling with Legacy Mode enabled)
> > > +$(info Overwriting RPMCFLAGS...)
> > > +CFLAGS	:=	-DVERSION=\"$(VERSION)\" $(FOPTS) $(MOPTS) $(WOPTS) $(DEFS) -std=c99 -DLEGACY
> > > +endif
> > 
> > It's overwriting CFLAGS, not RPMCFLAGS.  And if both RPMCFLAGS and
> > LEGACY are set, it looks like this ignores RPMCFLAGS.

I think my confusion here was that it's overriding rather than
overwriting.  If we do end up doing this, a comment explaining why
would be helpful.

> > 
> > Why is SOPTS dropped?  The closest thing I can see in the changelog is
> > -fcp-protection, but that's part of FOPTS (along with many other
> > things).
> 
> ```
> SOPTS	:= 	-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
> -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1
> ```
> When building on RHEL-7, it was though SOPTS still make their way through, even
> though the comment specifies that these are variables that only come from the
> specfile for DNF-family distributions.I was getting errors due to annobin not
> being present, so I stripped it out.

Oh, I was looking for "annocheck" which explains why I didn't notice
that.

So the issue is that when you *are* building an RPM, the specfile isn't
passing the right things for that OS version?  Wouldn't that be an
issue with the specfile?

> > > @@ -221,3 +240,22 @@ tarball:  clean
> > >  
> > >  annocheck: stalld
> > >  	annocheck --ignore-unknown --verbose --profile=el10 --debug-dir=/usr/lib/debug/ ./stalld
> > > +
> > > +help:
> > > +	@echo  'Cleaning targets:'
> > > +	@echo  '  clean            - Clean the main executable, tests, objects, and tarballs.'
> > > +	@echo  ''
> > > +	@echo 'Building targets:'
> > > +	@echo '  stalld            - Build the main executable (stalld).'
> > > +	@echo '  static            - Build a statically linked executable (stalld-static).'
> > > +	@echo '  tests             - Run tests in the "tests" subdirectory.'
> > > +	@echo '  tarball           - Create a source tarball: $(NAME)-$(VERSION).tar.$(CEXT)'
> > > +	@echo ''
> > > +	@echo 'Installation targets:'
> > > +	@echo '  install           - Install the executable, man page, docs, and licenses.'
> > > +	@echo '  uninstall         - Remove all installed files.'
> > > +	@echo ''
> > > +	@echo 'Utility targets:'
> > > +	@echo '  annocheck         - Run the annocheck security analysis tool on the stalld executable.'
> > > +	@echo '  help              - Display this help message.'
> > > +
> > 
> > This seems unrelated?
> > 
> 
> Sorry, I am unsure what you're referring to. Are you saying the addition of a
> `make help` target is unrelated?

Yes, it seemed like something that would normally be a separate
patch, though that may be me getting stuck in kernel development mode
:-)

I wouldn't have mentioned it if the patch didn't feel off in other
ways.

-Crystal