Page MenuHomeFreeBSD

release: Integrate mfsBSD image build targets into the release tool set
Needs ReviewPublic

Authored by soobinrho on Sep 3 2023, 1:13 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 10, 4:12 PM
Unknown Object (File)
Mon, Dec 9, 4:12 PM
Unknown Object (File)
Fri, Dec 6, 11:03 AM
Unknown Object (File)
Wed, Nov 27, 1:18 PM
Unknown Object (File)
Sun, Nov 24, 7:22 PM
Unknown Object (File)
Sat, Nov 23, 7:00 AM
Unknown Object (File)
Fri, Nov 22, 10:56 PM
Unknown Object (File)
Nov 12 2024, 2:34 PM

Details

Reviewers
gjb
dch
Group Reviewers
releng
Summary

This commit integrates mfsBSD into the release/Makefile.

Test Results

Tested under 15.0-CURRENT. Completes without any error and produces
mfsbsd-se.img and mfsbsd-se.iso at /usr/obj/usr/src/${ARCH}/release/.

Two ways of building mfsBSD have been implemented. The first use case is
intended for most use cases, while the second is intended only for
release engineering purposes -- e.g. creating all of the FreeBSD
installation media for distribution purposes.

First Use Case

cd /usr/src/release && make mfsbsd-se.img mfsbsd-se.iso
Basically, it's very similar to mfsBSD's own make process with the
CUSTOM=1 BUILDWORLD=1 BUILDKERNEL=1 flags. What this means is that
this process invokes cd /usr/src && make buildworld buildkernel
internally and uses those object files to create mfsBSD images.

Second Use Case

cd /usr/src/ && make buildworld buildkernel -j12 && cd release && make release WITH_MFSBSD=1
The mfsBSD build process is the same for both the first use case and
the second use case. However, the second use case also build all the
release artifacts and all of the FreeBSD installation media, such as
cdrom, dvdrom, memstick, and mini-memstick. The first use case only
builds mfsBSD itself, and thus is intended for most use cases, while
the second use case's purpose is to build all release media.

Testing

I've used two of my amd64 machines for testing. Here's the script I used
for testing and verifying this code:

#!/bin/sh
alias DATE="date '+%Y%m%d_%H%M'"
mkdir -p /logs_mfsbsd

#
# TESTING USE CASE 1
#
cd /usr/src/release
make mfsbsd-se.img mfsbsd-se.iso \
    > /logs_mfsbsd/interface1_build_$(DATE).log 2>&1

echo "--------------------------------------------------------------"
echo "> Type: mfsBSD build"
echo "> Completed Time: $(DATE)"
echo "--------------------------------------------------------------"

ls -lh /usr/obj/usr/src/amd64.amd64/release/mfsbsd*

#
# Clean up.
#
make clean > /dev/null
cd /usr/src && make clean > /dev/null

#
# TESTING USE CASE 2
#
cd /usr/src
time make buildworld -j12 \
        > /logs_mfsbsd/interface2_buildWorld_$(DATE).log 2>&1
time make buildkernel -j6 \
        > /logs_mfsbsd/interface2_buildKernel_$(DATE).log 2>&1

cd ./release
COUNT=0
while [ $COUNT -lt 10 ]
do
    time make release WITH_MFSBSD=1 \
            > /logs_mfsbsd/interface2_release_$(DATE).log 2>&1

    mkdir -p ./images
    make install WITH_MFSBSD=1 DESTDIR=/usr/src/release/images \
            > /logs_mfsbsd/interface2_install_$(DATE).log 2>&1

    echo "--------------------------------------------------------------"
    echo "> Type: mfsBSD build alongside all release artifacts"
    echo "> Completed Time: $(DATE)"
    echo "--------------------------------------------------------------"

    ls -lh /usr/src/release/images/mfsbsdimages/
    make mfsbsd-clean > /dev/null

    COUNT=$(expr $COUNT + 1)
done

Note that the testing script above iterates the build process 10 times.
It was because I had a bug in which the build process fails every other
time. Now it's fixed, but I just wanted to ensure that the bug is gone
and the build process completes successfully now.

Extra Testing

Coincidentally, one of the two machines used for testing did not have
internet access because there's no driver for the AQC113C ethernet
chipset available yet, but thanks to this, this showed me that the code
works equally well even without internet access.

In addition, this code was tested to ensure its compatibility with
release/release.sh as well. Here's my testing script for release.sh:

#!/bin/sh
alias DATE="date '+%Y%m%d_%H%M'"
mkdir -p /logs_mfsbsd

#
# TESTING COMPATIBILITY WITH release.sh
#
cd /usr/src/release
cat ./release.conf.sample | sed \
	-e 's/GITROOT="https:\/\/git.freebsd.org\/"/GITROOT="https:\/\/github.com\/soobinrho\/"/g' \
	-e 's/GITSRC="src.git"/GITSRC="freebsd-src.git"/g' \
	-e 's/GITPORTS="ports.git"/GITPORTS="freebsd-ports.git"/g' \
	-e 's/SRCBRANCH="main"/SRCBRANCH="integrate-mfsBSD-building"/g' \
	-e 's/#WITH_MFSBSD=/WITH_MFSBSD=1/g' \
	> ./release.conf

./release.sh -c ./release.conf \
    > /logs_mfsbsd/release_sh_build_$(DATE).log 2>&1

echo "--------------------------------------------------------------"
echo "> Type: mfsBSD build (release.sh)"
echo "> Completed Time: $(DATE)"
echo "--------------------------------------------------------------"

ls -lh /scratch/R/mfsbsdimages
Acknowledgement

This series of commits was produced as a part of the mfsBSD integration
project, a Google Summer of Code 2023 project as described here:
https://wiki.freebsd.org/SummerOfCode2023Projects/IntegrateMfsBSDIntoTheReleaseBuildingTools

Sponsored by: Google, Inc. (GSoC 2023)
Signed-off-by: Soobin Rho <soobinrho@FreeBSD.org>

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 53561
Build 50452: arc lint + arc unit

Event Timeline

Several small nits and a question about the Unified format we did afew years ago.
But the biggest issue is writing to the source tree...

release/Makefile
350

Need newline here

release/Makefile.mfsbsd
12

Does mfsroot not support the dual image format we have support for in base for doing iso and img at the same time?

15

Do you really need the realpath expansion here? It's generally not done elsewhere.

Also I don't see the contrib mfsbsd files in this review. Are they in a different one I overlooked?

39

The indentation looks wrong. But it also looks misplaced. I'd expect something like this to be before the first .for loop above

42

I don't think this is -j safe.

Also this shouldn't be created in MFSBSD_DIR since that is supposed to be read only source.

49

I'd either skip these checks or I'd invert them to be errors if either variable is blank

58

Same comment as above

79

Why not use install(1) here?

87

These checksums should be part of the build... not the install.

91

Isn't -C ${.CURDIR} redundant?

100

I'd be tempted to add these to the CLEANFILES variables so clean-all works.

I also don't see where the files created b6 the mv I tagged as unsafe are removed.

I'd like to reiterate that we shouldn't be writing to MFSBSD_DIR

123

Newline.

release/release.conf.sample
124

Newline

release/Makefile
350

Acknowledged. Making an updated version right now. I'll update you back in a few days.

release/Makefile.mfsbsd
12

I'll definitely research into that. It might be the case there's no support for a dual image format yet. I had built mfsBSD in both .img and .iso, but .iso didn't work for drives.

I'll look into it, and update you. Thank you for pointing me to this! I appreciate it.

15

I'll test to see if we can do it without realpath expansion. Originally, there was an error (I'll document exactly where, too. Thanks for pointing it out) that occurred because ${WORLDPATH} itself contains .. and one of the build processes exited with an error because of the ..

However, I'm making a lot of new changes to the code, so it might be possible to do without realpath expansion. I'll let you know how the new code goes!

39

My bad. Correcting it now. Thank you!

42

Agreed. Correcting it to make it -j safe and also I'll make it to built at obj instead.

49

Makes sense. Thank you! Making changes accordingly now.

58

Acknowledged.

79

That's a really good idea. install(1) will be a much better implementation. Updating the code revision now.

87

/usr/src/release/Makefile.vm and /usr/src/release/Makefile create checksum during install, so I believe this may be the release convention. Please correct me if I'm wrong!

91

Agreed. Removing it and updating my code revision now.

123

Acknowledged. Correcting now.

release/release.conf.sample
124

Acknowledged.

release/Makefile.mfsbsd
100

Thank you! Taking out rm ..., and instead adding to CLEANFILES.

release/Makefile.mfsbsd
15

Sorry! I missed the second part of your question by mistake. Here's the link to the contrib mfsbsd files: https://reviews.freebsd.org/D41704

I'm still working on a revised version based on all of your feedback. I'll update you as soon as it's ready for another review. Thanks, Soobin.

soobinrho marked 11 inline comments as done.EditedSep 13 2023, 2:08 AM

Update: Revised code diffs have been updated addressing every feedback received. In addition, one major change to the code has been made -- i.e. whereas the previous version of this code allowed for only one way of building mfsBSD, another crucial way of building mfsBSD has been added.

Two Use Cases
Originally, the code only allowed for mfsBSD build process through make buildworld buildkernel && cd release && make release WITH_MFSBSD=1. This, however, had a problem in that it builds all other FreeBSD release artifacts alongside it, which can be quite inefficient when you just want to build the mfsBSD images.

Now, it allows for another way to build mfsBSD images: make mfsbsd-se.img mfsbsd-se.iso. Unlike the other use case where all release artifacts are built, this method builds only what is necessary to build mfsBSD images.

release/Makefile.mfsbsd
12

As of now, there's no native support for dual image format in mfsBSD. Could you please point me to how to implement the dual image format?

Supported formats currently are .img, .iso, gce, and tar.

42
  1. Now, the code writes to obj and never writes to src/cotrib. I've implemented it with cp -rpf <src contrib mfsBSD dir> <a temporary mfsBSD dir inside obj>. Ideally, I wanted to implement the same function without copying mfsBSD source code into another folder, but - to the best of my knowledge - it was not possible because (a) mfsBSD uses tools located inside its <src contrib mfsBSD dir>, but at the same time (b) mfsBSD saves its final image files inside ${.OBJDIR}, which means the mfsBSD build process would write its final images files to src directory, unless we copy the mfsBSD source code outside the src directory.
  1. Also, I rewrote the code from
${MAKE} -C ${.CURDIR} ${.MAKEFLAGS} ${TYPE}-${FORMAT} \
	MFSBSD_TYPE=${TYPE} MFSBSD_FORMAT=${FORMAT}
mv ${MFSBSD_DIR}/${TYPE}.${FORMAT} ${.OBJDIR}/

to this:

`${MAKE} -C ${.CURDIR} ${.MAKEFLAGS} ${TYPE}.${FORMAT}`
79
  • Updated to use install -m -644 <filename> instead of mv.
91

When -C ${.CURDIR} is removed, MAKE(1) gives an error saying the target doesn't exist. I've read codes in release/Makefile and it seems like -C ${.CURDIR} is necessary to call the targets of itself. So, -C ${.CURDIR} is kept in the new revision update.

100

Thanks again! I've removed the part where it's writing to the src contrib directory, and I've updated the code to build all mfsBSD object files at MFSBSD_TMPDIR?= ${.OBJDIR}/../contrib/mfsbsd_tmp and added CLEANDIRS+= ${MFSBSD_TMPDIR}.

Updated with fixes to the feedback. (See comment above)

soobinrho edited the summary of this revision. (Show Details)
soobinrho edited the summary of this revision. (Show Details)

I added a few comments after a first pass. I'm unfamiliar with the src build system, so treat my comments/questions with that in mind. I'm going through a few tests now, and I'll add more comments here when those are done. Also, at some point, it would also be helpful to update release(7).

release/Makefile.mfsbsd
1–3

$FreeBSD$ tags have been removed, so you can remove these lines.

17

Should we avoid hardcoding the path here? Could this be ${SRCTOP} or ${WORLDDIR}?

20–21

Are these two variables used?

27

Do we want to default to using all the system's CPUs? I see you are including ${.MAKEFLAGS}, so I think users could specify -j.

I successfully built an image, did some basic tests, and didn't see any obvious problems. \o/. Hopefully, someone from RE or someone more familiar with the build system can take another look.

release/Makefile.mfsbsd
17

Should we avoid hardcoding the path here? Could this be ${SRCTOP} or ${WORLDDIR}?

${WORLDDIR} maybe, but ${SRCTOP} is not defined in release.sh (and all the release.sh seems to only be referring to /usr/src fixed path)

In D41705#955842, @jrm wrote:

I added a few comments after a first pass. I'm unfamiliar with the src build system, so treat my comments/questions with that in mind. I'm going through a few tests now, and I'll add more comments here when those are done. Also, at some point, it would also be helpful to update release(7).

Nice findings. Thank you for noticing those! I'll include these into the next reivision update and combine them with the feedback from @releng.

And yes, updating release(7) is a good idea, and in fact, I've created this child revision for it: https://reviews.freebsd.org/D41706

release/Makefile.mfsbsd
1–3

Got it. Thanks!

17

Great idea! Changing it from /usr/src to ${WORLDDIR}.

20–21

Thank you for noticing! Deleting MFSBSD_DISTDIR and MFSBSD_ROOTDIR now. For your reference, those two variables were used in my previous iterations, but after extensive testing, it turns out it's better to do another approach, which is the current approach. I'll update the revision with these two removed.

27

Do we want to default to using all the system's CPUs? I see you are including ${.MAKEFLAGS}, so I think users could specify -j.

I believe it's necessary to keep MFSBSD_JOBS. I first tried to build the code without using any arbitrary environmental variable such as MFSBSD_JOBS at first by just using the -j flag and ${.MAKEFLAGS}, but then I realized mfsBSD is designed to accept a separte environmental variables called MAKEJOBS [1] instead of accepting -j flag. The moment when I realized this was when I ran mfsBSD build process even without any release/Makefile integration written at all. For example, cp -r /usr/src/contrib/mfsbsd /; cd /mfsbsd; make CUSTOM=1 BUILDWORLD=1 BUILDKERNEL=1 -j12 exits with an error.

So, MFSBSD_JOBS seems to be necessary for now, as long as that part of the code in the mfsBSD upstream is remaining.

[1] https://github.com/mmatuska/mfsbsd/blob/0da806178042b0d3cd20fb6b2e6e38a338a24b9c/Makefile#L156