Address open review comments on PR #15578 (kernel flavor versioning)

toolkit/docs/how_it_works/3_package_building.md

@@ -118,8 +118,23 @@ dot -Tpng -o visualized.png < graph.dot
 ```
 
 ### Dynamic versioning
-We have a versionsprocessor tool that iterates over all Specs and writes their release and versions into a macro file in a format of
-`azl_<package_name>_release`, `azl_<package_name>_version`, note that the `<package_name>` needs any `-` are replaced with `_` due to macros not allowing `-`.
+
+The `versionsprocessor` tool scans all spec files in the `SPECS` directory and writes their version and release information into a macro file (by default `macros.releaseversions`). This file is automatically loaded by `rpmbuild` during package builds, enabling specs to reference other packages' versions without hard-coding them.
+
+For each spec file, the tool generates macros in the following format:
+- `%azl_<package_name>_version` — the package's version (e.g., `1.2.3`)
+- `%azl_<package_name>_release` — the package's release number **without** the distro suffix (e.g., `5`, not `5.azl3`)
+- `%azl_<package_name>_epoch` — the package's epoch number; only emitted when epoch is non-zero
+
+The `<package_name>` component uses the RPM `%{NAME}` field (the binary package name), with any `-` characters replaced by `_` because RPM macros cannot contain hyphens.
+
+For example, for a package named `my-cool-pkg` at version `1.2.3` with release `5.azl3`, the tool emits:
+```
+%azl_my_cool_pkg_version 1.2.3
+%azl_my_cool_pkg_release 5
+```
+
+These macros allow out-of-tree kernel module specs (and others) to reference the exact version of the kernel they target, replacing older workarounds that queried `kernel-headers` via `rpm --whatprovides`.
 
 ### Stage 1: Grapher
 

toolkit/tools/pkg/specreaderutils/specreaderutil.go

@@ -56,6 +56,36 @@ type parseResult struct {
 	err      error
 }
 
+// ChrootOption is a functional option for CreateChroot.
+type ChrootOption func(*chrootConfig)
+
+type chrootConfig struct {
+	specsDir                string
+	srpmsDir                string
+	releaseVersionMacrosFile string
+}
+
+// WithSpecsDir adds a bind mount for the given specs directory inside the chroot.
+func WithSpecsDir(specsDir string) ChrootOption {
+	return func(c *chrootConfig) {
+		c.specsDir = specsDir
+	}
+}
+
+// WithSRPMsDir adds a bind mount for the given SRPMs directory inside the chroot.
+func WithSRPMsDir(srpmsDir string) ChrootOption {
+	return func(c *chrootConfig) {
+		c.srpmsDir = srpmsDir
+	}
+}
+
+// WithReleaseVersionMacrosFile copies the given macros file into the chroot's RPM macros directory.
+func WithReleaseVersionMacrosFile(file string) ChrootOption {
+	return func(c *chrootConfig) {
+		c.releaseVersionMacrosFile = file
+	}
+}
+
 // ParseSPECsWrapper wraps parseSPECs to conditionally run it inside a chroot.
 // If workerTar is non-empty, parsing will occur inside a chroot, otherwise it will run on the host system.
 // releaseVersionMacrosFile, if non-empty, is made available inside the chroot at the same path as on the host.
@@ -67,7 +97,10 @@ func ParseSPECsWrapper(buildDir, specsDir, rpmsDir, srpmsDir, toolchainDir, dist
 
 	if workerTar != "" {
 		const leaveFilesOnDisk = false
-		chroot, err = CreateChroot("specparser_chroot", workerTar, buildDir, specsDir, srpmsDir, releaseVersionMacrosFile)
+		chroot, err = CreateChroot("specparser_chroot", workerTar, buildDir,
+			WithSpecsDir(specsDir),
+			WithSRPMsDir(srpmsDir),
+			WithReleaseVersionMacrosFile(releaseVersionMacrosFile))
 		if err != nil {
 			return
 		}
@@ -126,24 +159,30 @@ func ParseSPECsWrapper(buildDir, specsDir, rpmsDir, srpmsDir, toolchainDir, dist
 	return
 }
 
-// createChroot creates a chroot to parse SPECs inside of.
-func CreateChroot(chrootName, workerTar, buildDir, specsDir, srpmsDir, releaseVersionMacrosFile string) (chroot *safechroot.Chroot, err error) {
+// CreateChroot creates a chroot to parse SPECs inside of.
+func CreateChroot(chrootName, workerTar, buildDir string, opts ...ChrootOption) (chroot *safechroot.Chroot, err error) {
 	const (
 		existingDir      = false
 		leaveFilesOnDisk = false
 	)
 
+	cfg := &chrootConfig{}
+	for _, opt := range opts {
+		opt(cfg)
+	}
+
 	// Mount the specs and srpms directories to an identical path inside the chroot.
 	// Since specreader saves the full paths to specs in its output that grapher will then consume,
 	// the pathing needs to be preserved from the host system.
 	var extraDirectories []string
 
-	extraMountPoints := []*safechroot.MountPoint{
-		safechroot.NewMountPoint(specsDir, specsDir, "", safechroot.BindMountPointFlags, ""),
+	var extraMountPoints []*safechroot.MountPoint
+	if cfg.specsDir != "" {
+		extraMountPoints = append(extraMountPoints, safechroot.NewMountPoint(cfg.specsDir, cfg.specsDir, "", safechroot.BindMountPointFlags, ""))
 	}
 
-	if srpmsDir != "" {
-		extraMountPoints = append(extraMountPoints, safechroot.NewMountPoint(srpmsDir, srpmsDir, "", safechroot.BindMountPointFlags, ""))
+	if cfg.srpmsDir != "" {
+		extraMountPoints = append(extraMountPoints, safechroot.NewMountPoint(cfg.srpmsDir, cfg.srpmsDir, "", safechroot.BindMountPointFlags, ""))
 	}
 
 	chrootDir := filepath.Join(buildDir, chrootName)
@@ -156,7 +195,7 @@ func CreateChroot(chrootName, workerTar, buildDir, specsDir, srpmsDir, releaseVe
 
 	// If this is not a regular build then copy in all of the SPECs since there are no bind mounts.
 	if !buildpipeline.IsRegularBuild() {
-		dirsToCopy := []string{specsDir, srpmsDir}
+		dirsToCopy := []string{cfg.specsDir, cfg.srpmsDir}
 		for _, dir := range dirsToCopy {
 			dirInChroot := filepath.Join(chroot.RootDir(), dir)
 			err = directory.CopyContents(dir, dirInChroot)
@@ -172,8 +211,8 @@ func CreateChroot(chrootName, workerTar, buildDir, specsDir, srpmsDir, releaseVe
 
 	// If a release version macros file is provided, copy it into the default RPM macros directory
 	// inside the chroot so rpmspec/rpmbuild pick it up automatically.
-	if releaseVersionMacrosFile != "" {
-		err = chroot.AddRPMMacrosFile(releaseVersionMacrosFile)
+	if cfg.releaseVersionMacrosFile != "" {
+		err = chroot.AddRPMMacrosFile(cfg.releaseVersionMacrosFile)
 		if err != nil {
 			logger.Log.Errorf("Failed to add release version macros file to chroot: %s", err)
 		}

toolkit/tools/versionsprocessor/versionsprocessor.go

@@ -14,6 +14,7 @@ import (
 	"path/filepath"
 	"regexp"
 	"runtime"
+	"strconv"
 	"strings"
 
 	"github.com/microsoft/azurelinux/toolkit/tools/internal/exe"
@@ -28,6 +29,10 @@ import (
 	"gopkg.in/alecthomas/kingpin.v2"
 )
 
+// packageVersionRe matches lines of the form "NAME: EPOCHNUM|VERSION-RELEASE"
+// as produced by the rpm query format "%{NAME}: %{EPOCHNUM}|%{VERSION}-%{RELEASE}".
+var packageVersionRe = regexp.MustCompile(`^([^:]+): (\d+)\|(.+)-(.+)$`)
+
 var (
 	app           = kingpin.New("versionsprocessor", "A tool to generate a macro file of all specs version and release")
 	specsDir      = exe.InputDirFlag(app, "Directory to scan for SPECS")
@@ -83,31 +88,29 @@ func main() {
 	}
 
 	const leaveFilesOnDisk = false
-	chroot, err = specreaderutils.CreateChroot("versionprocessor_chroot", *workerTar, *buildDir, *specsDir, "", "")
+	chroot, err = specreaderutils.CreateChroot("versionprocessor_chroot", *workerTar, *buildDir,
+		specreaderutils.WithSpecsDir(*specsDir))
 	if err != nil {
-		logger.PanicOnError("Failed to create chroot")
+		logger.Log.Errorf("Failed to create chroot: %s", err)
+		os.Exit(1)
 	}
 	defer chroot.Close(leaveFilesOnDisk)
 
 	doParse := func() error {
 		// Find all spec files
 		allSpecFiles, err := specreaderutils.FindSpecFiles(*specsDir, nil)
 		if err != nil {
-			logger.Log.Panicf("Error finding spec files: %s", err)
+			logger.Log.Errorf("Error finding spec files: %s", err)
 			return err
 		}
 
 		logger.Log.Infof("Processing version and release for %d spec files into %s", len(allSpecFiles), *output)
 
-		// Process all specs files
+		// Get spec file version-release
 		for _, specFile := range allSpecFiles {
-
-			// Get spec file version-release
 			macrosOutput, err = processSpecFile(specFile, buildArch, prefix, macrosOutput)
-
 			if err != nil {
-				logger.Log.Errorf("Error processing spec file (%s): %s", specFile, err)
-				continue
+				return fmt.Errorf("error processing spec file (%s): %w", specFile, err)
 			}
 		}
 
@@ -120,6 +123,11 @@ func main() {
 		err = doParse()
 	}
 
+	if err != nil {
+		logger.Log.Errorf("Failed to generate versions macros: %s", err)
+		os.Exit(1)
+	}
+
 	err = writeExtraFilesToOutput(*extraFiles, macrosOutput, *output)
 	if err != nil {
 		logger.Log.Errorf("Failed to write extra files to output: %s", err)
@@ -128,14 +136,12 @@ func main() {
 }
 
 func processSpecFile(specFile string, buildArch string, prefix string, macrosOutput []string) (newMacrosOutput []string, err error) {
-	// Get spec file version-release
-
 	specFileName := filepath.Base(specFile)
 
 	sourceDir := filepath.Dir(specFile)
 	defines := rpm.DefaultDistroDefines(false, *distTag)
 
-	packages, err := rpm.QuerySPEC(specFile, sourceDir, `%{NAME}: %{evr}\n`, buildArch, defines, rpm.QueryHeaderArgument)
+	packages, err := rpm.QuerySPEC(specFile, sourceDir, `%{NAME}: %{EPOCHNUM}|%{VERSION}-%{RELEASE}\n`, buildArch, defines, rpm.QueryHeaderArgument)
 
 	if err != nil {
 		logger.Log.Errorf("Failed to query spec file (%s). Error: %s", specFileName, err)
@@ -144,46 +150,51 @@ func processSpecFile(specFile string, buildArch string, prefix string, macrosOut
 
 	for _, packageVersionString := range packages {
 
-		macros, err := processPackageVersionString(packageVersionString, specFileName, prefix)
+		packageMacros, err := processPackageVersionString(packageVersionString, specFileName, prefix)
 		if err != nil {
 			logger.Log.Errorf("Error processing package version string: %s", err)
 			continue
 		}
 
-		macrosOutput = append(macrosOutput, macros)
+		macrosOutput = append(macrosOutput, packageMacros...)
 	}
 
 	return macrosOutput, nil
 }
 
-func processPackageVersionString(packageVersionString string, specFileName string, prefix string) (macros string, err error) {
-	// the output of the above query is in the format of "packagename: version-release",
-	// so split by ": " to get the version-release portion we want the second part
-	releaseVerSplit := regexp.MustCompile(`^[^:]+: (.+)-(.+)$`).FindStringSubmatch(packageVersionString)[1:]
-
-	if len(releaseVerSplit) != 2 {
-		err = fmt.Errorf("Empty version-release format retrieved from spec file (%s)", specFileName)
-		logger.Log.Errorf("Empty version-release format retrieved from spec file (%s)", specFileName)
+func processPackageVersionString(packageVersionString string, specFileName string, prefix string) (macros []string, err error) {
+	// The output of the rpm query is in the format "NAME: EPOCHNUM|VERSION-RELEASE".
+	match := packageVersionRe.FindStringSubmatch(packageVersionString)
 
-		return "", err
+	if len(match) != 5 {
+		err = fmt.Errorf("unexpected version format retrieved from spec file (%s): %q", specFileName, packageVersionString)
+		logger.Log.Errorf("%s", err)
+		return nil, err
 	}
 
-	version := releaseVerSplit[0]
-	release := releaseVerSplit[1]
-	releaseClean := strings.Replace(release, *distTag, "", 1) // targetting azl3 specifically since this won't go into above 3.0 toolkit
+	packageName := match[1]
+	epochStr := match[2]
+	version := match[3]
+	release := match[4]
 
-	// strip out the .spec suffix and replace '-' with '_' as RPM macros cannot have '-'
-	specFileNameMacroFormat := strings.Replace(specFileName, ".spec", "", 1)
-	specFileNameMacroFormat = strings.ReplaceAll(specFileNameMacroFormat, "-", "_")
+	// Remove the dist tag from the release. The release macro does not include the distro suffix
+	// so that it can be compared across different distros.
+	releaseClean := strings.Replace(release, *distTag, "", 1)
 
-	versionMacroString := prefix + "_" + specFileNameMacroFormat + "_version"
-	releaseMacroString := prefix + "_" + specFileNameMacroFormat + "_release"
+	// Replace '-' with '_' as RPM macros cannot contain '-'.
+	packageNameMacroFormat := strings.ReplaceAll(packageName, "-", "_")
 
-	// Generate RPM macro definitions instead of modifying spec files directly.
-	macros = fmt.Sprintf("%%%s %s\n%%%s %s",
-		versionMacroString, version,
-		releaseMacroString, releaseClean,
-	)
+	versionMacroString := prefix + "_" + packageNameMacroFormat + "_version"
+	releaseMacroString := prefix + "_" + packageNameMacroFormat + "_release"
+
+	epochNum, convErr := strconv.Atoi(epochStr)
+	if convErr == nil && epochNum > 0 {
+		epochMacroString := prefix + "_" + packageNameMacroFormat + "_epoch"
+		macros = append(macros, fmt.Sprintf("%%%s %s", epochMacroString, epochStr))
+	}
+
+	macros = append(macros, fmt.Sprintf("%%%s %s", versionMacroString, version))
+	macros = append(macros, fmt.Sprintf("%%%s %s", releaseMacroString, releaseClean))
 
 	return macros, nil
 }