Page MenuHomeFreeBSD

filesystems/pjdfstest: new port
AcceptedPublic

Authored by asomers on Wed, May 6, 2:29 PM.
Tags
None
Referenced Files
F156074420: D56848.diff
Sun, May 10, 1:03 PM
F156021980: D56848.id177304.diff
Sun, May 10, 7:07 AM
Unknown Object (File)
Sat, May 9, 5:47 PM
Unknown Object (File)
Fri, May 8, 7:22 PM
Unknown Object (File)
Fri, May 8, 7:06 PM
Unknown Object (File)
Wed, May 6, 8:34 PM
Subscribers

Details

Reviewers
markj
olivier
Summary

pjdfstest is a file system test suite to assess the correctness of file
system implementations in terms of POSIX compliance. This port is for the
Rust rewrite of the sh-based original, which remains in contrib/ for
now.

WWW: https://github.com/saidsay-so/pjdfstest

Diff Detail

Repository
R11 FreeBSD ports repository
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 72849
Build 69732: arc lint + arc unit

Event Timeline

asomers created this revision.

My ports-fu isn't really strong enough to review this, but it seems fine. I'd still be inclined to name the port something like pjdfstest-rs or something like that to differentiate it from the "official" pjdfstest, even if the port itself installs a pjdfstest executable. But given that no one has provided a pjdfstest port before, I'm not sure it matters very much.

filesystems/pjdfstest/Makefile
24

Extra newline.

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.

I'll seek out a ports committer for a more complete review.

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.

olivier added a subscriber: olivier.

Did you run a portlint -A on it ?

WARN: Makefile: COMMENT is not supposed to begin with 'A ', 'An ', or 'The '.
WARN: Makefile: use of DISTFILES with single file is discouraged. distribution filename should be set by
DISTNAME and EXTRACT_SUFX.
WARN: Makefile: DISTFILES/DISTNAME affects WRKSRC. take caution when changing them.
WARN: Makefile: seems to have unnecessary blank lines at the last part.

So I would fix the comment, because the easiest fix.
About the other, only warning, something like that could "solve" (but this is not mandatory), replacing the:

MASTER_SITES= CRATESIO
DISTFILES=    ${CARGO_DIST_SUBDIR}/${DISTNAME}${CARGO_CRATE_EXT}

with:

USE_GITHUB=   yes
GH_ACCOUNT=   saidsay-so
GH_PROJECT=   pjdfstest
GH_TAGNAME=   v${DISTVERSION}    # or whatever the upstream tag is
This revision is now accepted and ready to land.Wed, May 6, 9:44 PM

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?

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

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

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

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

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

I think having the port create it is fine, I just asked because that's not implemented in this patch AFAICS.

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.

Anyways, to make port create a user you have to define USERS and GROUPS knobs in Makefile and add entries to ${PORTSDIR}/UIDs and ${PORTSDIR}/GIDs.

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.

It is very appropriate. But this port needs not just one unprivileged user, but three. The default configuration uses tests, nobody, and pjdfstest.

Anyways, to make port create a user you have to define USERS and GROUPS knobs in Makefile and add entries to ${PORTSDIR}/UIDs and ${PORTSDIR}/GIDs.

I'll do that.

FYI, the reason why the legacy sh-based pjdfstest doesn't need to do this is because it hard-codes the three UIDs 65533, 65534, and 65535, and calls setuid directly on those UIDs without checking whether such a user exists.

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.

I don't like the idea of adding a user to the ports tree unless we're REALLY sure that we need it

As do I, but turns out portmgr thinks differently: https://reviews.freebsd.org/D56578#1297337