Page MenuHomeFreeBSD
Differential D8378

Get rid of the QT_FOODIR plist subs
ClosedPublic
Actions

Authored by tcberner on Oct 28 2016, 7:32 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 15, 4:57 PM
Unknown Object (File)
Mon, Apr 14, 7:25 AM
Unknown Object (File)
Mar 20 2025, 1:35 AM
Unknown Object (File)
Mar 12 2025, 12:23 AM
Unknown Object (File)
Mar 10 2025, 10:29 PM
Unknown Object (File)
Feb 21 2025, 6:29 PM
Unknown Object (File)
Jan 24 2025, 10:43 AM
Unknown Object (File)
Jan 20 2025, 12:43 AM
View All Files
Subscribers
adridg

Details

Reviewers
rakuco
mat
Group Reviewers
O5: Ports Framework(Owns No Changed Paths)
portmgr
kde
Commits
rP427356: Stop exporting QT_BINDIR and QT_LIBDIR to PLIST_SUB.
Summary

Very few non-Qt-dist ports actually needed to use the plist subs for
QT_BINDIR, QT_LIBDIR and so on. However, as they were exported by bsd.qt.mk,
make makeplist freely replaced every occurence of bin or lib with
said subs.

There is however no straight-forward way to decide whether a sub is sensible or
not without manually looking at the created plist and the projects buildscripts,
followed by thinking about it (does it actually install to `where the Qt
binaries go`, or just bin/).

Therefore simply get rid of all the QT_FOODIR subs.

Note1: Maybe this is too aggressive, and we could only rid ourselves of
QT_BINDIR and QT_LIBDIR as they are the most egregiously misused ones.
But, I think ripping them all out at once is the cleanest way to handle this.

Note2: As there are no changes apart from plist rewrites, I don't think any version
needs to be bumped.

Note3: It could be, that some ports (or slaveports) that have QT4&QT5 options
and used %%QT_FOO%% are broken by this -- they need to define the PLIST_SUB
now manually, like multimedia/gstreamer1-qt[45] and math/vtk6.

[diff created using 'svn diff --diff-cmd diff -x -U99999', as arc choked on it]

Test Plan
  • This of course will require an exp-run :)
  • I test built all affected ports -- I did however not chase options.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 6082
Build 6345: arc lint + arc unit

Event Timeline

tcberner updated this revision to Diff 21781.Oct 28 2016, 7:32 PM
tcberner retitled this revision from to Get rid of the QT_FOODIR plist subs.
tcberner updated this object.
tcberner edited the test plan for this revision. (Show Details)
tcberner added reviewers: kde, mat, rakuco.
tcberner set the repository for this revision to rP FreeBSD ports repository.
tcberner added a subscriber: adridg.
Herald added a reviewer: portmgr. · View Herald TranscriptOct 28 2016, 7:32 PM
tcberner added a comment.Oct 28 2016, 7:38 PM

As a "lighter" version of this diff, we could keep exporting the QT_FOODIR subs whenever QT_DIST is defined. That way the qt[45]-ports would not be affected by this, and the diff would be more managable.

mat edited edge metadata.Oct 28 2016, 11:05 PM

Mmmm, I was only talking about removing QT_BINDIR and QT_LIBDIR, because both are way too short. The other are fine.

tcberner added a comment.Oct 29 2016, 6:58 AM

Misunderstandings are fun, aren't they :D

In that case, the whole thing is moot.

With qt-5.7 I plan to move the qt5 libraries to lib/qt5. QT_LIBDIR therefore becomes lib/qt5 and QT_BINDIR for qt4 and qt5 will become ${QT_LIBDIR}/bin [or maybe bin/${_QT_RELMAME}].

So they won't get plist-subbed in anymore then if not actually needed anyway.

mat added a comment.Oct 29 2016, 9:12 AM

I don't quite like having QT_BINDIR not being in PREFIX/bin, it means its content will not be in PATH, and not readily accessible. (unless you add symlinks to PREFIX/bin, in which case I don't really see the point of them being some place else anyway.)

tcberner added a comment.Oct 29 2016, 9:17 AM

misc/qtchooser will be used which installs to ${LOCALBASE}/bin and selects the proper qmake /uic/moc/foo depending on the environment.

mat added a comment.Oct 30 2016, 9:54 AM

So, this means people working on QT apps, when they want to build with qt4 or qt5, they'll have to sudo and run some software ?

tcberner added a comment.Oct 30 2016, 11:18 AM

misc/qtchooser installs wrapper binaries that select the correct non-wrapped binary depending on the value of QT_SELECT (which has to be set to an installed qt-profile [1]).

% env QT_SELECT=qt4 qmake -v
QMake version 2.01a
Using Qt version 4.8.7 in /usr/local/lib/qt4

% env QT_SELECT=qt5 qmake -v
QMake version 3.0
Using Qt version 5.7.1 in /usr/local/lib/qt5

This means with that change, users could set their default QT_SELECT in their shell-profile, and qmake would simply give them the correct one. Unlike now, where you have to decide whether you want /usr/local/bin/qmake-qt4 or /usr/local/lib/qt5/bin/qmake.

For the ports, bsd.qt.mk/qt.mk will just choose the correct profile.

This also allows people that work on Qt itself to have multiple version of Qt installed simultaneously and build against the proper one by providing a new qt-foo profile [1].

[1] The profiles currently installed are

% cat /usr/local/etc/xdg/qtchooser/qt4.conf
/usr/local/lib/qt4/bin
/usr/local/lib/qt4
% cat /usr/local/etc/xdg/qtchooser/qt5.conf
/usr/local/lib/qt5/bin
/usr/local/lib/qt5

A person working on Qt could simply add a profile ala

% cat qt58.conf 
/home/Tobias/qt-devel/qt-5.8/bin
/home/Tobias/qt-devel/qt-5.8/lib

and use it via env QT_SELECT=qt58 qmake.

tcberner updated this revision to Diff 22068.Nov 8 2016, 1:13 AM
tcberner edited the test plan for this revision. (Show Details)
tcberner edited edge metadata.

Let's do this completely minimal...

Only export QT_BINDIR and QT_LIBDIR if
QT_DIST is set.

Thereby all the Qt-ports themselves are unaffected by
this change, and non Qt-ports will no longer pick them
up by mistake.

Owners added a reviewer: O5: Ports Framework.Nov 8 2016, 1:13 AM
Herald edited edge metadata. · View Herald TranscriptNov 8 2016, 1:13 AM
tcberner added a comment.Nov 26 2016, 1:22 PM

ping @mat pong @rakuco

rakuco accepted this revision.Nov 26 2016, 1:54 PM
rakuco edited edge metadata.

Looks fine to me, but I'd drop the whitespace changes in bsd.qt.mk. They make the patch bigger, the history more difficult to navigate through and moving to Uses/qt.mk will make these changes short-lived.

mat added a comment.Nov 28 2016, 3:36 PM

Yes, drop the whitespace change, you will be able to use Tools/scripts/indent_make_if.pl when moving to USES=qt.

tcberner updated this revision to Diff 22579.Nov 28 2016, 5:08 PM
tcberner edited edge metadata.

Get rid of the changes made by Tools/scripts/indent_make_if.pl .

Owners edited edge metadata.Nov 28 2016, 5:08 PM
Herald edited edge metadata. · View Herald TranscriptNov 28 2016, 5:08 PM
rakuco accepted this revision.Nov 28 2016, 8:32 PM
rakuco edited edge metadata.

lgtm!

tcberner updated this revision to Diff 22589.Nov 28 2016, 9:26 PM
tcberner edited edge metadata.

An other grep for QT_BINDIR and QT_LIBDIR brought some new wrong uses.
They are fixed now too.

Owners edited edge metadata.Nov 28 2016, 9:26 PM
Herald edited edge metadata. · View Herald TranscriptNov 28 2016, 9:26 PM
mat accepted this revision.Nov 28 2016, 9:56 PM
mat edited edge metadata.

Macro shipit:

This revision is now accepted and ready to land.Nov 28 2016, 9:56 PM
Closed by commit rP427356: Stop exporting QT_BINDIR and QT_LIBDIR to PLIST_SUB. (authored by tcberner). · Explain WhyNov 29 2016, 6:20 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
Mk/
9 lines
cad/
qelectrotech/
20 lines
devel/
qscintilla2-qt5/
8 lines
qscintilla2/
8 lines
qt4-libqtassistantclient/
10 lines
graphics/
gle-graphics/
8 lines
math/
vtk6/
2 lines
multimedia/
gstreamer-qt4/
4 lines
phonon/
8 lines
qt4-mobility/
116 lines
sysutils/
qzeitgeist/
4 lines
x11-themes/
qt5-style-plugins/
2 lines

Diff 22579

View Options

Mk/bsd.qt.mk

  • 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

cad/qelectrotech/pkg-plist

  • 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

devel/qscintilla2-qt5/pkg-plist

  • 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

devel/qscintilla2/pkg-plist

  • 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

devel/qt4-libqtassistantclient/pkg-plist

Loading...
View Options

graphics/gle-graphics/pkg-plist

Loading...
View Options

math/vtk6/pkg-plist

Loading...
View Options

multimedia/gstreamer-qt4/pkg-plist

Loading...
View Options

multimedia/phonon/pkg-plist

Loading...
View Options

multimedia/qt4-mobility/pkg-plist

Loading...
View Options

sysutils/qzeitgeist/pkg-plist

Loading...
View Options

x11-themes/qt5-style-plugins/pkg-plist

Loading...