Page MenuHomeFreeBSD

Normalize deployment tools usage and definitions
ClosedPublic

Authored by sobomax on Jun 5 2019, 5:32 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 11, 12:28 PM
Unknown Object (File)
Mon, Mar 11, 12:28 PM
Unknown Object (File)
Mon, Mar 11, 12:28 PM
Unknown Object (File)
Mon, Mar 11, 12:28 PM
Unknown Object (File)
Mon, Mar 11, 12:28 PM
Unknown Object (File)
Mon, Mar 11, 12:28 PM
Unknown Object (File)
Fri, Mar 8, 12:43 AM
Unknown Object (File)
Jan 23 2024, 9:25 AM

Details

Summary

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
#!/bin/sh

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
do
  if [ ${t} = "distrib-dirs" ]
  then
    cleanup_warea
  fi
  sudo make DESTDIR="${WAREA}" ${t}
done

sudo rm -r "${WAREA}"

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

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 24693

Event Timeline

share/mk/sys.mk
241

In one line:

INSTALL ?= ${INSTALL_CMD:Uinstall}

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.

Makefile.inc1
892

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

893

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

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

MTREE_CMD= should be used here to be properly consistent

910

MTREE_CMD in these two

share/mk/sys.mk
241

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

share/mk/src.bootools.mk
1

This is a terrible name. src.tools.mk 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.

src.bootools.mk -> src.tools.mk as suggested by @imp

sobomax added inline comments.
Makefile.inc1
892

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

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

share/mk/src.bootools.mk
1

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

share/mk/sys.mk
241

@rgrimes Well, it's not that we need indirection, it's just I think because sys.mk 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 src.tools.mk in there, so this is a quick way to make it obey and use proper override when we are doing installkernel.

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 src.tools.mk 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.

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

Makefile.inc1
907

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.

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!

I need to think through the sys.mk 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.

share/mk/sys.mk
240

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

share/mk/sys.mk
240

Perhaps because it won't work?

sys.mk is read before anything else, so ?= is often meaningless unless a makefile read by sys.mk has already set a value, or a parent has exported a value to environment.
Of course we do support local*sys.mk 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.

share/mk/sys.mk
240

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

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

Suggested by: bapt

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

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