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

[CI:BUILD] Packit: add jobs for downstream Fedora package builds #18465

Merged
merged 1 commit into from
May 24, 2023

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented May 4, 2023

Get rid of podman.spec.rpkg in favour of
rpm/podman.spec which gets synced with fedora dist-git on every upstream release. The version in the new spec file is set to 0 by default and gets updated by packit automatically on every packit task.

For local manual rpm builds using the spec, the helper script in the rpm/ subdir will update the Version field with the latest version found in the upstream repo.

Packit will automatically create a PR on fedora dist-git on every new upstream release. A sample PR will look like:
https://src.fedoraproject.org/rpms/container-selinux/pull-request/10#

A dry run for this can be triggered using:
$ packit propose-downstream --local-content

To run this command locally, you would need to have your packit user-configuration-file set.
Ref: https://packit.dev/docs/configuration/#user-configuration-file

along with a fedora api key created at:
https://src.fedoraproject.org/settings#nav-api-tab with sufficient ACLs.

Also includes a revised package Makefile target which will build rpms
using rpm/podman.spec. Fixes: #18421.

[NO NEW TESTS NEEDED]

Does this PR introduce a user-facing change?

None

@packit-as-a-service
Copy link

Failed to load packit config file:

Please correct data and retry.

For more info, please check out the documentation or contact the Packit team.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lsm5

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2023
@packit-as-a-service
Copy link

Failed to load packit config file:

Please correct data and retry.

For more info, please check out the documentation or contact the Packit team.

@lsm5 lsm5 force-pushed the packit-downstream-sync branch 3 times, most recently from af2877e to 38c4793 Compare May 4, 2023 18:29
@lsm5 lsm5 linked an issue May 4, 2023 that may be closed by this pull request
@lsm5 lsm5 marked this pull request as draft May 4, 2023 18:30
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2023
@lsm5 lsm5 force-pushed the packit-downstream-sync branch 9 times, most recently from c0a8c46 to afacec4 Compare May 4, 2023 19:53
@lsm5
Copy link
Member Author

lsm5 commented May 4, 2023

@mheon any idea if sqlite3 doesn't play well with centos 8 stream?

# github.com/containers/podman/vendor/github.com/mattn/go-sqlite3
_build/src/github.com/containers/podman/vendor/github.com/mattn/go-sqlite3/sqlite3.go:74:1: warning: '_sqlite3_exec' defined but not used [-Wunused-function]
 _sqlite3_exec(sqlite3* db, const char* pcmd, long long* rowid, long long* changes)
 ^~~~~~~~~~~~~
error: Bad exit status from /var/tmp/rpm-tmp.k4tlck (%build)

works on centos 9 stream and fedora.

@lsm5
Copy link
Member Author

lsm5 commented May 4, 2023

@mheon ignore me, the actual error was btrfs/ioctl.h: no such file or directory way up in the build log.

@lsm5 lsm5 force-pushed the packit-downstream-sync branch 2 times, most recently from a2f6fd2 to af49ef5 Compare May 8, 2023 14:45
@lsm5 lsm5 force-pushed the packit-downstream-sync branch 2 times, most recently from 20a8ac8 to eacef91 Compare May 22, 2023 19:28
@lsm5 lsm5 changed the title [DO NOT MERGE] [CI:BUILD] Packit: add jobs for downstream Fedora package builds [CI:BUILD] Packit: add jobs for downstream Fedora package builds May 22, 2023
@lsm5 lsm5 marked this pull request as ready for review May 22, 2023 19:33
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 22, 2023
@lsm5 lsm5 force-pushed the packit-downstream-sync branch from eacef91 to 29c9093 Compare May 22, 2023 19:44
@edsantiago
Copy link
Member

@lsm5 help please, I'm feeling very dumb. I'm trying to figure out where the new specfile comes from, so I can look at the minimalest set of diffs. I've compared against fedpkg @ main and f38 and f37, and those diffs are huge. Where can I find the smallest set of diffs?

Also, when you say "gets synced with fedora dist-git", do you mean that Packit pushes to fedpkg, or that Packit pulls from fedpkg, or something else? I'm guessing Packit pushes something to fedpkg, but am not sure what that looks like (rpm/podman.spec with updated NVR???)

I realize these are stupid questions, and apologize.

@lsm5
Copy link
Member Author

lsm5 commented May 23, 2023

@lsm5 help please, I'm feeling very dumb. I'm trying to figure out where the new specfile comes from, so I can look at the minimalest set of diffs. I've compared against fedpkg @ main and f38 and f37, and those diffs are huge. Where can I find the smallest set of diffs?

New specfile rpm/podman.spec was taken from fedora rawhide @ https://src.fedoraproject.org/rpms/podman/raw/rawhide/f/podman.spec

But it's not exactly the same. I added a lot of conditionals at the top to make all distro envs happy (centos 8/9). If you ignore the conditionals at the top, there shouldn't be too much difference with the rest. Let me know if any particular diff lines are of concern to you.

Also, when you say "gets synced with fedora dist-git", do you mean that Packit pushes to fedpkg, or that Packit pulls from fedpkg, or something else? I'm guessing Packit pushes something to fedpkg, but am not sure what that looks like (rpm/podman.spec with updated NVR???)

Packit pushes to fedpkg. So, downstream is dependent on upstream and all the changes first go upstream.
When a new release is posted on upstream podman, the propose-downstream task mentioned in .packit.yaml gets
triggered and submits a PR to the fedora package like: https://src.fedoraproject.org/rpms/podman/pull-request/64 .

The fedora PR waits on a human for merge, so the ultimate fedora packaging responsibility still depends on humans. Once the PR is merged, packit will handle koji builds and bodhi updates.

See builds and updates by the packit user about a day ago:
https://koji.fedoraproject.org/koji/packageinfo?packageID=23542
https://bodhi.fedoraproject.org/updates/?packages=container-selinux

The additional tasks in .packit.yaml in this PR take zero extra upstream time and don't get in upstream's way. They occur only on every upstream release, and not on every PR or every commit.

Main benefit is this now means a single rpm spec file which will be used for:

  1. CI copr build tests on every PR
  2. copr builds on rhcontainerbot/podman-next copr after every commit to main branch
  3. official fedora package builds AFTER every upstream release.

I realize these are stupid questions, and apologize.

Not at all, good to have them answered so everyone's willingly on board :D

If you need any additional comments in the commit message or spec file, I can add those. Let me know.

@lsm5
Copy link
Member Author

lsm5 commented May 23, 2023

@edsantiago if it makes things easier, I can first push the rpm/podman.spec separately to dist-git rawhide, so that your new diff will be minimal. wdyt ?

@edsantiago
Copy link
Member

Thanks, but I'm good. Your explanation helped. Am in the middle of reviewing now.

Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

First pass done. Some concerns, some suggestions, but if this is working right now I'm fine with merging and then adding a followup PR. I'm reluctant to re-review this whole thing again :-)

rpm/podman.spec Outdated Show resolved Hide resolved
# Keep Version in upstream specfile at 0. It will be automatically set
# to the correct value by Packit for copr and koji builds.
# IGNORE this comment if you're looking at it in dist-git.
Version: 0
Copy link
Member

Choose a reason for hiding this comment

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

I might like to see a test like

%if %{version} == 0
bail out with a very loud message like "please read the specfile"

(to avoid someone just playing with it).

Again, OK to do later.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, yes. I think I can stick that in the build section. Thanks for the tip. Let me give it a try.

Copy link
Member Author

Choose a reason for hiding this comment

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

i'll leave this for the future. It's pretty much impossible for rpmbuild to proceed with the current spec file when Version is 0, so this version check isn't an urgent requirement.

rpm/podman.spec Outdated Show resolved Hide resolved
Comment on lines 3 to 5
# This script will update the Version field in the spec which is set to 0 by
# default. Useful for local manual rpm builds where the Version needs to be set
# correctly.
Copy link
Member

Choose a reason for hiding this comment

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

Comment is copypasted from the next script, and is misleading. And actually, since this script isn't even used, I might suggest removing it and adding it back when you fix the TODO. I won't block over that though.

Copy link
Member Author

Choose a reason for hiding this comment

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

err, yes I need to change that comment. I'm actually also checking with the packit people on the side.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated comment.

# default. Useful for local manual rpm builds where the Version needs to be set
# correctly.

SPEC_FILE=$(pwd)/podman.spec
Copy link
Member

Choose a reason for hiding this comment

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

Is this really $(pwd)? I looked in .packit.yml and don't see where it cd's to the rpm directory?

I would rather see something like $(dirname $0)/podman.spec, it seems safer. But it's very likely that I'm missing a key piece of Packit magic.

Copy link
Member Author

Choose a reason for hiding this comment

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

update-spec-version.sh is only if someone needs a manual build or use make package locally. Doesn't get used by packit. I can update the comment to make that clear.

@lsm5
Copy link
Member Author

lsm5 commented May 23, 2023

First pass done. Some concerns, some suggestions, but if this is working right now I'm fine with merging and then adding a followup PR. I'm reluctant to re-review this whole thing again :-)

If you don't wanna review the whole thing again, I can repush only with the $(hack/libdm_tag.sh) reinserted. Everything else can wait for followup PRs.

@edsantiago
Copy link
Member

Up to you. I'm comfortable using git range-diff for small to medium changesets.

Get rid of `podman.spec.rpkg` in favour of
`rpm/podman.spec` which gets synced with fedora dist-git on every
upstream release. The version in the new spec file is set to `0` by
default and gets updated by packit automatically on every packit task.

For local manual rpm builds using the spec, the helper script in the
`rpm/` subdir will update the Version field with the latest version
found in the upstream repo.

Packit will automatically create a PR on fedora dist-git on every new
upstream release. A sample PR will look like:
https://src.fedoraproject.org/rpms/container-selinux/pull-request/10#

A dry run for this can be triggered using:
`$ packit propose-downstream --local-content`

To run this command locally, you would need to have your packit
user-configuration-file set.
Ref: https://packit.dev/docs/configuration/#user-configuration-file

along with a fedora api key created at:
https://src.fedoraproject.org/settings#nav-api-tab with sufficient ACLs.

Also includes a revised `package` Makefile target which will build rpms
using `rpm/podman.spec`. Fixes: containers#18421.

[NO NEW TESTS NEEDED]

Signed-off-by: Lokesh Mandvekar <[email protected]>
@lsm5 lsm5 force-pushed the packit-downstream-sync branch from f0522b5 to 6003dca Compare May 23, 2023 20:45
@lsm5
Copy link
Member Author

lsm5 commented May 23, 2023

updated PR with libdm_tag re-added and better comments in spec and the update-spec-provides script. Also check https://src.fedoraproject.org/rpms/podman/pull-request/66#request_diff for a sample of the dist-git diff that would be created when we do a new release.

Btw, I've removed the selinux build tag because it's not needed anymore: https://github.com/opencontainers/selinux/blob/main/README.md#usage

@lsm5 lsm5 marked this pull request as draft May 24, 2023 13:48
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 24, 2023
@lsm5 lsm5 marked this pull request as ready for review May 24, 2023 14:43
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 24, 2023
@lsm5
Copy link
Member Author

lsm5 commented May 24, 2023

@edsantiago PTAL. If all the major comments are addressed, let's get this in. Thanks!

@lsm5
Copy link
Member Author

lsm5 commented May 24, 2023

Btw, I've submitted the spec file change to dist-git rawhide branch already, in case you wanna run another diff between spec files.

@edsantiago
Copy link
Member

git range-diff shows (most of) what I expected to see. The rest of my concerns can be addressed later (please don't forget :-)

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 24, 2023
@openshift-merge-robot openshift-merge-robot merged commit 74d804d into containers:main May 24, 2023
@lsm5
Copy link
Member Author

lsm5 commented May 24, 2023

/cherrypick v4.5

@openshift-cherrypick-robot
Copy link
Collaborator

@lsm5: #18465 failed to apply on top of branch "v4.5":

Applying: Packit: add jobs for downstream Fedora package builds
Using index info to reconstruct a base tree...
M	.packit.yaml
M	Makefile
M	podman.spec.rpkg
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): podman.spec.rpkg deleted in Packit: add jobs for downstream Fedora package builds and modified in HEAD. Version HEAD of podman.spec.rpkg left in tree.
Auto-merging Makefile
Auto-merging .packit.yaml
CONFLICT (content): Merge conflict in .packit.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Packit: add jobs for downstream Fedora package builds
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick v4.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lsm5 added a commit to lsm5/gvisor-tap-vsock that referenced this pull request May 24, 2023
Podman upstream now syncs the rpm spec file with Fedora dist-git.
This podman.spec also builds gvisor-tap-vsock as a subpackage which
conflicts with the standalone gvisor-tap-vsock package on podman-next.
Ref: containers/podman#18465

This commit will disable build tasks on `rhcontainerbot/podman-next` to
avoid installation conflicts.

The copr build tasks on every PR will still continue to check for any
potential build issues.

Signed-off-by: Lokesh Mandvekar <[email protected]>
cfergeau pushed a commit to containers/gvisor-tap-vsock that referenced this pull request Jun 7, 2023
Podman upstream now syncs the rpm spec file with Fedora dist-git.
This podman.spec also builds gvisor-tap-vsock as a subpackage which
conflicts with the standalone gvisor-tap-vsock package on podman-next.
Ref: containers/podman#18465

This commit will disable build tasks on `rhcontainerbot/podman-next` to
avoid installation conflicts.

The copr build tasks on every PR will still continue to check for any
potential build issues.

Signed-off-by: Lokesh Mandvekar <[email protected]>
@lsm5 lsm5 deleted the packit-downstream-sync branch June 20, 2023 14:25
lsm5 added a commit to lsm5/conmon-rs that referenced this pull request Jun 28, 2023
The packit workflow for Podman and other repos has recently changed to
include the official rpm spec file upstream and have Fedora dist-git
sync from upstream on every release.
Ref: containers/podman#18465

This has made it necessary to verify ELN package build success in our
copr builds. And enabling ELN on copr is causing conmon-rs failures
Ref: https://copr.fedorainfracloud.org/coprs/rhcontainerbot/podman-next/build/6121423/

Since ELN is not an immediate concern, this commit will only do copr
builds for centos stream and fedora rawhide, 38 and 37.

Signed-off-by: Lokesh Mandvekar <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 19, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable build of RPMs on RHEL 8
5 participants