Refactor dist, azl macros

[dmcilvaney]

Merge Checklist

All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)

• The toolchain has been rebuilt successfully (or no changes were made to it)
• The toolchain/worker package manifests are up-to-date
• Any updated packages successfully build (or no packages were changed)
• Packages depending on static components modified in this PR (Golang, *-static subpackages, etc.) have had their Release tag incremented.
• Package tests (%check section) have been verified with RUN_CHECK=y for existing SPEC files, or added to new SPEC files
• All package sources are available
• cgmanifest files are up-to-date and sorted (./cgmanifest.json, ./toolkit/scripts/toolchain/cgmanifest.json, .github/workflows/cgmanifest.json)
• LICENSE-MAP files are up-to-date (./SPECS/LICENSES-AND-NOTICES/data/licenses.json, ./SPECS/LICENSES-AND-NOTICES/LICENSES-MAP.md, ./SPECS/LICENSES-AND-NOTICES/LICENSE-EXCEPTIONS.PHOTON)
• All source files have up-to-date hashes in the *.signatures.json files
• sudo make go-tidy-all and sudo make go-test-coverage pass
• Documentation has been updated to match any changes to the build system
• Ready to merge

---

Summary

We define %dist as .azl3 (previously .cm2) in /usr/lib/rpm/macros.d/dist.macros. This is currently part of mariner-rpm-macros package. This has a few issues:

• mariner-rpm-macros is not included in many images, it is generally only pulled in with rpm-build etc.
• %dist can't be used easily for conditional flags in spec files (%if 0%{?rhel} is a common pattern).

Define a new macro %azl 3 (which will be defined in macros.dist in the future). Since the DIST_TAG can be customized we set this macro explicitly from values baked into the go tools.

https://microsoft.sharepoint.com/:w:/t/Mariner/EQ5GV3mMEZRLsLNIwxFXj-gBxBalfkJpba-xHiz-iOJc-Q?e=Dw2Min

Change Log

• Add %azl to build macros
• Update specs that are using similar macros to use the new versions.

Does this affect the toolchain?

YES

Test Methodology

• Pipeline build id: https://dev.azure.com/mariner-org/mariner/_build/results?buildId=510616&view=results https://dev.azure.com/mariner-org/mariner/_build/results?buildId=515509&view=results

[ddstreetmicrosoft]

I would really prefer my PR over this one #8028

[ddstreetmicrosoft]

I would really prefer my PR over this one #8028

specifically, i don't want to see the mariner-rpm-macros package get bigger, it should get split up into separate packages. And the macros.dist file absolutely should belong to the azurelinux-release package, not mariner-rpm-macros (or a subpackage).

[dmcilvaney]

I would really prefer my PR over this one #8028

specifically, i don't want to see the mariner-rpm-macros package get bigger, it should get split up into separate packages. And the macros.dist file absolutely should belong to the azurelinux-release package, not mariner-rpm-macros (or a subpackage).

I'm happy to merge these changes together somehow, I'm not married to the exact implementation. Especially given I've now realized that the tools will need to define these macros independently of the actual macro file (at least for the toolchain build) the exact location of the macros isn't important to me anymore.

[dmcilvaney]

@ddstreetmicrosoft Ok, what I'll do is remove my changes to macros.dist. The build tools will be setting the values of these macros anyways, so the exact location/source of the file isn't critical.

The one wrinkle there is that we need to set all the macros we care about inside the tools. How many of the new dist macros are critical? I'll leave azl3 1 and azl 3 defined (unless you are really keen on azure?), but we might need to add more to the go tools in the future.

[PawelWMS]

I would really prefer my PR over this one #8028

specifically, i don't want to see the mariner-rpm-macros package get bigger, it should get split up into separate packages. And the macros.dist file absolutely should belong to the azurelinux-release package, not mariner-rpm-macros (or a subpackage).

I'm happy to merge these changes together somehow, I'm not married to the exact implementation. Especially given I've now realized that the tools will need to define these macros independently of the actual macro file (at least for the toolchain build) the exact location of the macros isn't important to me anymore.

I think I also like the idea of macros.dist coming from azurelinux-release. If we're going to make that switch, however, we have to keep a few things in mind:

• Current "source of truth" for the dist macro is the DIST_TAG variable defined in toolkit/scripts/build_tag.mk. It's used during the toolchain build (when no packages are available, yet) and it determines the value of dist everywhere else. One of the features, that it's meant to support, is to allow non-core Azure Linux teams to build packages with their own dist tag. We should make sure we don't introduce regressions here.
• If we want to make use of the new macros defined in azurelinux-release, we'd need to either add that package to the toolchain or extend the logic inside rpm.go:DefaultDefines() to make sure they are applied during packing of the SRPMs. Ideally, I'd prefer to rely on the macros.dist and not toolkit's magic as we do now, but that's a lot bigger tooling change, which I doubt we'll be able to handle right now. Or at least I wouldn't try to bundle it all up into one change.

With that in mind and Daniel's revert of the mariner-rpm-macros.spec changes in this PR my suggestion is:

• Take this PR first, since it's a smaller change, which will at least help us establish one version of the azl/azure macros across our specs.
• Rebase the azurelinux-release PR on top of this change, since it's more involved considering the two points I mentioned above.

What do you think?

[ddstreetmicrosoft]

@ddstreetmicrosoft Ok, what I'll do is remove my changes to macros.dist. The build tools will be setting the values of these macros anyways, so the exact location/source of the file isn't critical.

I don't particularly care what the toolkit does, as long as it results in rpms that are identical to what a 'normal' (i.e. non-toolkit) build produces.

The one wrinkle there is that we need to set all the macros we care about inside the tools. How many of the new dist macros are critical? I'll leave azl3 1 and azl 3 defined (unless you are really keen on azure?), but we might need to add more to the go tools in the future.

%azure only please. No %azl nor %azl3.

[ddstreetmicrosoft]

I would really prefer my PR over this one #8028

specifically, i don't want to see the mariner-rpm-macros package get bigger, it should get split up into separate packages. And the macros.dist file absolutely should belong to the azurelinux-release package, not mariner-rpm-macros (or a subpackage).

I'm happy to merge these changes together somehow, I'm not married to the exact implementation. Especially given I've now realized that the tools will need to define these macros independently of the actual macro file (at least for the toolchain build) the exact location of the macros isn't important to me anymore.

I think I also like the idea of macros.dist coming from azurelinux-release. If we're going to make that switch, however, we have to keep a few things in mind:

• Current "source of truth" for the dist macro is the DIST_TAG variable defined in toolkit/scripts/build_tag.mk. It's used during the toolchain build (when no packages are available, yet) and it determines the value of dist everywhere else. One of the features, that it's meant to support, is to allow non-core Azure Linux teams to build packages with their own dist tag. We should make sure we don't introduce regressions here.
• If we want to make use of the new macros defined in azurelinux-release, we'd need to either add that package to the toolchain or extend the logic inside rpm.go:DefaultDefines() to make sure they are applied during packing of the SRPMs. Ideally, I'd prefer to rely on the macros.dist and not toolkit's magic as we do now, but that's a lot bigger tooling change, which I doubt we'll be able to handle right now. Or at least I wouldn't try to bundle it all up into one change.

With that in mind and Daniel's revert of the mariner-rpm-macros.spec changes in this PR my suggestion is:

• Take this PR first, since it's a smaller change, which will at least help us establish one version of the azl/azure macros across our specs.

If this is only changing the toolkit then go for it, as I said I don't care about changes to the toolkit. Please remove the systemd.spec changes though.

• Rebase the azurelinux-release PR on top of this change, since it's more involved considering the two points I mentioned above.

What do you think?

[ddstreetmicrosoft]

@ddstreetmicrosoft Ok, what I'll do is remove my changes to macros.dist. The build tools will be setting the values of these macros anyways, so the exact location/source of the file isn't critical.

I don't particularly care what the toolkit does, as long as it results in rpms that are identical to what a 'normal' (i.e. non-toolkit) build produces.

The one wrinkle there is that we need to set all the macros we care about inside the tools. How many of the new dist macros are critical? I'll leave azl3 1 and azl 3 defined (unless you are really keen on azure?), but we might need to add more to the go tools in the future.

%azure only please. No %azl nor %azl3.

To clarify - %azl seems like we're doing the acronym thing like %rhel, but while %rhel shortens 'red hat enterprise linux' and there isn't really any part of that that makes sense standalone (e.g. %redhat or %enterprise wouldn't really make sense as far as referring to a specific product), we have 'azure linux' so %azl isn't even acronymming correctly, and we also can simply use %azure like %fedora does - e.g. 'fedora linux' uses %fedora' and 'azure linux' can use %azure. That's what makes the most sense to me. But if we really don't want to use %azure then just %azl is the next best option IMHO. For sure it makes no sense at all to have %azl3.

Guess we should also see what @christopherco @jslobodzian think

[dmcilvaney]

I feel like %azure is too broad, but that's just my personal feelings. I'll get an opinion from Chris/Jon and go from there.

As for azl3, I based my macros on https://docs.fedoraproject.org/en-US/packaging-guidelines/DistTag/#_conditionals, specifically the fedora %{fc#} and %{el#}. Probably not super critical for our core packages since we split our releases into separate branches today, but it would be handy for down stream teams building packages, or maybe in the future if we split our packages away. This allows a single spec to easily target multiple versions.

[dmcilvaney]

I've... broken GitHub...
Commits on my branch aren't showing up.

[ddstreetmicrosoft]

I feel like %azure is too broad, but that's just my personal feelings. I'll get an opinion from Chris/Jon and go from there.

As for azl3, I based my macros on https://docs.fedoraproject.org/en-US/packaging-guidelines/DistTag/#_conditionals, specifically the fedora %{fc#} and %{el#}. Probably not super critical for our core packages since we split our releases into separate branches today, but it would be handy for down stream teams building packages, or maybe in the future if we split our packages away. This allows a single spec to easily target multiple versions.

I don't have any problem with defining %azl3, I just think using it is a mistake in 99% of cases (instead, the unversioned %azure or %azl macro should be used).

[dmcilvaney]

I don't have any problem with defining %azl3, I just think using it is a mistake in 99% of cases (instead, the unversioned %azure or %azl macro should be used).

100% agree, seems very specialized and personally I'm always going to recommend using the numerical value of azl ( ~%if %{azl} > 2 ).

If we want to omit it I'm certainly not too hung up on it, if you don't see any real value I'll go ahead and remove it. Any maybe by the time I'm done GH will fix itself...

[dmcilvaney]

Had a chat, going with azl, but I'll drop azl#.

[ddstreetmicrosoft]

LGTM but probably should get a review from someone else more familiar with the toolkit also

[PawelWMS]

We're not setting or updating these values here, so no need to return them. I think we might as well completely drop this function and just move the if majorVersion < 0 || nameAbreviation == "" directly to the caller. Alternatively, the function could return a boolean and leave it to the caller to construct a proper error message.

[dmcilvaney]

Both loadLdDistroFlags() and SetDistroMacros() call it, but I agree it doesn't need to return the values (hold over from when it was calling atoi).

[PawelWMS]

Would it be possible to test one scenario per test function? It's going to be clearer what we're checking for and what exactly failed in case of a failure.

[PawelWMS]

Basically, instead of a comment line for each test case in this one function, my suggestion is to have a separate test function with a name that reflects what it's testing.