In D57326#1313561, @des wrote:

In D57326#1313548, @asomers wrote:

In D57326#1313547, @des wrote:

In D57326#1313542, @asomers wrote:

Why?

options was never correct, it has always been instr. I assume whoever first wrote the page copy-pasted fts_children() and forgot to adjust options to instr.

Crucially, functions that take an options argument accept multiple value bitwise-or'ed together, while fts_set() accepts only one value at a time.

The prototype at the top of the man page documents the argument as being named "options". And the header file does not contradict that. So "options" is definitely the name of the argument, and change e030e4e73fe7 was correct. Whether it _should_ be named that is a different question. I would not describe this change of yours as a reversion.

I just explained to you that the prototype at the top of the page is incorrect. The header does not name the argument. The code names the argument instr and always has; options was never a reasonable name for this argument.

In D57326#1313547, @des wrote:

In D57326#1313542, @asomers wrote:

Why?

options was never correct, it has always been instr. I assume whoever first wrote the page copy-pasted fts_children() and forgot to adjust options to instr.

Crucially, functions that take an options argument accept multiple value bitwise-or'ed together, while fts_set() accepts only one value at a time.

Why?

In D57318#1313384, @jlduran wrote:

Hmm, I was about to submit a change that:

--- a/usr.sbin/bsdinstall/scripts/script
+++ b/usr.sbin/bsdinstall/scripts/script
@@ -50,6 +50,7 @@ f_include $BSDCFG_SHARE/variable.subr
 ############################################################ GLOBALS

 : ${TMPDIR:="/tmp"}
+: ${DISTRIBUTIONS=""}; export DISTRIBUTIONS

 #
 # Strings that should be moved to an i18n file and loaded with f_include_lang()

In my opinion is a "better" fix.

In D56717#1313354, @jlduran wrote:

Sorry for not testing this thoroughly, I just realized that in order for DISTRIBUTIONS to work, the variable must be exported (see inline). Otherwise the user should "manually" export it. If this is going to be the case, then the manual page should be updated to include export DISTRIBUTIONS in the example.

Not really a domain expert, either, am I. Can you give me a clue about how to test this? I can use virtio-scsi with bhyve. But how does one tell bhyve to tell the guest that the storage capacity has changed?

Add js, who's taken an interest in libxo.

I haven't tested this yet, but it looks like a good start. My main questions are why you use manual space padding in some places instead of a field width specifier, and why fuser still includes so many " if (xo_get_style(NULL) == XO_STYLE_TEXT)" expressions.

@emaste I went with option B since there isn't a 1:1 mapping between COMPONENTS and DISTRIBUTIONS. @jlduran the current Lua code doesn't make it possible to install both base and non-base packages using the same variable. It could maybe happen in the future, but not for now. And it's not the way that the interactive installer works. So an installerconfig that installs non-base packages still needs to look something like this:

export COMPONENTS="base debug lib32"
...
#! /bin/sh
...
/usr/sbin/pkg install -y sysutils/py-salt

• Man page changes requested by @ziaee
• Eliminate the METHOD variable

I don't like the idea of adding a user to the ports tree unless we're REALLY sure that we need it, because the user has to stay there for pretty much forever. So I took a longer harder look at the tests today, and I think we can remove the third user without sacrificing coverage. I'm going to take the issue up upstream. I'll settle it there before merging this PR.

In D56848#1303169, @arrowd wrote:

Yes, unless you have a better idea for a third builtin user that it can use.

We have the tests builtin user, maybe it is appropriate.

In D56872#1303138, @emaste wrote:

LGTM, but is it worth adding an echo in the case that PKG_VERSION didn't change so that we always emit some message here?

Something like

.if defined(PKG_VERSION_FROM_DIR)
    .if ${PKG_VERSION} != ${PKG_VERSION_FROM}
        @echo "==> Package version ${PKG_VERSION} unchanged, keeping old package set"
    .else
        ...
     .endif
.endif

Or actually, avoiding more indentation, changing the existing else case to

.elif defined(PKG_VERSION_FROM_DIR)
    @echo version not changed
.else
    @echo bootstrapping repository, not checking...
.endif

• For non-interactive installations, default COMPONENTS to "base

@brd this is what I was talking about when I complained about errors running "make update-packages".

• Remove "pkgbase" from user-facing documentation.

Actually @emaste we can't do what I just suggested, because either packages or distsets might need to be fetched over the network. It's valid for neither to be on the installation media. So if you don't like the PKGBASE=no variable, then the best proposal I can come up with is:

• If COMPONENTS is set in /etc/installerconfig, use packages
• If DISTRIBUTIONS is set, use distsets
• If both are set, error
• If neither is set, use packages with the default COMPONENTS.

In D56848#1302599, @markj wrote:

Do you still want the port to add a pjdfstest user?

Yes, I saw the portlint output. I've fixed the comment and the blank line, but for some reason arcanist can't update the diff. Regarding DISTFILES, I prefer to download from CRATESIO instead of from Github. Though admittedly, I don't have a very good reason for doing that. And as long as the port is set to download from CRATESIO, I can't figure out how to get rid of setting DISTFILES. It's what all of the other ports seem to do. Do you think it's important enough to switch the download to Github?

I don't know why, but arcanist is struggling to update this port. But I've noted your whitespace observation and I will be sure to commit it correctly.

I don't think we need to worry about pjdfstest/pjdfstest-rs name collisions. In addition to the fact that there's never been a pjdfstest port, the sh-based pjdfstest isn't even very amenable to system-wide installation. That's why nobody's ever created a port before.

In D56717#1299812, @emaste wrote:

So if you want a distset installation, you must specify DISTRIBUTIONS=... and PKGBASE=no in the installerconfig. Otherwise, you must specify COMPONENTS=... .

Could we not do

1. If COMPONENTS is set and packages are on the install media, use packages
2. If DISTRIBUTIONS is set and dist sets are on the install media, use dist sets
3. Error

Thanks for fixing this. You should ask re@ if they want it in 15.1.

See also D56765

• Fixup: sooth pep8

Thanks for catching this. Would it be possible to add an ATF test to catch the problem?

In D56717#1299794, @jrtc27 wrote:

In D56717#1299142, @asomers wrote:

In D56717#1299128, @jrtc27 wrote:

Should this not do the same as 9134ed157388f3e34374322a5de06449a031f1ec?

It's not the same thing. That older revision changed the default interactive installation to pkgbase. What my change is is for the non-interactive installations. That is, the installations that the CD will do if you put an /etc/installerconfig file on the CD.

My point is, that commit made the default depend on what's on the installation media. Should this change not do the same, rather than make it always pkgbase?

In D56717#1299770, @ziaee wrote:

We're trying to avoid making pkgbase a proper user facing noun

In D56717#1299128, @jrtc27 wrote:

Should this not do the same as 9134ed157388f3e34374322a5de06449a031f1ec?

Note that @grembo , while AFK, indicated his approval to me over email.

In D56675#1298767, @emaste wrote:

LGTM, feel free to give other reviewers some time.

• Respond to emaste's style comment.

This was independently fixed by @vishwin in cfbc3c7b9000a58bf0dd99b109b2e638b8a9add5 .

Now the revision shows two ways to solve the problem:

• Merge the two gitrefs into a single cell

This is D54342 rebased.

Sorry: I rebased in between the first and second revisions. I'll abandon and recreate the revision.

• Merge the two gitrefs into a single cell

• Split the ZFS libuutil change into its own row in the table

Yes, the plan is that this will replace the sh-based implementation. So I don't want to completely rename it. Or I suppose we could just retire the sh-based implementation, and call this one "pjdfstest2" or something.
Also, while this diff LGTM, you shouldn't commit it just yet, until we resolve all of the issues you opened on Github. That's because some of those might change the config file format.

Good catch. Is this something that can be triggered with libcam?