package: specfile #15

schuellerf · 2024-12-18T16:30:08Z

This is a maybe controversial version of #14 which tries to avoid commiting vendor/ but patching source tar ball.
Prerequisite to testing osbuild/release-action#25

As we've now vendored our dependencies let's enable dependabot in the same way we use it in our other Go projects. Signed-off-by: Simon de Vlieger <supakeen@redhat.com>

Provide a spec file for `image-builder-cli`. The spec file is based on `osbuild-composer`'s spec file except it is simplified as (some) usecases would currently cloud the intent of the specfile. We can add back RHEL conditionals when/if we start shipping and building for RHEL. Some tools from `osbuild-composer` are also included; most notably the one that generates bundled dependencies from vendored modules. Signed-off-by: Simon de Vlieger <supakeen@redhat.com>

Supports to package `vendor/` directory which is not part of the repository.

mvo5

Thank you! I really like this approach!

thozza · 2024-12-19T04:42:33Z

I'm sorry, but as far as I can tell, this won't work for releasing to Fedora, CS, and RHEL.

Once we start using GitHub action to push release tags, GH will generate the tarball for us, and it will miss the vendored packages. This PR solves only RPM builds in CI using Makefile using a full repo checkout, which won't work with downstream repositories. The problem that needs to be solved is that the release tarball attached to GH releases needs to contain all vendored code.

EDIT: OK, with the modified release action, this should work. Looking forward to see this work in action 🤔

supakeen · 2024-12-19T08:54:40Z

I'm sorry, but as far as I can tell, this won't work for releasing to Fedora, CS, and RHEL.

Once we start using GitHub action to push release tags, GH will generate the tarball for us, and it will miss the vendored packages. This PR solves only RPM builds in CI using Makefile using a full repo checkout, which won't work with downstream repositories. The problem that needs to be solved is that the release tarball attached to GH releases needs to contain all vendored code.

EDIT: OK, with the modified release action, this should work. Looking forward to see this work in action 🤔

I'm still not fully on-board as I've had to deal with people excluding/including things in release tarballs way too often while packaging stuff. I'm not entirely up to date on the Go macros but do they use the actual release tarballs or do they check out the tag from git?

We'd also need to run make in PackIt right?

I prefer that a certain git checkout is always buildable.

thozza · 2024-12-19T08:55:25Z

Makefile

+
+$(RPM_TARBALL):
+	mkdir -p $(CURDIR)/rpmbuild/SOURCES
+	git archive --prefix=image-builder-cli-$(COMMIT)/ --format=tar.gz HEAD > $(RPM_TARBALL)


This should probably consider release tags v* so that our release tarballs have a nice filename that consists only of the name and version but no commit ref.

Good point - I'll align, how this repo will be versioned

@mvo5 @supakeen @thozza should I just manually set the tag v1 for now?

thozza · 2024-12-19T09:00:01Z

Makefile

+
+RPM_TARBALL_UNCOMPRESSED=$(RPM_TARBALL:.tar.gz=.tar)
+
+$(RPM_TARBALL):


The tarball should ontain SPEC file with proper Provides for bundled code, so the commands executed in the $(RPM_SPECFILE) target should be executed also on the SPEC that ends up in the release tarball.

Thanks for the hint!

I'll fix it - but it's wrong here too
https://github.com/osbuild/osbuild-composer/releases/tag/v127

Yes, it is a bug in our osbuild-composer tarball. We do not have proper provides for bundled code in CS/RHEL. :-/

thozza · 2024-12-19T09:00:27Z

Makefile

+#
+
+RPM_SPECFILE=rpmbuild/SPECS/image-builder-cli.spec
+RPM_TARBALL=rpmbuild/SOURCES/image-builder-cli-$(COMMIT).tar.gz


Tarball should IMO contain also the release version, not just the commit.

I'll align with @mvo5 and @supakeen

schuellerf · 2024-12-19T09:08:30Z

@supakeen

We'd also need to run make in PackIt right?

Not necessarily, the action will take care, that the release contains the tarball with vendored stuff already.
( see https://github.com/osbuild/release-action/blob/staging/action.yml#L46 )

I prefer that a certain git checkout is always buildable.

It is - go.mod contains the versions anyway and will fetch when building…
So either fetch the source tarball with vendor stuff or just build with go

thozza · 2024-12-19T09:11:28Z

Makefile

+#         pre-fetched but evaluated at time of use.
+#
+
+VERSION := $(shell (cd "$(SRCDIR)" && grep "^Version:" image-builder-cli.spec | sed 's/[^[:digit:]]*\([[:digit:]]\+\).*/\1/'))


This should take potential "dot" versions into consideration. Such as 12.1, since we use that for backporting. Moreover, this variable does not seem to be used anywhere at all.

schuellerf · 2024-12-19T09:14:03Z

EDIT: OK, with the modified release action, this should work. Looking forward to see this work in action 🤔

me too 😄🎉

supakeen · 2024-12-19T09:21:55Z

@supakeen

We'd also need to run make in PackIt right?

Not necessarily, the action will take care, that the release contains the tarball with vendored stuff already. ( see https://github.com/osbuild/release-action/blob/staging/action.yml#L46 )

PackIt builds from commits (and releases) but the vendor directory would not be there in commits?

I prefer that a certain git checkout is always buildable.

It is - go.mod contains the versions anyway and will fetch when building… So either fetch the source tarball with vendor stuff or just build with go

That works well when you have network access in your build environment. While I have that on my own workstation I don't have it everywhere.

schuellerf · 2024-12-19T09:26:03Z

PackIt builds from commits (and releases) but the vendor directory would not be there in commits?

That's the point of this approach, that the release contains the vendor directory.

That works well when you have network access in your build environment. While I have that on my own workstation I don't have it everywhere.

So you'll just download the source tarball with vendored stuff

supakeen · 2024-12-19T09:27:51Z

PackIt builds from commits (and releases) but the vendor directory would not be there in commits?

That's the point of this approach, that the release contains the vendor directory.

That works well when you have network access in your build environment. While I have that on my own workstation I don't have it everywhere.

So you'll just download the source tarball with vendored stuff

Which doesn't exist for each commit, only tagged releases?

Packit needs expects the archive path to be printed when specifying a custom action to build the archive. See [0]. [0] https://packit.dev/docs/configuration/actions#create-archive Signed-off-by: Tomáš Hozza <thozza@redhat.com>

thozza · 2024-12-19T09:41:05Z

PackIt builds from commits (and releases) but the vendor directory would not be there in commits?

That's the point of this approach, that the release contains the vendor directory.

That works well when you have network access in your build environment. While I have that on my own workstation I don't have it everywhere.

So you'll just download the source tarball with vendored stuff

Which doesn't exist for each commit, only tagged releases?

The answer is YES, Packit will need to have a custom action to build the archive with vendored code for PRs.

For now, Packit should trigger COPR buils for PRs and merges to the `main` branch. Signed-off-by: Tomáš Hozza <thozza@redhat.com>

The release artifact should contain the spec file including the bundle information.

Packit needs golang to be installed when we build SRPM, because we need to vendor code to generate the archive. Moreover, we need to vendor code to be able to modify SPEC port upstream clone to include bundled code. Signed-off-by: Tomáš Hozza <thozza@redhat.com>

mvo5 · 2024-12-19T10:15:14Z

.packit.yaml


 actions:
  get-current-version: bash -c "git describe --tags --abbrev=0 | sed 's|v||'"
-  post-upstream-clone: "./tools/rpm_spec_add_provides_bundle.sh"
+  post-upstream-clone: bash -c "go mod vendor && ./tools/rpm_spec_add_provides_bundle.sh"


supakeen · 2024-12-19T10:17:59Z

Let's undraft this one, I've closed #14 as efforts seem to be more focused on this PR and while I have my own opinions it's not really important enough if everyone else doesn't want /vendor.

I'd really like if we have RPMs shipping today :)

FWIW, I never get || and && correctly in Bash for the first time :D Also handle the case then the repository does not contain any version tags yet by defaulting to "1". Signed-off-by: Tomáš Hozza <thozza@redhat.com>

thozza · 2024-12-19T10:28:31Z

image-builder-cli.spec

+A service for building customized OS artifacts, such as VM images and OSTree
+commits, that uses osbuild under the hood. Besides building images for local
+usage, it can also upload images directly to cloud.
+
+It is compatible with composer-cli and cockpit-composer clients.


This was probably not changed when copied over from osbuild-composer. We should update it.

thozza · 2024-12-19T10:30:19Z

image-builder-cli.spec

+BuildRequires:  systemd
+BuildRequires:  krb5-devel
+BuildRequires:  python3-docutils


I'm not sure if they are still relevant, because we do not interact with systemd or Koji (krb5) and I'm not sure about docs...

These are likely leftovers from the copy :)

thozza · 2024-12-19T10:31:15Z

image-builder-cli.spec

+# Build requirements of 'github.com/containers/storage' package
+BuildRequires:  device-mapper-devel
+%if 0%{?fedora}
+BuildRequires:  systemd-rpm-macros


This is, in my opinion, not needed because we do not enable/disable/start/stop/restart any services as part of the RPM installation process.

These are likely leftovers from the copy :)

thozza · 2024-12-19T10:36:42Z

@schuellerf the sources archive should really be named image-builder-cli-${version_from_spec}.tar.gz. This is what the rpmbuild tooling expects and this is how things work on any of our other repositories.

see https://download.copr.fedorainfracloud.org/results/packit/osbuild-image-builder-cli-15/fedora-41-x86_64/08414991-image-builder-cli/builder-live.log.gz

+ /usr/lib/rpm/rpmuncompress -x /builddir/build/SOURCES/image-builder-cli-65b31cf96d13561bca0b30f455e48874ead1e196.tar.gz
+ STATUS=0
+ '[' 0 -ne 0 ']'
+ cd image-builder-cli-1
/var/tmp/rpm-tmp.vardlL: line 41: cd: image-builder-cli-1: No such file or directory

schuellerf · 2024-12-19T11:10:15Z

@thozza

@schuellerf the sources archive should really be named image-builder-cli-${version_from_spec}.tar.gz. This is what the rpmbuild tooling expects and this is how things work on any of our other repositories.

Sound really reasonable, but why would this work with osbuild-composer then :-/
https://github.com/osbuild/osbuild-composer/blob/main/Makefile#L317

SPEC always uses the next version that will be released. Signed-off-by: Tomáš Hozza <thozza@redhat.com>

RPM tooling expects the source tarball to match the name of the package, suffixed with the version in the SPEC file. Modify the Makefile to comply with this expectation. Signed-off-by: Tomáš Hozza <thozza@redhat.com>

thozza · 2024-12-19T11:27:22Z

So, 33d65c2 fixed build in COPR (and would make things work in downstream distros), but it broke the make rpm use case. In COPR and build systems, we can't define commit when building (S)RPM. So we need to build the tarball conditionally with $(VERSION) or $(COMMIT) based on the use case.

thozza · 2024-12-19T11:29:10Z

@thozza

@schuellerf the sources archive should really be named image-builder-cli-${version_from_spec}.tar.gz. This is what the rpmbuild tooling expects and this is how things work on any of our other repositories.

Sound really reasonable, but why would this work with osbuild-composer then :-/ https://github.com/osbuild/osbuild-composer/blob/main/Makefile#L317

This would work only for CI or local invocations of rpmbuild with the commit variable defined. We can't do that in COPR and downstream build systems.

ondrejbudai · 2024-12-19T14:26:51Z

For the record, cockpit does something similar for node_modules, so it's a viable method. :)

See:

supakeen and others added 3 commits December 18, 2024 16:39

ci: dependabot

8be7470

As we've now vendored our dependencies let's enable dependabot in the same way we use it in our other Go projects. Signed-off-by: Simon de Vlieger <supakeen@redhat.com>

Makefile: support for make release_artifacts

de640f9

Supports to package `vendor/` directory which is not part of the repository.

mvo5 reviewed Dec 18, 2024

View reviewed changes

thozza reviewed Dec 19, 2024

View reviewed changes

Makefile/release_artifacts: print the archive path

5301de0

Packit needs expects the archive path to be printed when specifying a custom action to build the archive. See [0]. [0] https://packit.dev/docs/configuration/actions#create-archive Signed-off-by: Tomáš Hozza <thozza@redhat.com>

Add initial Packit configuration

09d743b

For now, Packit should trigger COPR buils for PRs and merges to the `main` branch. Signed-off-by: Tomáš Hozza <thozza@redhat.com>

thozza force-pushed the specfile branch from 792ce08 to 09d743b Compare December 19, 2024 09:44

schuellerf and others added 2 commits December 19, 2024 10:47

Makefile: assure bundle info in spec file

e1a6eed

The release artifact should contain the spec file including the bundle information.

mvo5 reviewed Dec 19, 2024

View reviewed changes

supakeen mentioned this pull request Dec 19, 2024

package: specfile #14

Closed

FIXUP: .packit.yaml: fix issues when building SRPM

a904a27

FWIW, I never get || and && correctly in Bash for the first time :D Also handle the case then the repository does not contain any version tags yet by defaulting to "1". Signed-off-by: Tomáš Hozza <thozza@redhat.com>

thozza reviewed Dec 19, 2024

View reviewed changes

thozza added 2 commits December 19, 2024 12:11

SPEC: use version 1

8b8333e

SPEC always uses the next version that will be released. Signed-off-by: Tomáš Hozza <thozza@redhat.com>

Makefile: use version instead of commit for archive filename

33d65c2

RPM tooling expects the source tarball to match the name of the package, suffixed with the version in the SPEC file. Modify the Makefile to comply with this expectation. Signed-off-by: Tomáš Hozza <thozza@redhat.com>

schuellerf marked this pull request as ready for review December 19, 2024 11:25

schuellerf added 3 commits December 19, 2024 13:42

.packit.yaml: implement version fallback until we have a tag

ae6a20b

Makefile: support building commit and version based tarball

a17f5eb

.packit.yaml: fix patching the spec file

2492f56

supakeen approved these changes Dec 19, 2024

View reviewed changes

supakeen added this pull request to the merge queue Dec 19, 2024

Merged via the queue into osbuild:main with commit 1f94684 Dec 19, 2024
19 of 23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

package: specfile #15

package: specfile #15

schuellerf commented Dec 18, 2024

mvo5 left a comment

thozza commented Dec 19, 2024 •

edited

Loading

supakeen commented Dec 19, 2024

thozza Dec 19, 2024

schuellerf Dec 19, 2024

schuellerf Dec 19, 2024

thozza Dec 19, 2024

schuellerf Dec 19, 2024

schuellerf Dec 19, 2024

thozza Dec 19, 2024

thozza Dec 19, 2024

schuellerf Dec 19, 2024

schuellerf commented Dec 19, 2024 •

edited

Loading

thozza Dec 19, 2024

schuellerf commented Dec 19, 2024

supakeen commented Dec 19, 2024

schuellerf commented Dec 19, 2024

supakeen commented Dec 19, 2024

thozza commented Dec 19, 2024

mvo5 Dec 19, 2024

supakeen commented Dec 19, 2024

thozza Dec 19, 2024

thozza Dec 19, 2024

supakeen Dec 19, 2024

thozza Dec 19, 2024

supakeen Dec 19, 2024

thozza commented Dec 19, 2024

schuellerf commented Dec 19, 2024

thozza commented Dec 19, 2024

thozza commented Dec 19, 2024

ondrejbudai commented Dec 19, 2024


		RPM_TARBALL_UNCOMPRESSED=$(RPM_TARBALL:.tar.gz=.tar)

		$(RPM_TARBALL):

package: specfile #15

package: specfile #15

Conversation

schuellerf commented Dec 18, 2024

mvo5 left a comment

Choose a reason for hiding this comment

thozza commented Dec 19, 2024 • edited Loading

supakeen commented Dec 19, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schuellerf commented Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

schuellerf commented Dec 19, 2024

supakeen commented Dec 19, 2024

schuellerf commented Dec 19, 2024

supakeen commented Dec 19, 2024

thozza commented Dec 19, 2024

Choose a reason for hiding this comment

supakeen commented Dec 19, 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

thozza commented Dec 19, 2024

schuellerf commented Dec 19, 2024

thozza commented Dec 19, 2024

thozza commented Dec 19, 2024

ondrejbudai commented Dec 19, 2024

thozza commented Dec 19, 2024 •

edited

Loading

schuellerf commented Dec 19, 2024 •

edited

Loading