• @ziaee needs to provide a ship-it for the manpage changes.

This still feels ambiguous in a non-desirable way to me, but I can see the value in making this possible so I want to give this the space to get more feedback instead of me serving as an unnecessary impediment.

• Please send out a message with a brief summary to hackers@ and testing@.
• Please add tests for the new feature:
  • Positive case: '! false' in m_argv[0]: negation behavior should be applied.
  • Negative case: 'true !' in m_argv[1]: negation behavior should not be applied.
• I would leave existing tests that could be simplified with the new feature as-is until enough time has been given to get feedback on the new feature.
• I would set the MFC timer to a higher value like 2 weeks or a month.

The test changes seem very reasonable. Just one style(9) suggestion to make the information easier for humans to find test names.

pkg already provides tests. Should we leverage those instead of having another set of different tests?

In D58448#1341093, @bz wrote:

In D58448#1341087, @ngie wrote:

In D58448#1340980, @bz wrote:

The module is not built at all so I wonder if you actually built it with mfsbsd.

I have other outstanding diffs on a branch which are required to get brcm to build/install: https://github.com/ngie-eign/freebsd-src/tree/brcm-polish . I'm trying to get a working driver on my mid-2010s MacBooks.

brcmfmac compiles on arm64 and amd64; that's not the issue; loading firmware will fail on arm64 (and possibly amd64) because of trouble with copies to IO space.

BUT the driver will do NOTHING even if you get it loaded as a massive piece of compat code is missing, so there is little point in even trying to at this point... unless you want to go and write that code clean room.

In D58451#1340983, @bz wrote:

The fact that there was a different behaviour with the extra bool means there was a reason for this.
We cannot unconditionally clear this and change behaviour as we still support Linux 6.1-lts drm-kmod and others.
This will need more careful investigation.

In D58448#1340980, @bz wrote:

The module is not built at all so I wonder if you actually built it with mfsbsd.

That said, If we do this we should similarly apply this to iwlwifi, rtw88, rtw89, mt76, ath1/2/3k Makefile BUT I do have outstanding changes to them in my local tree.
Would you mind if I do have a look at my local changes first before we apply this to all of them?

Ok, this is weird... This is actually a "Apple S3X NVMe Controller". I'll look at the logic carefully and see if something's awry here.

In D57087#1339149, @guest-seuros wrote:

Before we touch code, please boot to macos and let it update the firmware.

A Lot of time i found bugs in apple hardware, tougth it was quirk, then could not reproduce it once the firmware was updated. (just install the latest version possible, the update flow , update the EFI, SMC and firmwares ...

I bought a 2016 macbook, i will receive it this week. I suspect we need to handle T1.

PS: this quirk is needed for every 2018-2020 computer.

I would steer clear of naming variables nitems in the code because we have a macro that is named that already in sys/param.h.

Just taking a quick pass after you pointed me at the review -- I really like this idea -- solid :)!

This change causes my non-T2 MacBook Pro ('16 MBP) to panic at boot with "received completion for unknown cmd": https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=296946 .

@joneum : ping?

In D58262#1337132, @imp wrote:

In D58262#1336336, @ngie wrote:

Side thought: it would be nice if these were actually broken down into subpackages, e.g., indent-tests, etc.

Not sure about this, but beyond the scope.

I also have one or two other changes to make this work I'll roll in before committing.

In D58056#1336833, @kib wrote:

In D58056#1336831, @ngie wrote:

This change broke the build on armv7 (32-bit ARM). I addressed the definition/declaration mismatch in rG5cafd6213f14.

This is the supposed fix, I am waiting for tinderbox to finish.

diff --git a/sys/kern/sys_generic.c b/sys/kern/sys_generic.c
index c867867f2618..c16e806e0bd7 100644
--- a/sys/kern/sys_generic.c
+++ b/sys/kern/sys_generic.c
@@ -2353,8 +2353,8 @@ kern_exterrctl(struct thread *td, u_int op, u_int flags, void *ptr)
 }
 
 int
-exterr_set(int eerror, int category, const char *mmsg, uintptr_t pp1,
-    uintptr_t pp2, int line)
+exterr_set(int eerror, int category, const char *mmsg, uint64ptr_t pp1,
+    uint64ptr_t pp2, int line)
 {
   struct thread *td;
 

This change broke the build on ARM64/GENERIC.

Side thought: it would be nice if these were actually broken down into subpackages, e.g., indent-tests, etc.

Apply suggestions from @des and @jilles.

• Remove redundant SRCS entry added in original diff.
• Add MK_USB == no entries to OptionalObsoleteFiles.inc .

I want to echo some of the concerns @jrtc27 had about these files being best suited for packages (or at the very least being distributed as small set of files for a short period of time until they can be incorporated into the installer) instead of being hardcoded in the installer -- otherwise you're chasing whatever the [current] defaults are in the upstream projects and the configuration themselves do not change.
This level of complexity was part of how sysinstall bitrotted over time, so we should avoid this kind of anti-pattern as much as possible.
Back in the day there used to be a meta-port for X11 (then xorg) drivers. Can something like that be done to avoid a lot of the hardcoded logic? Also.. how about moving the driver selection logic into a separate/standalone script that's distributed with a package (as well). I could see something like that having a large degree of utility outside of bsdinstall for general purpose use.

Ugh. Didn't mean to "accept" the revision yet - I'm sorry :(.

This is a really nice, simple feature request -- I love it!

Disclaimers: I do not have a ports commit bit and my testing is likely limited.

@guest-seuros: can this change land independent of D57853?

In D57894#1327246, @joneum wrote:

Thanks for working on this!

I have one question regarding the YAJL option. I’m not entirely sure if I understand this correctly.

The change introduces:

YAJL_CONFIGURE_ON= --with-yajl
YAJL_CONFIGURE_OFF= --without-yajl

However, CONFIGURE_ARGS still appears to unconditionally pass:

--with-yajl=${LOCALBASE}

Could you clarify how these interact? I wasn’t sure whether this is intentional, or whether the unconditional --with-yajl=${LOCALBASE} should also become conditional now that YAJL support is optional.

Thanks!

• Remove gdbm option as it's not actually required [directly] by the package [1].
• Correct logic to truly make YAJL support conditional.

What are the conflicting symbols BTW?