Page MenuHomeFreeBSD

packages: Convert world to a subdir build
Needs ReviewPublic

Authored by ivy on Wed, Mar 25, 4:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 30, 11:39 AM
Unknown Object (File)
Mon, Mar 30, 6:41 AM
Unknown Object (File)
Mon, Mar 30, 6:35 AM
Unknown Object (File)
Sun, Mar 29, 6:20 AM
Unknown Object (File)
Sun, Mar 29, 6:03 AM
Unknown Object (File)
Sun, Mar 29, 12:33 AM
Unknown Object (File)
Fri, Mar 27, 6:14 PM
Unknown Object (File)
Fri, Mar 27, 6:04 PM

Details

Reviewers
gnn
brooks
sjg
Group Reviewers
pkgbase
Summary

Instead of driving the world package build from Makefile.inc1,
use a subdir build where each package has a subdirectory under
packages/ using the new <bsd.pkg.mk>.

Convert some metadata that was previously in the UCL files (e.g.
sets and dependencies) to Makefile variables.

Build the packages under objdir (not repodir), and use the new
stagepackages target to copy them to repodir when creating the
repository.

There are a few advantages to doing it this way:

  • The package build works more like the rest of the build system, so it's more accessible to developers.
  • We can customise the packages we build based on src.conf options, e.g. skipping a package entirely, or adjusting its dependencies based on what it actually requires.
  • We have a specific list of packages that we want to build, and an unexpectedly missing package results in a build error, instead of silently producing a broken repository.
  • It's possible to build (and in the future, install) an individual package without having to rebuild the entire repository.

This doesn't apply to the dtb, kernel-* or src-* packages; those
have their own build systems in Makefile.inc1 and will be converted
later.

MFC after: 4 weeks (stable/15 only)
Sponsored by: https://www.patreon.com/bsdivy

Diff Detail

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

Event Timeline

ivy requested review of this revision.Wed, Mar 25, 4:27 PM

@jlduran, @imp: this may impact what you're working on for nanobsd.

i've tested via artefact inspection that this produces the same packages we currently build. there are quite a few improvements we could make to the build process (ideally, removing generate-ucl.lua entirely) but i want to get the basics in first.

In D56087#1282790, @ivy wrote:

@jlduran, @imp: this may impact what you're working on for nanobsd.

Thank you for keeping us in the loop. This looks great!
I will hold D56070 until this has landed.
Please also tag me when you'll submit the equivalent change for dtb and kernel. I have a few minor patches for those as well, but I'll better wait until you finish.
I have briefly tested this patch, and I'm getting:

/usr/libexec/flua: /usr/src/release/packages/generate-ucl.lua:77: attempt to call a nil value (method 'match')

As if the previous clean step did something it should not do (it dirtied the repo). Please disregard if I'm doing something evidently wrong.

I have briefly tested this patch, and I'm getting:

/usr/libexec/flua: /usr/src/release/packages/generate-ucl.lua:77: attempt to call a nil value (method 'match')

As if the previous clean step did something it should not do (it dirtied the repo). Please disregard if I'm doing something evidently wrong.

i will try to reproduce this here, but in the mean time, could you please share a full make buildworld / buildkernel / update-packages log?

In D56087#1282872, @ivy wrote:

I have briefly tested this patch, and I'm getting:

/usr/libexec/flua: /usr/src/release/packages/generate-ucl.lua:77: attempt to call a nil value (method 'match')

As if the previous clean step did something it should not do (it dirtied the repo). Please disregard if I'm doing something evidently wrong.

i will try to reproduce this here, but in the mean time, could you please share a full make buildworld / buildkernel / update-packages log?

I'm preparing the logs. In the meantime, the only thing I did different was that I ran make packages, not update-packages.

In D56087#1282872, @ivy wrote:

I have briefly tested this patch, and I'm getting:

/usr/libexec/flua: /usr/src/release/packages/generate-ucl.lua:77: attempt to call a nil value (method 'match')

As if the previous clean step did something it should not do (it dirtied the repo). Please disregard if I'm doing something evidently wrong.

i will try to reproduce this here, but in the mean time, could you please share a full make buildworld / buildkernel / update-packages log?

https://people.freebsd.org/~jlduran/

brooks added subscribers: sjg, brooks.

I like the overall change. I've at most skimmed the makefile bits, but this seems like a move in the right direction.

Tagging @sjg in case he has any thoughts on this. IIRC he has previously said that driving package creation from individual directories was required to support dirdeps packages so hopefully this is a move in the right direction.

packages/acct/Makefile
3

I find myself wondering if dbg should be added by default since it's nearly universal.

This revision is now accepted and ready to land.Thu, Mar 26, 9:34 AM

it looks like the clean step is going wrong, and cleans the source directory instead of the obj directory; i assume this happens in case the obj directory doesn't exist. i couldn't get this to reproduce the error you hit, but it might be the same cause... i'll see about the best way to fix this and update the diff. (also, i realised we don't handle WITH_MANSPLITPKG or WITHOUT_* flags correctly right now, so i need to fix that too.)

i'll probably make SUBPACKAGES?=dbg man the default, and individual packages can override that if they don't produce one or the other packages. the man package will be automatically disabled if WITHOUT_MANSPLITPKG, whihc is the default.

I like the overall change. I've at most skimmed the makefile bits, but this seems like a move in the right direction.

Tagging @sjg in case he has any thoughts on this. IIRC he has previously said that driving package creation from individual directories was required to support dirdeps packages so hopefully this is a move in the right direction.

Indeed it is. I've mainly looked at the share/mk bits, there's probably scope for improvement - at least if using DIRDEPS_BUILD, but this is a *big* step towards making that possible.

Thanks Ivy.

what is the difference b/w packages/ and release/packages/ ?

it would be trivial to add packages/ and/or release/packages/ to the list of places dirdeps-targets.mk searches for "top-level" targets in DIRDEPS_BUILD.

In D56087#1283195, @sjg wrote:

what is the difference b/w packages/ and release/packages/ ?

originally we only had release/packages, but i've moved the new bits to packages/ because packages aren't really a release thing, they're part of the src build. the remaining files in release/packages will go away when i redo the kernel/dtb and src packages.

In D56087#1283196, @ivy wrote:
In D56087#1283195, @sjg wrote:

what is the difference b/w packages/ and release/packages/ ?

originally we only had release/packages, but i've moved the new bits to packages/ because packages aren't really a release thing, they're part of the src build. the remaining files in release/packages will go away when i redo the kernel/dtb and src packages.

Sounds good.

about dirdeps: i'm hoping this will allow us to eventually support a dirdeps build for packages, but that's a long-term plan and not something i'm working on right now. but if there's anything we can change here to make that easier in the future (especially if it changes the developer-facing interface) i can definitely look at doing that now.

In D56087#1283209, @ivy wrote:

about dirdeps: i'm hoping this will allow us to eventually support a dirdeps build for packages, but that's a long-term plan and not something i'm working on right now. but if there's anything we can change here to make that easier in the future (especially if it changes the developer-facing interface) i can definitely look at doing that now.

Yes, it should help a lot. There are a number of tweaks that could be made when doing DIRDEPS_BUILD (eg. staging of packages would be automagic), so perhaps the stagepackages target might be redundant in that case, but for now it is fine to have this work without any dependence on dirdeps.

improve the objdir build

objdir wasn't working correctly for packages because it's not hooked up
to the main build in Makefile, so it wasn't getting the normal pre-created
objdirs, and the build artefacts would end up in src/packages/ instead.

this interacted with ${CLEANDIRS} in a way that made it also delete source
files when running update-packages.

work around this by:

  • renaming the generated *.ucl to *.pkgucl, so there's no overlap in file extensions between src and obj
  • forcing MK_AUTO_OBJ=yes for the package build, so that objdirs are created when required.

there might be a better way to handle this, but i don't think we're ready
to add packages/ as a proper top-level subdirectory yet (in particular,
this would make it impossible to build src without pkg installed).

@jlduran could you please check if this fixes the issue you ran into it?

this doesn't address the other things i mentioned, so it's not the final
form of this commit.

This revision now requires review to proceed.Fri, Mar 27, 12:50 PM
packages/acct/Makefile
2

Curious - why WORLDPACKAGE rather than just PACKAGE ?

packages/acct/Makefile
2

i would have preferred PACKAGE, but this is already used for things that use <bsd.prog.mk> or <bsd.lib.mk> and i thought it might be confusing to use the same variable name for both.

Thank you! It works well now (overall). There are a couple of things that fail on aarch64 (described inline). Other than that, this is great. Thank you!

packages/Makefile
74

There is no libipt package on aarch64, so I removed it.

84

There is no libvgl package on aarch64, so I removed it.

packages/apm/Makefile
3

There is no apm-dbg package on aarch64.

packages/bootloader/Makefile
3

There is no bootloader-dbg package on aarch64.

Thank you! It works well now (overall). There are a couple of things that fail on aarch64 (described inline). Other than that, this is great. Thank you!

locally, i've converted packages/Makefile to use <bsd.arch.inc.mk> and am going through it to make sure we build the right things in the right platform, i'll update the diff for this later.

fix remaining issues

  • support WITH_MANSPLITPKG. the default for SUBPACKAGES is now "dbg man"; either/both of those are removed based on the corresponding src.conf options. (suggested by brooks)
  • use <bsd.arch.inc.mk> to build arch-specific packages. this avoids trying to build packages that only exist on some platforms. (reported by jlduran)

this isn't quite the final form of this diff; i need to go through the
non-amd64 archs and add the appropriate Makefile.${MACHINE}, but those
are mechanical changes that i won't update the diff for unless any
other problems crop up.

in terms of review, this diff is quite large, but most of it is package
Makefiles which are all broadly similar. i'd suggest the most important
bits to review are:

  • Makefile.inc1
  • packages/Makefile*
  • share/mk/bsd.pkg.mk, share/mk/bsd.pkg.pre.mk
  • share/mk/bsd.subdir.mk

i'll land this in main in one week unless anyone objects (or we find
other problems that need to be fixed, in which case i'll do another
review cycle).

Nice! I'm sorry, but I found one very minor thing. REPODIR is unset in bsd.pkg.mk, therefore creating the repo directory under /, I documented inline what I did in order to make it work.
Besides this minor issue and the missing machine-specific Makefiles you mentioned, everything else is superb!

Makefile.inc1
2267
share/mk/bsd.pkg.mk
21

Should there also be a guard for REPODIR?

40

REPODIR is unset.