application-inventory: use RPM's Version and Release, set Epoch #263

ginglis13 · 2024-06-03T00:29:16Z

Issue number:

Closes #140

Description of changes:

Before the onset of kits, Bottlerocket would build RPMs with Release values set to the commit of Bottlerocket being released (or, more accurately, the commit of Bottlerocket being built). This worked fine under the monorepo model, but with the advent of kits, this is not enough for indicating incremental changes to a project. There must be some monotonically increasing value present in the application-inventory for a Bottlerocket variant for systems to key off of for versioning and Release comparisons.

This change computes the unix timestamp in ms of the project commit from which a package is built by buildsys and sets the Release of the package as <timestamp>+<commit short sha>. It also sets the Version of a package to the actual package's version, and not the Bottlerocket version.

Testing done:

Built a Bottlerocket variant with these changes and inspected packages:

cargo make \
    -e=TWOLITER_ALLOW_SOURCE_INSTALL=false \
    -e=TWOLITER_ALLOW_BINARY_INSTALL=false \
    -e=TWOLITER_SKIP_VERSION_CHECK=true \
    build-variant

rpm -qip build/rpms/shim/bottlerocket-shim-15.8-11717197220_a87879e6.aarch64.rpm
Name        : bottlerocket-shim
Version     : 15.8
Release     : 11717197220_a87879e6
...

Launched a bottlerocket instance:

bash-5.1# cat aarch64-bottlerocket-linux-gnu/sys-root/usr/share/bottlerocket/application-inventory.json  | grep Epoch 
....

     "Epoch": "1",
     "Epoch": "1",
     "Epoch": "1",

bash-5.1# head aarch64-bottlerocket-linux-gnu/sys-root/usr/share/bottlerocket/application-inventory.json
{
  "Content": [
    {
      "Name": "acpid",
      "Publisher": "Bottlerocket",
      "Version": "2.0.34",
      "Release": "11717197220_a87879e6",
      "Epoch": "1",
      "InstalledTime": "2024-06-04T17:36:24Z",
      "ApplicationType": "Unspecified",

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

bcressey

Looks good!

twoliter/embedded/Makefile.toml

twoliter/embedded/build.Dockerfile

twoliter/embedded/rpm2img

twoliter/embedded/build.Dockerfile

tools/buildsys/src/args.rs

ginglis13 · 2024-06-04T18:49:12Z

^ split rpm2img changes into a separate commit; add .br1 tag to Release, add more comments to changes.

ginglis13 · 2024-06-04T19:54:21Z

Rebased locally and seeing the failure now. There are some integ tests that expect package naming of a certain form that changes in this PR

webern · 2024-06-04T22:48:40Z

Rebased locally and seeing the failure now. There are some integ tests that expect package naming of a certain form that changes in this PR

Hmm yeah the test ensures that the packages were built for the kit by checking for the RPM output file. Can you update the test to accommodate the new naming convention?

ginglis13 · 2024-06-04T23:24:50Z

^ update twoliter/src/cmd/build.rs tests to include defaults for Release that populate an rpm name.

ginglis13 · 2024-06-04T23:55:16Z

^ use . instead of _ as separator in Release

ginglis13 · 2024-06-08T00:01:18Z

^ set Epoch in package spec and each subpackage, source Epoch for application-inventory via rpm query now instead of via environment variable

The kernel package definitions are a special case in terms of Requires for subpackages. Each has

# Pull in expected modules and development files.
Requires: %{name}-modules = %{version}-%{release}
Requires: %{name}-devel = %{version}-%{release}

Defined in their spec. This change includes some seding to insert the subpackage Epoch value in these statements.

E.g. showing package and its subpackages set Epoch:

╰─➤  rpm -qip ~/bottlerocket/build/rpms/kernel-5.15/bottlerocket-kernel-5.15-5.15.158-11717197220.a87879e6.br1.aarch64.rpm
Name        : bottlerocket-kernel-5.15
Epoch       : 1
...

╰─➤  rpm -qip ~/bottlerocket/build/rpms/kernel-5.15/bottlerocket-kernel-5.15-devel-5.15.158-11717197220.a87879e6.br1.aarch64.rpm
Name        : bottlerocket-kernel-5.15-devel
Epoch       : 1
...

tools/buildsys/src/args.rs

twoliter/embedded/Makefile.toml

twoliter/embedded/build.Dockerfile

cbgbt · 2024-06-10T17:54:00Z

twoliter/src/cmd/build.rs

-            let rpm = kit_output_dir.join(&format!("bottlerocket-{package}-0.0-0.{arch}.rpm"));
+            let rpm = kit_output_dir.join(&format!(
+                "bottlerocket-{package}-0.0-00000000000.00000000.br1.{arch}.rpm"
+            ));


Will this test still succeed if the commit is -dirty?

I'm confused as to how these default values are arriving in the build-package calls. I think this test is actually demonstrating the problem of having these args absent from cargo make build-kit. build-kit will result in package builds, and the package builds will then have these values absent.

I think buildsys needs to choke on the absence of these values and the Makefile needs to ensure they exist.

Will this test still succeed if the commit is -dirty?

If the commit were dirty, the rpm would be named "bottlerocket-{package}-0.0-00000000000.00000000_dirty.br1.{arch}.rpm" per /~https://github.com/bottlerocket-os/twoliter/pull/263/files#diff-a08098dd4b8cb6afda85dec0555534eaeb5a75f4e43f2db7ddcbc2e8c63d0e17R170-R171. So no, this test would not succeed if the package is being built from a dirty tree.

I'm confused as to how these default values are arriving in the build-package calls. I think this test is actually demonstrating the problem of having these args absent from cargo make build-kit. build-kit will result in package builds, and the package builds will then have these values absent.

These values are set via environment variables in twoliter's Makefile.toml: /~https://github.com/bottlerocket-os/twoliter/pull/263/files#diff-ad5b91511e741abc0f447ce47f0d0c399ec15a4acd7666aa0f30094356d53af5R27-R31. They are read in BuildPackageArgs here: /~https://github.com/bottlerocket-os/twoliter/pull/263/files#diff-d69207eb08549801c5be3079f2fef42b9c132b83ecdb9d73160b91db074395cdR119-R135, which ultimately is used in a build-package call. I see between this and other similar comments, we must set these values in kit build as well, otherwise we'll always have default values present for packages built from kits...

I think buildsys needs to choke on the absence of these values and the Makefile needs to ensure they exist.

can do

I did some validation with kit building given these changes. Because kit building also builds packages, the required environment variables will be set for package building, and resultant kits will have packages with these values properly set:

$ ls tests/projects/local-kit/build/kits/extra-3-kit/aarch64/Packages bottlerocket-pkg-e-0.0-01717804500.7185d90.br1.aarch64.rpm bottlerocket-pkg-f-0.0-01717804500.7185d90.br1.aarch64.rpm bottlerocket-pkg-g-0.0-01717804500.7185d90.br1.aarch64.rpm $ ls tests/projects/local-kit/build/rpms/pkg-b bottlerocket-pkg-b-0.0-01717804500.7185d90.br1.aarch64.rpm

As for why the integration test is using default values, that's because the integ test builds kits in a temp dir that and is not a git project.

webern · 2024-06-10T18:08:13Z

twoliter/embedded/Makefile.toml

+export BUILDSYS_VERSION_BUILD="${BUILDSYS_VERSION_BUILD}"
+export BUILDSYS_VERSION_BUILD_TIMESTAMP="${BUILDSYS_VERSION_BUILD_TIMESTAMP}"
+export BUILDSYS_VERSION_BUILD_EPOCH="${BUILDSYS_VERSION_BUILD_EPOCH}"


Aren't these also needed in build-kit?

See above comment, we won't need these in build-kit as they're already included in build-package

webern · 2024-06-10T18:10:01Z

twoliter/src/cmd/build.rs

-            let rpm = kit_output_dir.join(&format!("bottlerocket-{package}-0.0-0.{arch}.rpm"));
+            let rpm = kit_output_dir.join(&format!(
+                "bottlerocket-{package}-0.0-00000000000.00000000.br1.{arch}.rpm"
+            ));


I'm confused as to how these default values are arriving in the build-package calls. I think this test is actually demonstrating the problem of having these args absent from cargo make build-kit. build-kit will result in package builds, and the package builds will then have these values absent.

I think buildsys needs to choke on the absence of these values and the Makefile needs to ensure they exist.

ginglis13 · 2024-06-10T21:00:38Z

^ take feedback to add some clarity in comments, group all Epoch changes to an RPM in the rpmbuild layer

twoliter/embedded/build.Dockerfile

twoliter/embedded/rpm2img

tools/buildsys/src/args.rs

twoliter/embedded/build.Dockerfile

bcressey · 2024-06-10T23:02:57Z

twoliter/embedded/build.Dockerfile

   && cat generated.bconds ${PACKAGE}.spec >> rpmbuild/SPECS/${PACKAGE}.spec \
+   && sed -i "/%package.*$/a Epoch: ${BUILD_EPOCH}" rpmbuild/SPECS/${PACKAGE}.spec \


consider deleting existing Epoch lines, and being more rigorous about matches:

Suggested change

&& sed -i "/%package.*$/a Epoch: ${BUILD_EPOCH}" rpmbuild/SPECS/${PACKAGE}.spec \

&& sed -i '/^Epoch:/d' rpmbuild/SPECS/${PACKAGE}.spec \

&& sed -i "/^%package\s.*$/a Epoch: ${BUILD_EPOCH}" rpmbuild/SPECS/${PACKAGE}.spec \

I think this fits into the case that we could alter the expectations of a packager by modifying their Epoch value. Would it be better to fail the build than to produce something unexpected?

In the latest revision of this PR, I opted not to delete existing Epoch lines for two reasons:

attempting to delete previous Epoch was resulting in invalid RPM, with an error along the lines of File must begin with "/": Epoch: . I didn't dive further becaue I concluded that instead of making this change, we can:

By always attempting to set our Epoch, we will induce RPM failures for the case where a packager attempts to specify their own Epoch. This seems more reasonable than silently changing the value they intend to set as Epoch

twoliter/embedded/build.Dockerfile

Before the onset of kits, Bottlerocket would build RPMs with Release values set to the commit of Bottlerocket being released (or, more accurately, the commit of Bottlerocket being built). This worked fine under the monorepo model, but with the advent of kits, this is not enough for indicating incremental changes to a project. There must be some monotonically increasing value present in the application-invetory for a Bottlerocket variant for systems to key off of for versioning and Release comparisons. This change computes the unix timestamp in ms of the project commit from which a package is built by buildsys and sets the Release of the package as <timestamp>+<commit short sha>. It also sets the Epoch value of RPMs and their subpackages based on an environment variable BUILDSYS_VERSION_BUILD_EPOCH. Signed-off-by: Gavin Inglis <giinglis@amazon.com>

when building application-inventory for packages, use the actual package Version specifying in the package spec, as well as its actualy Release value. Also, specify an Epoch for cases where package versioning breaks between releases. Signed-off-by: Gavin Inglis <giinglis@amazon.com>

ginglis13 · 2024-06-11T01:02:59Z

^ first force push, address feedback:

remove unnecessary inclusion of the epoch environment variable in buildsys subcommands where it is not needed.
clean up some sed commands to be more thorough, especially, in checking comparators for Requires|Obsoletes|Conflicts
strip -dirty completely from BUILD_ID when building an RPM's Release field

^ second force push, rebase

^ 3rd, fix a missed nit in above two pushes

revert most of bottlerocket-os#263, which set the Epoch for packages built with Twoliter, set package inventory Version as the actual package version, and built a customer release field to give to the package. This PR retains the change to setting Release in the actual package RPM but reverts the change to include that in the application inventory for the package. Signed-off-by: Gavin Inglis <giinglis@amazon.com>

ginglis13 requested review from bcressey, webern and yeazelm June 3, 2024 00:29

bcressey reviewed Jun 4, 2024

View reviewed changes

twoliter/embedded/Makefile.toml Outdated Show resolved Hide resolved

twoliter/embedded/build.Dockerfile Outdated Show resolved Hide resolved

twoliter/embedded/rpm2img Show resolved Hide resolved

webern approved these changes Jun 4, 2024

View reviewed changes

twoliter/embedded/rpm2img Show resolved Hide resolved

twoliter/embedded/build.Dockerfile Outdated Show resolved Hide resolved

tools/buildsys/src/args.rs Outdated Show resolved Hide resolved

ginglis13 force-pushed the application-json-updates branch 2 times, most recently from 46b512d to b8749fa Compare June 4, 2024 18:47

ginglis13 changed the title ~~buildsys: include commit timestamp in package Release~~ application-inventory: use RPM's Version and Release, set Epoch Jun 4, 2024

ginglis13 requested review from bcressey and webern June 4, 2024 18:52

ginglis13 force-pushed the application-json-updates branch from b8749fa to 68152bf Compare June 4, 2024 23:23

ginglis13 force-pushed the application-json-updates branch 2 times, most recently from f3c7472 to 5505e53 Compare June 4, 2024 23:54

webern approved these changes Jun 5, 2024

View reviewed changes

yeazelm approved these changes Jun 5, 2024

View reviewed changes

ginglis13 force-pushed the application-json-updates branch from 5505e53 to 7185d90 Compare June 7, 2024 23:56

ginglis13 requested review from yeazelm, webern and cbgbt June 8, 2024 00:01

bcressey mentioned this pull request Jun 10, 2024

core kit migration 🚧 tracking issue bottlerocket-os/bottlerocket#4048

Closed

cbgbt reviewed Jun 10, 2024

View reviewed changes

webern reviewed Jun 10, 2024

View reviewed changes

ginglis13 force-pushed the application-json-updates branch from 7185d90 to 5b0fd67 Compare June 10, 2024 20:59

ginglis13 requested review from cbgbt and webern June 10, 2024 21:02

webern approved these changes Jun 10, 2024

View reviewed changes

bcressey reviewed Jun 10, 2024

View reviewed changes

ginglis13 force-pushed the application-json-updates branch 2 times, most recently from 9215834 to 7e070cf Compare June 11, 2024 00:58

ginglis13 added 2 commits June 11, 2024 01:01

ginglis13 force-pushed the application-json-updates branch from 7e070cf to 51cd42e Compare June 11, 2024 01:01

ginglis13 requested review from bcressey and webern June 11, 2024 01:03

cbgbt approved these changes Jun 11, 2024

View reviewed changes

webern approved these changes Jun 11, 2024

View reviewed changes

yeazelm approved these changes Jun 11, 2024

View reviewed changes

ginglis13 merged commit f9071cc into bottlerocket-os:develop Jun 11, 2024
1 check passed

ginglis13 mentioned this pull request Jun 18, 2024

revert application-inventory: use RPM's Version and Release, set Epoch #301

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

application-inventory: use RPM's Version and Release, set Epoch #263

application-inventory: use RPM's Version and Release, set Epoch #263

ginglis13 commented Jun 3, 2024 •

edited

Loading

bcressey left a comment

ginglis13 commented Jun 4, 2024

ginglis13 commented Jun 4, 2024

webern commented Jun 4, 2024

ginglis13 commented Jun 4, 2024 •

edited

Loading

ginglis13 commented Jun 4, 2024

ginglis13 commented Jun 8, 2024

cbgbt Jun 10, 2024

webern Jun 10, 2024

ginglis13 Jun 10, 2024

ginglis13 Jun 10, 2024

webern Jun 10, 2024

ginglis13 Jun 10, 2024

webern Jun 10, 2024

ginglis13 commented Jun 10, 2024 •

edited

Loading

bcressey Jun 10, 2024

cbgbt Jun 11, 2024

ginglis13 Jun 11, 2024

ginglis13 commented Jun 11, 2024 •

edited

Loading

		&& cat generated.bconds ${PACKAGE}.spec >> rpmbuild/SPECS/${PACKAGE}.spec \
		&& sed -i "/%package.*$/a Epoch: ${BUILD_EPOCH}" rpmbuild/SPECS/${PACKAGE}.spec \

	&& sed -i "/%package.*$/a Epoch: ${BUILD_EPOCH}" rpmbuild/SPECS/${PACKAGE}.spec \
	&& sed -i '/^Epoch:/d' rpmbuild/SPECS/${PACKAGE}.spec \
	&& sed -i "/^%package\s.*$/a Epoch: ${BUILD_EPOCH}" rpmbuild/SPECS/${PACKAGE}.spec \

application-inventory: use RPM's Version and Release, set Epoch #263

application-inventory: use RPM's Version and Release, set Epoch #263

Conversation

ginglis13 commented Jun 3, 2024 • edited Loading

bcressey left a comment

Choose a reason for hiding this comment

ginglis13 commented Jun 4, 2024

ginglis13 commented Jun 4, 2024

webern commented Jun 4, 2024

ginglis13 commented Jun 4, 2024 • edited Loading

ginglis13 commented Jun 4, 2024

ginglis13 commented Jun 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ginglis13 commented Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ginglis13 commented Jun 11, 2024 • edited Loading

ginglis13 commented Jun 3, 2024 •

edited

Loading

ginglis13 commented Jun 4, 2024 •

edited

Loading

ginglis13 commented Jun 10, 2024 •

edited

Loading

ginglis13 commented Jun 11, 2024 •

edited

Loading