Awk should be able to handle any comments with another match line.

Looks like a debugging leftover?

I'm tempted to drop the ability to specify a source directory since it seems fragile and instead generate an mtree 2.0 file (which requires a pipe through mtree -C) like is used in the NO_ROOT case.

It will... I just wasn't sure how to do it without tracking state. Something like:

awk 'do_print=1; /^\#/ { do_print = 0 }; ...; do_print { print }'

maybe that's just fine though. I'll condense it to just awk the best I can. Let me know if that basic idea looks good, or you have something else.

yep... will remove when patch is finalized

I like that, will update.

I had to refresh my memory, but you can use next:

printf "#foo\nbar\n" | awk '/^\#/ {print; next}  /./ {print "xxx" $0}'

produces

#foo
xxxbar

@doc I welcome feedback on the content, before I format it.

@releng I now realize I am making quite a claim here... one that I don't have the authority to back up :)

My proposal with this change is that releng would build with a PKG_TIMESTAMP of their choosing, and publish that along with the REVISION in release notes. Anyone could then verify the build by running it with the same info.

If that's too much, I can back this off to simply state that it's possible to make reproducible builds, and not mention official releases.

If we keep doing this and don't just force the NO_ROOT path, I'd suggest hoisting the mtree commands up to this point an a .else branch and unconditionally creating ${dist}.meta and ${dist}.debug.meta as part of distributeworld. We could then eliminate the difference between the NO_ROOT and non-NO_ROOT paths.

This is the only place where NO_ROOT and non-NO_ROOT differ (the L flag). I tried to figure out where it came from and it arrived without explanation in the original commit. I think it's most likely an error that slipped through review.

I'm not a fan of implicitly using . here. Can we split out the mtree and awk parts so the path argument can be explicitly given at the uses?

@brooks had the opposite preference: https://reviews.freebsd.org/D48010/new/#1094353

I prefer it together as well because it ends up as a single ${MTREE_DIST} to produce the mtree file

Well, I particularly don't like hiding pipes, it looks like ${MTREE_DIST} foo bar should pass the arguments to mtree, but that's not true. Really though we should just be unconditionally creating the mtree files in distributeworld rather than reconstructing them in packageworld, and doing so with the right timestamps.

Yes, for reference the commit is 2d0bcb76c8530. It looks like I sent a draft "first attempt at debug.txz" as a start to this work. I reported having not tried even building with the patch, which was:

 diff --git a/Makefile.inc1 b/Makefile.inc1
index d65dd31..f9512ab 100644
--- a/Makefile.inc1
+++ b/Makefile.inc1
@@ -834,12 +834,31 @@ packageworld:
 .if defined(NO_ROOT)
        ${_+_}cd ${DESTDIR}/${DISTDIR}/${dist}; \
            tar cvJf ${DESTDIR}/${DISTDIR}/${dist}.txz \
+           --exclude usr/lib/debug \
            @${DESTDIR}/${DISTDIR}/${dist}.meta
 .else
        ${_+_}cd ${DESTDIR}/${DISTDIR}/${dist}; \
-           tar cvJf ${DESTDIR}/${DISTDIR}/${dist}.txz .
+           tar cvJf ${DESTDIR}/${DISTDIR}/${dist}.txz \
+           --exclude usr/lib/debug .
+.endif
+.endfor
+.if ${MK_DEBUG_FILES} != "no"
+       ${_+_}:>  ${DESTDIR}/${DISTDIR}/debug.tar
+.for dist in base ${EXTRA_DISTRIBUTIONS}
+.if defined(NO_ROOT)
+       ${_+_}cd ${DESTDIR}/${DISTDIR}/${dist}; \
+           tar rvf ${DESTDIR}/${DISTDIR}/debug.tar \
+           --include usr/lib/debug \
+           @${DESTDIR}/${DISTDIR}/${dist}.meta
+.else
+       ${_+_}cd ${DESTDIR}/${DISTDIR}/${dist}; \
+           tar rvf ${DESTDIR}/${DISTDIR}/debug.tar \
+           --include usr/lib/debug .
 .endif
 .endfor
+       ${_+_}xz ${DESTDIR}/${DISTDIR}/debug.tar
+.endif
+

 #
 # reinstall

The patch that got committed had tar cvJfL (which doesn't work), changed to tar cvJLf in 3b66656a204dc8b167c72863efb75b53a5707001.

So I concur, this was probably just an error that slipped through review. I don't see off hand why it would make sense to pass -L for the creating the debug tarball.

I think unconditional creation in distributeworld is the right way ago. Even if we decide to reconstruct them in the non-NO_ROOT case, we could just open code most of this since we'd only be writing it once.

(FWIW, what I didn't like about the . being exposed is it would be fragile to changing the command arguments because it depends -p being last.)

You can't just unilaterally remove flags. Otherwise you lose flags=schg on various key system files. -DNO_ROOT is really the best way to get deterministic output, because all the metadata comes from the build system itself, not whatever ended up on disk and affected by the environment.

we should just be unconditionally creating the mtree files in distributeworld rather than reconstructing them in packageworld, and doing so with the right timestamps.

Agreed

Addition of -R flags is because ZFS systems set uarch flag on a number of files, whereas UFS does not. I don't think this is the right place for the fix - it should be in distribute*.

Similar to -R flags, excluding *.bak and *.old should not happen here.

Should they even be necessary at all? I only got these extra files with -j20 buildworld buildkernel. Seems like some cleanup may be needed before building mtree.

One consequence of piping into ${XZ_CMD} is that if there's an issue tarring the mtree, then it produces an incomplete file and doesn't fail.

Is there a way to keep the pipe, but fail if there's an error on the ${TAR_CMD}? Or would it be okay to ${TAR_CMD} -f ${PACKAGEDIR}/${dist}-dbg.txz and bypass the pipe altogether? I don't know if someone out there is relying on the separation.

You're right, I had misunderstood mtree -i.

Moving mtree into distribute* and consolidating -DNO_ROOT is next up.

The use of XZ_CMD was added in bd9cab6fb43aa and is probably obsolete. We might want --options xz:threads=<some value> to the tar command instead.

This change is good and independent of the rest. Would you care to open a pull request and I'll pick this up?

https://github.com/freebsd/freebsd-src/pull/1547

D48041 is a first step in always using -DNO_ROOT to build release artifacts to facilitate consolidation.

Thanks, merged.