Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makefile.inc1: allow real-update-packages to be called independently #1445

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

svmhdvn
Copy link
Contributor

@svmhdvn svmhdvn commented Oct 2, 2024

To perform an incremental update of a pkgbase repo, you would call 'make update-packages', which will stage, create, and incrementally choose newer package versions to sign as part of a pkg repo. However, this forces you to stage the kernel and source packages along with the world packages. For a jail-only installation of FreeBSD, these packages are generally not required.

This patch separates the 'update-packages' target from the 'real-update-packages' target to allow choosing world, kernel, and/or source individually at the command line like so:

Jail-only installation:

make [...] -j$(nproc) buildworld
make [...] PKG_VERSION='15.snap<date>' \
    stage-packages-world create-packages-world \
    real-update-packages

Other minor changes are included to fix the PKG_VERSION variable definition logic and order of operations.

Makefile.inc1 Outdated
@@ -571,7 +571,6 @@ VERSION= FreeBSD ${_REVISION}-${_BRANCH:C/-p[0-9]+$//} ${TARGET_ARCH} ${SRCRELDA
.endif
MAJOR_REVISION= ${_REVISION:R}

.if !defined(PKG_VERSION)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The below variables should always be defined regardless of what PKG_VERSION is set to, since it is used to compare two repos for incremental updates.

@@ -593,9 +592,8 @@ EXTRA_REVISION= .${BRANCH_EXT}.${_TIMENOW}
BRANCH_EXT= p${_BRANCH:C/.*-p([0-9]+$)/\1/}
EXTRA_REVISION= ${BRANCH_EXT}
.endif
PKG_VERSION:= ${_PKG_REVISION}${EXTRA_REVISION:C/[[:space:]]//g}
.endif
.endif # !defined(PKG_VERSION)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was wrongly commented as # !defined(PKG_VERSION).

PKG_VERSION_FROM!=/usr/bin/readlink ${REPODIR}/${PKG_ABI}/latest
PKG_VERSION_FROM_DIR= ${REPODIR}/${PKG_ABI}/${PKG_VERSION_FROM}
BRANCH_EXT_FROM= ${PKG_VERSION_FROM:C/^[^[:alpha:]]+p?([[:alpha:]]*)[0-9]+$/\1/}
.else
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be left undefined instead of empty for better logic below.

@@ -2054,8 +2047,8 @@ PKGMAKEARGS+= PKG_VERSION=${PKG_VERSION} \
packages: .PHONY
${_+_}${MAKE} -C ${.CURDIR} ${PKGMAKEARGS} real-packages

update-packages: .PHONY
${_+_}${MAKE} -C ${.CURDIR} ${PKGMAKEARGS} real-update-packages
update-packages: stage-packages .PHONY
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The update-packages target continues to have the same behaviour as before. The only difference is that the real-update-packages target does not force a stage and creation of world, kernel, AND source package lists.

NOTE: stage-packages is listed in the prerequisites because it does not depend on the value of the PKG_VERSION variable, whereas create-packages does.

@echo "==> Bootstrapping repository, not checking for new packages"
.else
real-update-packages: .PHONY
.if defined(PKG_VERSION_FROM_DIR)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is now inverted for better flow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the inversion depend on your other changes or can it be merged standalone?

Makefile.inc1 Outdated
${PKG_CMD} -o ABI=${PKG_ABI} repo -o OSVERSION="${SRCRELDATE}" \
-m ${WSTAGEDIR}/meta \
-o ${REPODIR}/${PKG_ABI}/${PKG_VERSION} \
${REPODIR}/${PKG_ABI}/${PKG_VERSION} \
${PKG_REPO_SIGNING_KEY} ; \
cd ${REPODIR}/${PKG_ABI}; \
ln -s ${PKG_OUTPUT_DIR} latest
ln -sf ${PKG_OUTPUT_DIR} latest
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This avoids deleting the latest symlink when the PKG_CMD invocation fails.

@bsdimp
Copy link
Member

bsdimp commented Oct 4, 2024

Can you look at this @evadot @bapt @emaste ?

@echo "==> Bootstrapping repository, not checking for new packages"
.else
real-update-packages: .PHONY
.if defined(PKG_VERSION_FROM_DIR)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the inversion depend on your other changes or can it be merged standalone?

.endif
${_+_}@cd ${.CURDIR}; \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using make -C here I think could be its own commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to iteratively land the trivial changes out of the set, as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I had this question on the freebsd-dev IRC channel at some point, but I don't remember getting a definitive answer on it. Why is the ${_+_} prefix scattered around everywhere? As I understand, the manpage states that the + prefix can be used to force a command to always run, even when -n is used at the make command line. So why does it need to be expanded as a variable surrounded by _ characters?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was curious. It's a local oddity to make the -N option not recurse. See 56585ab and it's predecessor acabf29

Makefile.inc1 Outdated
@@ -2349,14 +2340,13 @@ real-sign-packages: _pkgbootstrap .PHONY
.if ${PKG_BIN_VERSION} < 11700
printf "packing_format = \"${PKG_FORMAT}\";\n" >> ${WSTAGEDIR}/meta
.endif
@[ -L "${REPODIR}/${PKG_ABI}/latest" ] && unlink ${REPODIR}/${PKG_ABI}/latest; \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, this hunk to use a forced ln -s to remove the previous link can be its own commit I think.

@svmhdvn
Copy link
Contributor Author

svmhdvn commented Oct 4, 2024

I broke the commit into three separate commits:

  • bugfix for the latest symlink
  • non-functional style changes
  • main change to make real-update-packages independent

Is this easier to review/merge incrementally now?

@svmhdvn
Copy link
Contributor Author

svmhdvn commented Oct 10, 2024

Updated the PR on top of 458dc7f

To perform an incremental update of a pkgbase repo, you would
call 'make update-packages', which will stage, create, and
incrementally choose newer package versions to sign as part
of a pkg repo. However, this forces you to stage the kernel
and source packages along with the world packages. For a
jail-only installation of FreeBSD, these packages are generally
not required.

This patch separates the 'update-packages' target from the
'real-update-packages' target to allow choosing world, kernel,
and/or source individually at the command line like so:

Jail-only installation:
1. make -j`nproc` buildworld
2. make PKG_VERSION='15.snap<date>' \
       stage-packages-world create-packages-world \
       real-update-packages
@svmhdvn
Copy link
Contributor Author

svmhdvn commented Oct 31, 2024

Rebased the PR on top of 8549e3c, which brought in some of the changes from the first commit in my patch series. Any updates on reviewing this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants