In D48458#1113797, @mikael wrote:

Personally, I don't like it when a service downloads / installs things at launch, isn't there another way?

diizzy's concerns seem to be addressed. Approved.

Great clean up! Thank you for working on that.

Thanks!

Ship it!

Seems reasonable.

Looks almost perfect. Two small bits left then it's ready to land.

Approved!

Thanks for taking care of this patch.

Hey! Thank you for taking care of this patch.

Thank you!

Looks good! Thank you for taking care of that!

A couple of grammar nits. Otherwise, lgtm.

The change is great and very much welcomed. Thank you for working on that.

I've done another review pass.

Please add "Reported by: " to the commit message.

In D48522#1107151, @kiwi wrote:

In D48522#1107150, @0mp wrote:

BTW, please make sure that the www values are not hardcoded somewhere else as well:

1. Check the files/ directory.
2. Check the source code of apache for any obvious hardcoded user/group values for FreeBSD.

Nothing found in the port (grep is my friend)

Do I understand correctly that SQLITE is always enabled? Is that what we want?

Thank you! Approved!

Cool, please update the commit message with the entertainer's approval. I'll review it and then accept the revision.

Same as with other Samba 4.20 changes:

Also, please reference the Samba 4.20 PR in the commit message.

Hey! Could you create two commits of this change? It looks good, but it would be easier for everyone else to understand the actual changes between ldb versions if we follow the two-step workflow here. See https://reviews.freebsd.org/D48658#1110744

• Please add the PR: 280533 to the commit message so that it references the Samba 4.20 PR.
• It's better if we add this port similarly to how I suggested committing devel/tevent016 (see https://reviews.freebsd.org/D48658#1110744)

It would be good if we committed this patch in two steps.

1. First, commit an unmodified copy of devel/tevent as devel/tevent016.
2. Second, update devel/tevent016 to 0.1.6.

BTW, please make sure that the www values are not hardcoded somewhere else as well:

Suggestions to the commit message:

It would be good to ask on the -ports mailing list about some extra review. Installing software via Python virtual environments is unprecedented in the ports tree I think. Or at least not well established. That is why additional review would be beneficial.

BTW, did you develop this patch yourself or is it someone's work from the PR?

One more question before we land it :)

Seems good to go! Thank you

Looks solid and the maintainer has approved the change. Ship it!

Cool, thanks for the updates. Let's wait for @joneum.

Note: security/sudo: Remove dependency on security/sssd is not a bad title either.

Otherwise, the commit looks good. Ping me again for the final review when you get the approval of the maintainer (or a timeout).

Suggestions for the commit message:

Thank you, kiwi@! The patch is ready to land.

Could you update the commit message as noted here?

Please make sure to add PR: 283364 to the commit message.

Regarding the commit message:

I've got some suggestions for the commit message:

Regarding the commit message, I'd suggest something along the lines of:

Notes:

• There is no need to mention in the commit message that the patch has been tested with poudriere. It's still a very valuable comment to add to the description on Bugzilla or Phabricator :)

LGTM, let's wait for the maintainers before committing.

Could you add the whole, final commit message to the description here on Phab? I'd like to make sure you don't miss anything (please include Approved by: 0mp (mentor) but do not push the change upstream yet).

The change looks pretty good.

I've left a question in a comment but it's not blocking. I'm just curious :)

Thank you all for feedback! :)

• Add UPDATING entry
• Fix typos in rc.subr.8 and do some extra wordsmithing there

Considering that git-arc is now more portable thanks to the patches by @jlduran_gmail.com, I'm closing this revision. If we decide to start quoting variables in git-arc for the sake of defensive programming, we should review the whole file instead of just the echo cases.

I'm closing this revision as I landed @jlduran_gmail.com's patches instead:

I'll commit it soon.