Page MenuHomeFreeBSD

Normalize deployment tools usage and definitions

Authored by sobomax on Jun 5 2019, 5:32 AM.



Normalize deployment tools usage and definitions by putting into one place instead of sprinkling them out over many disjoint files. This is a follow-up to achieve the same goal in an incomplete rev.348521.

Test Plan

set -e

WAREA="`mktemp -d /tmp/bootools.XXXXXXX`"

cleanup_warea() {
  sudo chflags -R noschg "${WAREA}"
  sudo rm -r ${WAREA}/*

for t in installworld distribution installkernel distrib-dirs
  if [ ${t} = "distrib-dirs" ]
  sudo make DESTDIR="${WAREA}" ${t}

sudo rm -r "${WAREA}"

echo "All looks good, have a wonderful day! :)"

Diff Detail

rS FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sobomax created this revision.Jun 5 2019, 5:32 AM
sobomax edited the test plan for this revision. (Show Details)Jun 5 2019, 5:32 AM
bapt added inline comments.Jun 5 2019, 6:05 AM
241 ↗(On Diff #58258)

In one line:


I am not really keen on adding yet another top level .mk file that sets command names, much of this is caused by scatering etc/Makefile contents and rules about the tree when it was and should of been maintained as the one place for all of this and simply called as a submake. Perhaps it makes since anymore, perhaps not. But certainly having INSTALL and INSTALL_CMD default to and 99.999% of the time be "install" is not a good idea, same for MTREE and MTREE_CMD, they are being used in mixed manners, this needs to be sorted out asap and just use
MTREE_CMD and untwist some of this MTREE overload that should really overlaod MTREE_CMD.

892 ↗(On Diff #58258)

I repeat again, this should just be ${INSTALL} and so far ngie has made no valid assertion that it should be otherwise.

893 ↗(On Diff #58258)

Anyone having recersion issues with this? I know it well work but now we are overloading what MTREE_CMD is to mean. Is it the command only "mtree", or is it the command with the flags?

I am also pretty sure this IMAKE_foo and all the related DESTDIR/WORLDTMP stuff should be pushed back down in etc/Makefile and not be in this Makefile.inc1. That would also push and cleanup much of the related stuff here.

905 ↗(On Diff #58258)

Please no, MTREE is used elsewhere, the original source of this stuff here infact, and is the list of etc/mtree/* files that are to be processed. And again 2 variables that end up being "mtree" should always be colapsed to 1 variable. MTREE was already taken, the correct variable for this is then MTREE_CMD and MTREE has another meaning.

907 ↗(On Diff #58258)

MTREE_CMD= should be used here to be properly consistent

910 ↗(On Diff #58258)

MTREE_CMD in these two

241 ↗(On Diff #58258)

Why do we need to add a level of indirection here? Having 2 variables, INSTALL and INSTALL_CMD that are install seems very odd.

imp requested changes to this revision.Jun 5 2019, 3:35 PM

bootools is a terrible name, so bad I'm ticking 'request changes'.

1 ↗(On Diff #58258)

This is a terrible name. is fine. We don't use 'boo' elsewhere in the tree and I don't see it adding any value at all, only confusion, because these aren't boot tools.

Also, this file needs a proper copyright / header

This revision now requires changes to proceed.Jun 5 2019, 3:35 PM
In D20520#443376, @imp wrote:

bootools is a terrible name, so bad I'm ticking 'request changes'.,

I agree here, but did not have a better name so opted to say nothing.

Also, this file needs a proper copyright / header

Actually Makefile and .mk files generally fall under the rule of "receipes" and are not copyrightable, though it seems as sjg/Juniper has asserted copyrights on some of them, which could be challenged and possibly removed. This new file could still use a header.

sobomax updated this revision to Diff 58269.Jun 5 2019, 4:28 PM -> as suggested by @imp

sobomax marked an inline comment as done.Jun 5 2019, 5:24 PM
sobomax added inline comments.
892 ↗(On Diff #58258)

Well, unlike original commit this change deliberately added INSTALL_CMD for consistency with other tools. The expectation is that XYZ_CMD might be something one can fiddle with, while XYZ could be whatever build system needs to do its magic.

893 ↗(On Diff #58258)

@rgrimes in our use case we pass arguments into _CMD override already, in fact we rely on it. So distinction is pretty much nil in practice. Could be either. This might look bit odd, but it works.

I would not really feel comfortable touching IMAKE_foo with a six-foot pole. Someone with better understanding of the build system and way more time available is needed to tackle that.

1 ↗(On Diff #58258)

No other file has a (c) in them. Most are much more beefier in terms of number of code/logic. Rename's done.

241 ↗(On Diff #58258)

@rgrimes Well, it's not that we need indirection, it's just I think because is kinda self-contained (or pretends to be anyway), so it contains its own (re-)definition of pretty much every tool under the sun. It's probably for a good reason to allow compiling out-of-the tree kmods. I did not feel brave enough to add .include in there, so this is a quick way to make it obey and use proper override when we are doing installkernel.

sobomax marked an inline comment as done.Jun 5 2019, 5:32 PM
In D20520#443376, @imp wrote:

bootools is a terrible name, so bad I'm ticking 'request changes'.

Well, my fear is that we just call it people will make a dump out of it by throwing more unrelated stuff in there. But well, I have been in absolute minority on this it seems.

sobomax updated this revision to Diff 58281.Jun 5 2019, 8:29 PM

Rename MTREE -> DISTR_MTREE for clarity and in hope to make @rgrimes happier.

sobomax added inline comments.Jun 5 2019, 8:47 PM
907 ↗(On Diff #58258)

I respectfully disagree with this. The whole point of having MAKE_CMD is that it should never be thrown away, especially in a file that might be included into other places. If the build system needs to use something else for a particular target (${WORLDTMP}/legacy/usr/sbin/mtree) at some point, it then should construct a new variable out and use that. I've renamed variable to clarify its scope.

sobomax planned changes to this revision.Jul 1 2019, 8:11 PM

Hey @imp can you please review the latest revision and confirm that it looks OK now?

sobomax requested review of this revision.EditedJul 1 2019, 8:13 PM

I will keep it at reviews for 1 week and then it will land unless major objections are received. Thanks!

imp added a comment.Jul 1 2019, 9:48 PM

I need to think through the change, and I've got no time today to do that. I'll try to articulate something coherent (one way or the other) by the end of the day tomorrow since I know what I've said so far isn't actionable.

240 ↗(On Diff #58281)

This gives me heartburn. Give me a day or so to articulate why please.

sjg added inline comments.Jul 1 2019, 10:44 PM
240 ↗(On Diff #58281)

Perhaps because it won't work? is read before anything else, so ?= is often meaningless unless a makefile read by has already set a value, or a parent has exported a value to environment.
Of course we do support local* so this can work.
If the indirection is desired, then as bapt says a single line would be neater

imp requested changes to this revision.Jul 1 2019, 11:36 PM

Please make the change that bapt suggested.

240 ↗(On Diff #58281)

Yea, that's why... I just came to that conclusion after looking at something else. I concur: let's change this to babt's one liner because it could work because it isn' until ${INSTALL} is expanded do we also expand ${INSTALL_CMD:Uinstall}.

This revision now requires changes to proceed.Jul 1 2019, 11:36 PM
sobomax updated this revision to Diff 68506.Feb 18 2020, 9:21 PM

Collapse .if/.else/.endif into one liner.

Suggested by: bapt

sobomax marked 5 inline comments as done.Feb 18 2020, 9:24 PM

@imp the requested changes have been applied. Please check. Thanks!

@imp any chance you can review and see if the change can land or indicate what other changes are necessary. Thanks!

imp accepted this revision.Mar 29 2020, 8:44 PM

Like the changes I requested, and I'm not seeing anything else that looks weird...

This revision is now accepted and ready to land.Mar 29 2020, 8:44 PM