Page MenuHomeFreeBSD
Differential D5946

For pointers replace 0 with NULL.
ClosedPublic
Actions

Authored by araujo on Apr 14 2016, 11:19 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 18, 1:27 AM
Unknown Object (File)
Mon, Apr 14, 3:05 AM
Unknown Object (File)
Mon, Apr 14, 2:38 AM
Unknown Object (File)
Mon, Apr 14, 2:23 AM
Unknown Object (File)
Sun, Apr 6, 4:48 PM
Unknown Object (File)
Mar 20 2025, 3:39 PM
Unknown Object (File)
Mar 11 2025, 1:31 AM
Unknown Object (File)
Mar 7 2025, 12:15 PM
View All 87 Files
Subscribers
None

Details

Reviewers
rpaulo
imp
Commits
rS298687: For pointers use NULL instead of 0.
Summary

In some cases like fopen(3) it will return NULL in case it can't
open the STREAM.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3262
Build 3295: arc lint + arc unit

Event Timeline

araujo updated this revision to Diff 15188.Apr 14 2016, 11:19 AM
araujo retitled this revision from to For pointers replace 0 with NULL..
araujo updated this object.
araujo edited the test plan for this revision. (Show Details)
araujo added reviewers: rpaulo, imp.
rpaulo accepted this revision.Apr 27 2016, 3:03 AM
rpaulo edited edge metadata.
This revision is now accepted and ready to land.Apr 27 2016, 3:03 AM
Closed by commit rS298687: For pointers use NULL instead of 0. (authored by araujo). · Explain WhyApr 27 2016, 3:07 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
usr.sbin/
config/
30 lines
18 lines

Diff 15188

View Options

usr.sbin/config/mkmakefile.c

  • This file was added.
#
# $FreeBSD$
#
jrmUnsubmitted
Not Done
Inline Actions

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

jrm: $FreeBSD$ tags have been removed, so you can remove these lines.
soobinrhoAuthorUnsubmitted
Not Done
Inline Actions

Got it. Thanks!

soobinrho: Got it. Thanks!
#
# Makefile for building mfsBSD images.
#
MFSBSD_TYPES?= mfsbsd-se
MFSBSD_FORMATS?= img iso
MFSBSD-SEIMG_DESC= Special edition mfsBSD .img file including base.txz and kernel.txz
MFSBSD-SEISO_DESC= Special edition mfsBSD .iso file including base.txz and kernel.txz
impUnsubmitted
Not Done
Inline Actions

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

imp: Does mfsroot not support the dual image format we have support for in base for doing iso and…
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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.

soobinrho: I'll definitely research into that. It might be the case there's no support for a dual image…
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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.

soobinrho: As of now, there's no native support for dual image format in mfsBSD. Could you please point me…
#
# Paths
#
impUnsubmitted
Done
Inline Actions

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?

imp: Do you really need the realpath expansion here? It's generally not done elsewhere. Also I…
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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!

soobinrho: I'll test to see if we can do it without realpath expansion. Originally, there was an error…
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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: Sorry! I missed the second part of your question by mistake. Here's the link to the contrib…
MFSBSD_DIR?= ${WORLDDIR}/contrib/mfsbsd
MFSBSD_SRCDIR?= /usr/src
jrmUnsubmitted
Not Done
Inline Actions

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

jrm: Should we avoid hardcoding the path here? Could this be ${SRCTOP} or ${WORLDDIR}?
otisUnsubmitted
Not Done
Inline Actions

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)

otis: > Should we avoid hardcoding the path here? Could this be ${SRCTOP} or ${WORLDDIR}?
soobinrhoAuthorUnsubmitted
Not Done
Inline Actions

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

soobinrho: Great idea! Changing it from `/usr/src` to `${WORLDDIR}`.
MFSBSD_TMPDIR?= ${.OBJDIR}/../contrib/mfsbsd_tmp
MFSBSD_WRKDIR?= ${MFSBSD_TMPDIR}/work
MFSBSD_DISTDIR!= make -C ${MFSBSD_DIR} -V _DESTDIR WRKDIR=${MFSBSD_WRKDIR}
MFSBSD_ROOTDIR!= make -C ${MFSBSD_DIR} -V _ROOTDIR WRKDIR=${MFSBSD_WRKDIR}
jrmUnsubmitted
Not Done
Inline Actions

Are these two variables used?

jrm: Are these two variables used?
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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.

soobinrho: Thank you for noticing! Deleting `MFSBSD_DISTDIR` and `MFSBSD_ROOTDIR` now. For your reference…
MFSBSD_MKINCLUDEDIR?= /usr/share/mk
#
# Build defaults
#
MFSBSD_JOBS!= sysctl -n hw.ncpu
jrmUnsubmitted
Not Done
Inline Actions

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.

jrm: Do we want to default to using all the system's CPUs? I see you are including `${.MAKEFLAGS}`…
soobinrhoAuthorUnsubmitted
Not Done
Inline Actions

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

soobinrho: > Do we want to default to using all the system's CPUs? I see you are including `${.
MFSBSD_KERNCONF?= GENERIC
MFSBSD_PKGSTATIC?= /usr/local/sbin/pkg-static
.if defined(WITH_MFSBSD) && !empty(WITH_MFSBSD)
CLEANDIRS+= ${MFSBSD_TMPDIR}
. for TYPE in ${MFSBSD_TYPES}
. for FORMAT in ${MFSBSD_FORMATS}
CLEANFILES+= ${TYPE}.${FORMAT}
. endfor
. endfor
.endif
impUnsubmitted
Done
Inline Actions

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

imp: The indentation looks wrong. But it also looks misplaced. I'd expect something like this to be…
soobinrhoAuthorUnsubmitted
Done
Inline Actions

My bad. Correcting it now. Thank you!

soobinrho: My bad. Correcting it now. Thank you!
mfsbsd-base: mfsbsd-image
mfsbsd-image:
impUnsubmitted
Done
Inline Actions

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.

imp: I don't think this is -j safe. Also this shouldn't be created in MFSBSD_DIR since that is…
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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

soobinrho: Agreed. Correcting it to make it -j safe and also I'll make it to built at `obj` instead.
soobinrhoAuthorUnsubmitted
Done
Inline Actions
  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}`
soobinrho: 1. Now, the code writes to `obj` and never writes to `src/cotrib`. I've implemented it with `cp…
.if defined(WITH_MFSBSD) && !empty(WITH_MFSBSD)
. for TYPE in ${MFSBSD_TYPES}
. for FORMAT in ${MFSBSD_FORMATS}
${IMAKE} ${.MAKEFLAGS} -C ${.CURDIR} ${TYPE}.${FORMAT}
. endfor
. endfor
.endif
impUnsubmitted
Done
Inline Actions

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

imp: I'd either skip these checks or I'd invert them to be errors if either variable is blank
soobinrhoAuthorUnsubmitted
Done
Inline Actions

Makes sense. Thank you! Making changes accordingly now.

soobinrho: Makes sense. Thank you! Making changes accordingly now.
mfsbsd-se.img: mfsbsd-before-build mfsbsd-build mfsbsd-build-install
cd ${MFSBSD_TMPDIR} && ${IMAKE} -m ${MFSBSD_MKINCLUDEDIR} \
${.MAKEFLAGS} MAKEJOBS=${MFSBSD_JOBS} WRKDIR=${MFSBSD_WRKDIR} \
SE=1 IMAGE=${.TARGET} BASE=${.OBJDIR} image
install -m 0644 ${MFSBSD_TMPDIR}/${.TARGET} .
mfsbsd-se.iso: mfsbsd-before-build mfsbsd-build mfsbsd-build-install
cd ${MFSBSD_TMPDIR} && ${IMAKE} -m ${MFSBSD_MKINCLUDEDIR} \
impUnsubmitted
Done
Inline Actions

Same comment as above

imp: Same comment as above
soobinrhoAuthorUnsubmitted
Done
Inline Actions

Acknowledged.

soobinrho: Acknowledged.
${.MAKEFLAGS} MAKEJOBS=${MFSBSD_JOBS} WRKDIR=${MFSBSD_WRKDIR} \
SE=1 ISOIMAGE=${.TARGET} BASE=${.OBJDIR} iso
install -m 0644 ${MFSBSD_TMPDIR}/${.TARGET} .
mfsbsd-before-build: ${MFSBSD_WRKDIR}/.tmp_done
${MFSBSD_WRKDIR}/.tmp_done:
# Copy mfsBSD source files to the temporary folder. This is necessary to
# prevent mfsBSD build process from writing to the src contrib directory.
mkdir -p ${MFSBSD_WRKDIR}
cp -rpf ${MFSBSD_DIR}/. ${MFSBSD_TMPDIR}/
touch ${MFSBSD_WRKDIR}/.tmp_done
.if !exists(${MFSBSD_PKGSTATIC})
# If pkg-static is not present at the device, mfsBSD build process
# cannot proceed further. Thus, creating .packages_done bypasses
# that specific part of the build process that requires pkg-static.
touch ${MFSBSD_WRKDIR}/.packages_done
.endif
mfsbsd-build: ${MFSBSD_WRKDIR}/.build_done
${MFSBSD_WRKDIR}/.build_done:
cd ${MFSBSD_SRCDIR} && ${IMAKE} \
impUnsubmitted
Done
Inline Actions

Why not use install(1) here?

imp: Why not use install(1) here?
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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

soobinrho: That's a really good idea. install(1) will be a much better implementation. Updating the code…
soobinrhoAuthorUnsubmitted
Done
Inline Actions
  • Updated to use install -m -644 <filename> instead of mv.
soobinrho: - Updated to use `install -m -644 <filename>` instead of `mv`.
${.MAKEFLAGS} -j${MFSBSD_JOBS} buildworld buildkernel
touch ${MFSBSD_WRKDIR}/.build_done
mfsbsd-build-install: ${MFSBSD_WRKDIR}/.install_done
${MFSBSD_WRKDIR}/.install_done:
${IMAKE} ${.MAKEFLAGS} -C ${.CURDIR} obj bootonly
cd ${MFSBSD_TMPDIR} && ${IMAKE} \
${.MAKEFLAGS} MAKEJOBS=${MFSBSD_JOBS} WRKDIR=${MFSBSD_WRKDIR} \
impUnsubmitted
Done
Inline Actions

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

imp: These checksums should be part of the build... not the install.
soobinrhoAuthorUnsubmitted
Done
Inline Actions

/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!

soobinrho: `/usr/src/release/Makefile.vm` and `/usr/src/release/Makefile` create checksum during install…
KERNCONF=${MFSBSD_KERNCONF} \
SE=1 BASE=${.OBJDIR} install
list-mfsbsd:
impUnsubmitted
Done
Inline Actions

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

imp: Isn't -C ${.CURDIR} redundant?
soobinrhoAuthorUnsubmitted
Done
Inline Actions

Agreed. Removing it and updating my code revision now.

soobinrho: Agreed. Removing it and updating my code revision now.
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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.

soobinrho: When `-C ${.CURDIR}` is removed, MAKE(1) gives an error saying the target doesn't exist. I've…
@${ECHO}
@${ECHO} "Supported mfsBSD image targets:"
.for TYPE in ${MFSBSD_TYPES}
. for FORMAT in ${MFSBSD_FORMATS}
@${ECHO} " ${TYPE}.${FORMAT}: ${${TYPE:tu}${FORMAT:tu}_DESC}"
. endfor
.endfor
mfsbsd-install:
impUnsubmitted
Done
Inline Actions

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

imp: I'd be tempted to add these to the CLEANFILES variables so clean-all works. I also don't see…
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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

soobinrho: Thank you! Taking out `rm ...`, and instead adding to `CLEANFILES`.
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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}.

soobinrho: Thanks again! I've removed the part where it's writing to the src contrib directory, and I've…
.if defined(WITH_MFSBSD) && !empty(WITH_MFSBSD)
mkdir -p ${DESTDIR}/mfsbsdimages
. for TYPE in ${MFSBSD_TYPES}
. for FORMAT in ${MFSBSD_FORMATS}
install -m 0644 ${TYPE}.${FORMAT} \
${DESTDIR}/mfsbsdimages/${OSRELEASE}-${TYPE}.${FORMAT}
. endfor
. endfor
cd ${DESTDIR}/mfsbsdimages && sha512 ${OSRELEASE}* > \
${DESTDIR}/mfsbsdimages/CHECKSUM.SHA512
cd ${DESTDIR}/mfsbsdimages && sha256 ${OSRELEASE}* > \
${DESTDIR}/mfsbsdimages/CHECKSUM.SHA256
.endif
mfsbsd-release:
.if defined(WITH_MFSBSD) && !empty(WITH_MFSBSD)
${IMAKE} -C ${.CURDIR} ${.MAKEFLAGS} mfsbsd-image
.endif
mfsbsd-clean:
cd ${MFSBSD_DIR} && env MAKEOBJDIR=${MFSBSD_TMPDIR} \
${IMAKE} ${.MAKEFLAGS} WRKDIR=${MFSBSD_WRKDIR} clean-all
impUnsubmitted
Done
Inline Actions

Newline.

imp: Newline.
soobinrhoAuthorUnsubmitted
Done
Inline Actions

Acknowledged. Correcting now.

soobinrho: Acknowledged. Correcting now.
View Options

usr.sbin/config/mkoptions.c

  • This file was added.
#
# $FreeBSD$
#
jrmUnsubmitted
Not Done
Inline Actions

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

jrm: $FreeBSD$ tags have been removed, so you can remove these lines.
soobinrhoAuthorUnsubmitted
Not Done
Inline Actions

Got it. Thanks!

soobinrho: Got it. Thanks!
#
# Makefile for building mfsBSD images.
#
MFSBSD_TYPES?= mfsbsd-se
MFSBSD_FORMATS?= img iso
MFSBSD-SEIMG_DESC= Special edition mfsBSD .img file including base.txz and kernel.txz
MFSBSD-SEISO_DESC= Special edition mfsBSD .iso file including base.txz and kernel.txz
impUnsubmitted
Not Done
Inline Actions

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

imp: Does mfsroot not support the dual image format we have support for in base for doing iso and…
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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.

soobinrho: I'll definitely research into that. It might be the case there's no support for a dual image…
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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.

soobinrho: As of now, there's no native support for dual image format in mfsBSD. Could you please point me…
#
# Paths
#
impUnsubmitted
Done
Inline Actions

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?

imp: Do you really need the realpath expansion here? It's generally not done elsewhere. Also I…
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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!

soobinrho: I'll test to see if we can do it without realpath expansion. Originally, there was an error…
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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: Sorry! I missed the second part of your question by mistake. Here's the link to the contrib…
MFSBSD_DIR?= ${WORLDDIR}/contrib/mfsbsd
MFSBSD_SRCDIR?= /usr/src
jrmUnsubmitted
Not Done
Inline Actions

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

jrm: Should we avoid hardcoding the path here? Could this be ${SRCTOP} or ${WORLDDIR}?
otisUnsubmitted
Not Done
Inline Actions

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)

otis: > Should we avoid hardcoding the path here? Could this be ${SRCTOP} or ${WORLDDIR}?
soobinrhoAuthorUnsubmitted
Not Done
Inline Actions

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

soobinrho: Great idea! Changing it from `/usr/src` to `${WORLDDIR}`.
MFSBSD_TMPDIR?= ${.OBJDIR}/../contrib/mfsbsd_tmp
MFSBSD_WRKDIR?= ${MFSBSD_TMPDIR}/work
MFSBSD_DISTDIR!= make -C ${MFSBSD_DIR} -V _DESTDIR WRKDIR=${MFSBSD_WRKDIR}
MFSBSD_ROOTDIR!= make -C ${MFSBSD_DIR} -V _ROOTDIR WRKDIR=${MFSBSD_WRKDIR}
jrmUnsubmitted
Not Done
Inline Actions

Are these two variables used?

jrm: Are these two variables used?
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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.

soobinrho: Thank you for noticing! Deleting `MFSBSD_DISTDIR` and `MFSBSD_ROOTDIR` now. For your reference…
MFSBSD_MKINCLUDEDIR?= /usr/share/mk
#
# Build defaults
#
MFSBSD_JOBS!= sysctl -n hw.ncpu
jrmUnsubmitted
Not Done
Inline Actions

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.

jrm: Do we want to default to using all the system's CPUs? I see you are including `${.MAKEFLAGS}`…
soobinrhoAuthorUnsubmitted
Not Done
Inline Actions

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

soobinrho: > Do we want to default to using all the system's CPUs? I see you are including `${.
MFSBSD_KERNCONF?= GENERIC
MFSBSD_PKGSTATIC?= /usr/local/sbin/pkg-static
.if defined(WITH_MFSBSD) && !empty(WITH_MFSBSD)
CLEANDIRS+= ${MFSBSD_TMPDIR}
. for TYPE in ${MFSBSD_TYPES}
. for FORMAT in ${MFSBSD_FORMATS}
CLEANFILES+= ${TYPE}.${FORMAT}
. endfor
. endfor
.endif
impUnsubmitted
Done
Inline Actions

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

imp: The indentation looks wrong. But it also looks misplaced. I'd expect something like this to be…
soobinrhoAuthorUnsubmitted
Done
Inline Actions

My bad. Correcting it now. Thank you!

soobinrho: My bad. Correcting it now. Thank you!
mfsbsd-base: mfsbsd-image
mfsbsd-image:
impUnsubmitted
Done
Inline Actions

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.

imp: I don't think this is -j safe. Also this shouldn't be created in MFSBSD_DIR since that is…
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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

soobinrho: Agreed. Correcting it to make it -j safe and also I'll make it to built at `obj` instead.
soobinrhoAuthorUnsubmitted
Done
Inline Actions
  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}`
soobinrho: 1. Now, the code writes to `obj` and never writes to `src/cotrib`. I've implemented it with `cp…
.if defined(WITH_MFSBSD) && !empty(WITH_MFSBSD)
. for TYPE in ${MFSBSD_TYPES}
. for FORMAT in ${MFSBSD_FORMATS}
${IMAKE} ${.MAKEFLAGS} -C ${.CURDIR} ${TYPE}.${FORMAT}
. endfor
. endfor
.endif
impUnsubmitted
Done
Inline Actions

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

imp: I'd either skip these checks or I'd invert them to be errors if either variable is blank
soobinrhoAuthorUnsubmitted
Done
Inline Actions

Makes sense. Thank you! Making changes accordingly now.

soobinrho: Makes sense. Thank you! Making changes accordingly now.
mfsbsd-se.img: mfsbsd-before-build mfsbsd-build mfsbsd-build-install
cd ${MFSBSD_TMPDIR} && ${IMAKE} -m ${MFSBSD_MKINCLUDEDIR} \
${.MAKEFLAGS} MAKEJOBS=${MFSBSD_JOBS} WRKDIR=${MFSBSD_WRKDIR} \
SE=1 IMAGE=${.TARGET} BASE=${.OBJDIR} image
install -m 0644 ${MFSBSD_TMPDIR}/${.TARGET} .
mfsbsd-se.iso: mfsbsd-before-build mfsbsd-build mfsbsd-build-install
cd ${MFSBSD_TMPDIR} && ${IMAKE} -m ${MFSBSD_MKINCLUDEDIR} \
impUnsubmitted
Done
Inline Actions

Same comment as above

imp: Same comment as above
soobinrhoAuthorUnsubmitted
Done
Inline Actions

Acknowledged.

soobinrho: Acknowledged.
${.MAKEFLAGS} MAKEJOBS=${MFSBSD_JOBS} WRKDIR=${MFSBSD_WRKDIR} \
SE=1 ISOIMAGE=${.TARGET} BASE=${.OBJDIR} iso
install -m 0644 ${MFSBSD_TMPDIR}/${.TARGET} .
mfsbsd-before-build: ${MFSBSD_WRKDIR}/.tmp_done
${MFSBSD_WRKDIR}/.tmp_done:
# Copy mfsBSD source files to the temporary folder. This is necessary to
# prevent mfsBSD build process from writing to the src contrib directory.
mkdir -p ${MFSBSD_WRKDIR}
cp -rpf ${MFSBSD_DIR}/. ${MFSBSD_TMPDIR}/
touch ${MFSBSD_WRKDIR}/.tmp_done
.if !exists(${MFSBSD_PKGSTATIC})
# If pkg-static is not present at the device, mfsBSD build process
# cannot proceed further. Thus, creating .packages_done bypasses
# that specific part of the build process that requires pkg-static.
touch ${MFSBSD_WRKDIR}/.packages_done
.endif
mfsbsd-build: ${MFSBSD_WRKDIR}/.build_done
${MFSBSD_WRKDIR}/.build_done:
cd ${MFSBSD_SRCDIR} && ${IMAKE} \
impUnsubmitted
Done
Inline Actions

Why not use install(1) here?

imp: Why not use install(1) here?
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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

soobinrho: That's a really good idea. install(1) will be a much better implementation. Updating the code…
soobinrhoAuthorUnsubmitted
Done
Inline Actions
  • Updated to use install -m -644 <filename> instead of mv.
soobinrho: - Updated to use `install -m -644 <filename>` instead of `mv`.
${.MAKEFLAGS} -j${MFSBSD_JOBS} buildworld buildkernel
touch ${MFSBSD_WRKDIR}/.build_done
mfsbsd-build-install: ${MFSBSD_WRKDIR}/.install_done
${MFSBSD_WRKDIR}/.install_done:
${IMAKE} ${.MAKEFLAGS} -C ${.CURDIR} obj bootonly
cd ${MFSBSD_TMPDIR} && ${IMAKE} \
${.MAKEFLAGS} MAKEJOBS=${MFSBSD_JOBS} WRKDIR=${MFSBSD_WRKDIR} \
impUnsubmitted
Done
Inline Actions

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

imp: These checksums should be part of the build... not the install.
soobinrhoAuthorUnsubmitted
Done
Inline Actions

/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!

soobinrho: `/usr/src/release/Makefile.vm` and `/usr/src/release/Makefile` create checksum during install…
KERNCONF=${MFSBSD_KERNCONF} \
SE=1 BASE=${.OBJDIR} install
list-mfsbsd:
impUnsubmitted
Done
Inline Actions

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

imp: Isn't -C ${.CURDIR} redundant?
soobinrhoAuthorUnsubmitted
Done
Inline Actions

Agreed. Removing it and updating my code revision now.

soobinrho: Agreed. Removing it and updating my code revision now.
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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.

soobinrho: When `-C ${.CURDIR}` is removed, MAKE(1) gives an error saying the target doesn't exist. I've…
@${ECHO}
@${ECHO} "Supported mfsBSD image targets:"
.for TYPE in ${MFSBSD_TYPES}
. for FORMAT in ${MFSBSD_FORMATS}
@${ECHO} " ${TYPE}.${FORMAT}: ${${TYPE:tu}${FORMAT:tu}_DESC}"
. endfor
.endfor
mfsbsd-install:
impUnsubmitted
Done
Inline Actions

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

imp: I'd be tempted to add these to the CLEANFILES variables so clean-all works. I also don't see…
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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

soobinrho: Thank you! Taking out `rm ...`, and instead adding to `CLEANFILES`.
soobinrhoAuthorUnsubmitted
Done
Inline Actions

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}.

soobinrho: Thanks again! I've removed the part where it's writing to the src contrib directory, and I've…
.if defined(WITH_MFSBSD) && !empty(WITH_MFSBSD)
mkdir -p ${DESTDIR}/mfsbsdimages
. for TYPE in ${MFSBSD_TYPES}
. for FORMAT in ${MFSBSD_FORMATS}
install -m 0644 ${TYPE}.${FORMAT} \
${DESTDIR}/mfsbsdimages/${OSRELEASE}-${TYPE}.${FORMAT}
. endfor
. endfor
cd ${DESTDIR}/mfsbsdimages && sha512 ${OSRELEASE}* > \
${DESTDIR}/mfsbsdimages/CHECKSUM.SHA512
cd ${DESTDIR}/mfsbsdimages && sha256 ${OSRELEASE}* > \
${DESTDIR}/mfsbsdimages/CHECKSUM.SHA256
.endif
mfsbsd-release:
.if defined(WITH_MFSBSD) && !empty(WITH_MFSBSD)
${IMAKE} -C ${.CURDIR} ${.MAKEFLAGS} mfsbsd-image
.endif
mfsbsd-clean:
cd ${MFSBSD_DIR} && env MAKEOBJDIR=${MFSBSD_TMPDIR} \
${IMAKE} ${.MAKEFLAGS} WRKDIR=${MFSBSD_WRKDIR} clean-all
impUnsubmitted
Done
Inline Actions

Newline.

imp: Newline.
soobinrhoAuthorUnsubmitted
Done
Inline Actions

Acknowledged. Correcting now.

soobinrho: Acknowledged. Correcting now.