Bug 258294 - security/suricata: Respect CC
Summary: security/suricata: Respect CC
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Craig Leres
URL:
Keywords: needs-qa
Depends on:
Blocks:
 
Reported: 2021-09-05 21:36 UTC by Ghost
Modified: 2021-10-08 16:33 UTC (History)
3 users (show)

See Also:
franco: maintainer-feedback+
koobs: merge-quarterly?


Attachments
v1 (use "git am") (1004 bytes, patch)
2021-09-05 21:36 UTC, Ghost
no flags Details | Diff
v1.1 (use "git am") (1011 bytes, patch)
2021-09-25 11:32 UTC, Ghost
franco: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ghost 2021-09-05 21:36:24 UTC
Created attachment 227693 [details]
v1 (use "git am")

security/suricata needs to respect CC per https://docs.freebsd.org/en/books/porters-handbook/porting-dads/#dads-cc similar to https://cgit.freebsd.org/ports/commit/?id=e6d430445

Reproduce by deleting 'cc' from jail and overriding CC to another value.

Package contents did change; PORTREVISION is bumped for security/suricata:

11.4/amd64: https://codeberg.org/ei/misc/commit/716325ef210
11.4/i386:  https://codeberg.org/ei/misc/commit/cdbc47d98c2
12.2/amd64: https://codeberg.org/ei/misc/commit/858a4cc1217
12.2/i386:  https://codeberg.org/ei/misc/commit/07437db86d8
13.0/amd64: https://codeberg.org/ei/misc/commit/14fc2ac3966

Log diff with the patch applied:

11.4/amd64: https://codeberg.org/ei/misc/commit/0a380048f17
11.4/i386:  https://codeberg.org/ei/misc/commit/097fe22f3a6
12.2/amd64: https://codeberg.org/ei/misc/commit/ae58a9c4a3c
12.2/i386:  https://codeberg.org/ei/misc/commit/a8e2c57bab2
13.0/amd64: https://codeberg.org/ei/misc/commit/7d711387079

Testport logs with the patch applied:

11.4/amd64: https://codeberg.org/ei/misc/commit/474602e619c
11.4/i386:  https://codeberg.org/ei/misc/commit/01696078a4a
12.2/amd64: https://codeberg.org/ei/misc/commit/b2656aaadd3
12.2/i386:  https://codeberg.org/ei/misc/commit/001aa252999
13.0/amd64: https://codeberg.org/ei/misc/commit/642ddf45993

13.0/aarch64 QA via qemu-user-static is omitted because the
dependency 'lang/rust' can not build per https://cgit.freebsd.org/ports/commit/lang/rust/Makefile?id=ea6c7641aa
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2021-09-06 02:52:39 UTC
Are environment/user CFLAGS and other flags (LDFLAGS, etc) honoured? Honoured meaning 'appended', not just included
Comment 2 Ghost 2021-09-06 11:31:45 UTC
(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.
Comment 3 Ghost 2021-09-06 11:33:02 UTC
(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.
Comment 4 Franco Fichtner 2021-09-06 12:21:40 UTC
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
Comment 5 Ghost 2021-09-21 15:00:05 UTC
Ping. This is still an issue even after ports 93924a20b38e.
Comment 6 Franco Fichtner 2021-09-21 15:01:44 UTC
I approved as maintainer, but I am not a committer.

The non-blocking question of maintainability still stands though.
Comment 7 Craig Leres freebsd_committer freebsd_triage 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.
Comment 8 Ghost 2021-09-25 11:32:49 UTC
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 9 Franco Fichtner 2021-09-27 07:22:14 UTC
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 freebsd_committer freebsd_triage 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 11 Franco Fichtner 2021-09-27 18:20:16 UTC
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.
Comment 12 Ghost 2021-10-03 19:59:15 UTC
(In reply to Craig Leres from comment #10)

Ping.
Comment 13 commit-hook freebsd_committer freebsd_triage 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 freebsd_committer freebsd_triage 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!