Makefile: introduce ARCHCFLAGS for arch specific cflags

[marcusfolkesson]

Do not use $(USERCFLAGS) for anything other than what the user provide.

If $(USERCFLAGS) is provided by the user, then the make command will be unable to add machine specific options (used for ARM) resulting in build failures.

Instead, introduce $(ARCHCFLAGS) for such flags.

[adrianreber]

Thanks for the PR. I am not sure why the user should overwrite USERCFLAGS.

I probably just don't get it totally. Is it somewhere documented that USERCFLAGS can be set or is it custom that a variable with this name is used for that?

I would expect, as a user, to be able to specify cflags via CFLAGS. Just looking for some additional clarification.

[marcusfolkesson]

The USERCFLAGS is, what I can tell, not documented.

Based on its name, I assume the purpose is to let the user add additional flags to the compiler.
It is also reasonable to assume that is the purpose since it is used early in the Makefile (only possible input is from user):

ifeq ($(origin HOSTCFLAGS), undefined)
HOSTCFLAGS := $(CFLAGS) $(USERCFLAGS)
endif

I'm completely okay with that, but reusing the variable and setting it in the Makefile does not work.
If the user sets USERCFLAGS, then the Makefile will be unable to override it, resulting in the -march=... for the ARM architecture is never set.

Similiar problems would be if the user sets CFLAGS on command line.

[codecov-commenter]

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (6d0e785) 70.64% compared to head (98ef566) 70.55%.
Report is 45 commits behind head on criu-dev.

❗ Current head 98ef566 differs from pull request most recent head 576119b. Consider uploading reports for the commit 576119b to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##           criu-dev    #2288      +/-   ##
============================================
- Coverage     70.64%   70.55%   -0.10%     
============================================
  Files           133      132       -1     
  Lines         33318    33515     +197     
============================================
+ Hits          23539    23647     +108     
- Misses         9779     9868      +89     

Files	Coverage Δ
criu/apparmor.c	54.15% <ø> (+0.14%)	⬆️
criu/include/parasite.h	100.00% <ø> (ø)
criu/include/vma.h	100.00% <100.00%> (ø)
criu/parasite-syscall.c	86.06% <100.00%> (+0.04%)	⬆️
criu/proc_parse.c	68.66% <100.00%> (+0.11%)	⬆️
criu/tun.c	73.93% <100.00%> (-0.23%)	⬇️
criu/files-reg.c	75.38% <50.00%> (ø)
criu/memfd.c	82.38% <81.81%> (-0.21%)	⬇️
criu/net.c	76.60% <60.00%> (-0.09%)	⬇️
criu/kerndat.c	57.83% <62.50%> (ø)
... and 1 more

... and 20 files with indirect coverage changes

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

[adrianreber]

For the Fedora package we do: CFLAGS+=something make

https://src.fedoraproject.org/rpms/criu/blob/rawhide/f/criu.spec#_115

Would that work for you?

Just had a closer look at $USERCFLAGS:

$ git grep USERCFLAGS
Makefile:        HOSTCFLAGS := $(CFLAGS) $(USERCFLAGS)
Makefile:                USERCFLAGS += -march=armv6
Makefile:                USERCFLAGS += -march=armv7-a+fp
Makefile:                USERCFLAGS += -march=armv7-a
Makefile:CFLAGS                 += $(USERCFLAGS) $(WARNINGS) $(DEFINES) -iquote include/

It doesn't seem to be ever overwritten. Just extended by +=, right?

Again, just trying to understand what is correct here. Calling it $ARCHCFLAGS sounds like a good idea, but I am still not sure what error you see.

[marcusfolkesson]

Hum hum, making it part of the environment would work, IOW:

USERCFLAGS=foo make
works but

make USERFLAGS=foo
does not

Maybe this is no problem then :-)

I would still prefer to not reuse USERCFLAGS for other than user-provided flags though. So maybe just change the commit message?

[rppt]

Maybe this is no problem then :-)

I would still prefer to not reuse USERCFLAGS for other than user-provided flags though.

It makes sense to me to separate architecture specific flags from USERCFLAGS

So maybe just change the commit message?

Yes :)
Also, test/zdtm/Makefile.inc should be updated as well

[adrianreber]

Looks like a useful change.

[rppt]

LGTM