Bug 258294

Comment 1 Kubilay Kocak 2021-09-06 02:52:39 UTC

Are environment/user CFLAGS and other flags (LDFLAGS, etc) honoured? Honoured meaning 'appended', not just included

(In reply to Kubilay Kocak from comment #1)

> Are environment/user CFLAGS and other flags (LDFLAGS, etc) honoured?

Yes. Checked via build log:

[...]
 #### /usr/local/etc/poudriere.d/130-amd64-make.conf ####
-CFLAGS+=-O0
-LDFLAGS+=-Wl,--as-needed
+CFLAGS+=-O3
+LDFLAGS+=-Wl,--no-as-needed
[...]
-  CFLAGS                                   -O2 -pipe -O0  -fstack-protector-strong -isystem /usr/local/include -fno-strict-aliasing  -DOS_FREEBSD -std=c11 -I${srcdir}/../rust/gen -I${srcdir}/../rust/dist
+  CFLAGS                                   -O2 -pipe -O3  -fstack-protector-strong -isystem /usr/local/include -fno-strict-aliasing  -DOS_FREEBSD -std=c11 -I${srcdir}/../rust/gen -I${srcdir}/../rust/dist
[...]
-/bin/sh ../../libtool  --tag=CC   --mode=link cc -I../.. -D_GNU_SOURCE -g -Wall -Wextra -std=gnu99 -pedantic -Wextra -Wno-missing-field-initializers -Wshadow -Wpointer-arith -Wstrict-prototypes -Wmissing-prototypes -Wno-unused-parameter -O2 -pipe -O0  -fstack-protector-strong -isystem /usr/local/include -fno-strict-aliasing  -O2 -Wstrict-overflow=1 -fstack-protector -D_FORTIFY_SOURCE=2 -Wformat -Wformat-security -fPIC  -Wl,--as-needed -fstack-protector-strong  -L/usr/local/lib -o liblzma-c.la    LzFind.lo LzmaDec.lo  -lz -L/usr/local/lib
+/bin/sh ../../libtool  --tag=CC   --mode=link cc -I../.. -D_GNU_SOURCE -g -Wall -Wextra -std=gnu99 -pedantic -Wextra -Wno-missing-field-initializers -Wshadow -Wpointer-arith -Wstrict-prototypes -Wmissing-prototypes -Wno-unused-parameter -O2 -pipe -O3  -fstack-protector-strong -isystem /usr/local/include -fno-strict-aliasing  -O2 -Wstrict-overflow=1 -fstack-protector -D_FORTIFY_SOURCE=2 -Wformat -Wformat-security -fPIC  -Wl,--no-as-needed -fstack-protector-strong  -L/usr/local/lib -o liblzma-c.la    LzFind.lo LzmaDec.lo  -lz -L/usr/local/lib
[...]

I didn't expose rust related build log via cargo.mk verbosity due to unfamiliarity with cargo, though the difference can be observed via build speed
(-O0 vs -O3):

[...]
    Compiling suricata v6.0.3 (/wrkdirs/usr/ports/security/suricata/work/suricata-6.0.3/rust)
-    Finished release [optimized + debuginfo] target(s) in 39.10s
+    Finished release [optimized + debuginfo] target(s) in 36.99s
[...]

> needs-qa

Can you clarify what QA does this change need? This port does not have consumers
and I already did QA on Tier 1 platforms and made sure that the package contents did not change.

(In reply to Evgeniy Khramtsov from comment #2)

> made sure that the package contents did not change.

Ctrl-C/Ctrl-V related typo from bug 258292.

I don't mind adding this but it feels like a high maintenance approach to hardcode RUSTFLAGS from ports directly seeing this is cherry-picked out of Mk/Uses/cargo.mk


Cheers,
Franco

Ping. This is still an issue even after ports 93924a20b38e.

I approved as maintainer, but I am not a committer.

The non-blocking question of maintainability still stands though.

Comment 7 Craig Leres 2021-09-21 20:00:20 UTC

Comment on attachment 227693 [details]
v1 (use "git am")

I'd prefer an explicit maintainer approval on the specific patchset.

Created attachment 228169 [details]
v1.1 (use "git am")

Update change after ports 7059b437276.

I request maintainer approval on this specific patchset, as requested by leres@.

Comment on attachment 228169 [details]
v1.1 (use "git am")

Maintainer approval was given for this ticket. The semantics of the patch are clear, but are less interesting as the open question of how this will be maintained when RUSTFLAGS need changes which this doesn't address at all (my original question).

Nevertheless, here is an explicit patch approval.


Thanks,
Franco

Comment 10 Craig Leres 2021-09-27 16:47:07 UTC

Two things about maintainer-approval; first (Evgeniy Khramtsov), you need to specify who the maintainer is when you request it. I think this is a relatively recent change. (I fixed it for your revised patch.) Second (Franco Fichtner), you give maintainer approval by clicking on "details" for the attachment/patch, changing the '?' to a '+', and clicking "submit". Maintainer approval is different from maintainer feedback (the latter seems to be some kind of possible use for tracking maintainer timeout?)

Sorry to be pedantic about this but it makes me more comfortable when I commit a PR patch for it to have explicit maintainer approval.

Comment on attachment 228169 [details]
v1.1 (use "git am")

Well, I did comment on the patch and try to set flag from the same page. Trying again now.

(In reply to Craig Leres from comment #10)

Ping.

Comment 13 commit-hook 2021-10-08 16:30:38 UTC

A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=923567274a0367bc5a67c568c882017eef629b2c

commit 923567274a0367bc5a67c568c882017eef629b2c
Author:     Craig Leres <leres@FreeBSD.org>
AuthorDate: 2021-10-08 16:29:37 +0000
Commit:     Craig Leres <leres@FreeBSD.org>
CommitDate: 2021-10-08 16:29:37 +0000

    security/suricata: Respect CC

    PR:             258294
    Reported by:    Evgeniy Khramtsov
    Approved by:    Franco Fichtner (maintainer)

 security/suricata/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comment 14 Craig Leres 2021-10-08 16:33:03 UTC

Apologies; I totally missed that you approved the patch at the same time you said you were going to.

Thanks for the patch!