-
Notifications
You must be signed in to change notification settings - Fork 788
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 #2011
Conversation
d147a18
to
957fce8
Compare
cd309dd
to
a931109
Compare
Successful copr build at: https://copr.fedorainfracloud.org/coprs/rhcontainerbot/packit-builds/build/6058022/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - other than a few total-guesses at things that might need fixing.
rpm/update-spec-provides.sh
Outdated
|
||
sed -i '/Provides: bundled(golang.*/d' $SPEC_FILE | ||
|
||
GO_IMPORTS=$(golist --imported --package-path github.com/containers/$PACKAGE --skip-self | sort -u | xargs -I{} echo "Provides: bundled(golang({}))") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure in this case, but might confirm that -I{}
doesn't need to be quoted to protect it from bash gobbling it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cevich not sure I understand you. In the fedora spec file, I need:
# DO NOT DELETE BELOW LINE - used for updating downstream goimports
# vendored libraries
Provides: bundled(golang(github.com/containers/common/pkg/auth))
Provides: bundled(golang(github.com/containers/common/pkg/flag))
Provides: bundled(golang(github.com/containers/common/pkg/report))
...
...
I tried with quotes "-I{}"
but that didn't work.
rpm/skopeo.spec
Outdated
%changelog | ||
%if %{with manual_changelog} | ||
* Mon May 01 2023 RH Container Bot <rhcontainerbot@fedoraproject.org> | ||
- Placeholder changelog for envs that are not autochangelog-ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's a placeholder it's probably minor, but is RH Container Bot <rhcontainerbot@fedoraproject.org>
still relevant here (since automation is going to packit)? Might consider updating this (and the date) if not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changelog entry was copy-pasta from podman spec. This changelog entry is only so that copr builds on centos 8 stream do not complain about no changelog. The copr repos are still owned by rhcontainerbot
user.
The changelog entry is not expected to be updated regularly, and might even go away once centos 8 stream gets autochangelog support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the date in changelog entry along with some comments.
rpm/skopeo.spec
Outdated
export CGO_CFLAGS=$CFLAGS | ||
|
||
%if %{without btrfs} | ||
export BUILDTAGS="exclude_graphdriver_btrfs btrfs_noversion" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In .cirrus.yml
the BUILDTAGS are 'btrfs_noversion libdm_no_deferred_remove containers_image_openpgp
. Is the value in the spec file correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, this is what we've had in the fedora spec file all this while. Let me get back on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I can just include what the upstream makefile includes by default. Looking in the current skopeo.spec.rpkg on main, it only has libdm_tag.sh apart from the btrfs tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try to use Makefile targets for rpm builds as well and see how those go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
giving up on Makefile targets in %build section for now. Adjusted buildtags to match upstream Makefile's.
I'm gonna try disabling |
b51e09d
to
8c6ff59
Compare
alright, I think I have addressed all comments. Added some additional comments in the spec file and also got rid of PTAL. |
err, once again I spoke too soon. https://copr.fedorainfracloud.org/coprs/rhcontainerbot/packit-builds/package/skopeo/ something seems wrong with version fetching. |
So, using packit's defaults means the Version string is fetched using: I guess upstream doesn't need to concern itself too much with all this, more of a copr usability and breakage thing. |
set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the current build has version 1.12.0 and not 1.12.1~dev. If that's acceptable, then we should be ok.
The currently-generated version numbers use the snapshot syntax, that’s a clear enough indication of what the sources are.
Get rid of `skopeo.spec.rpkg` in favour of `rpm/skopeo.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. 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. This patch incurs zero additional cost to upstream as the downstream tasks occur only after upstream release and do not block anything upstream. [NO NEW TESTS NEEDED] Co-authored-by: Miloslav Trmač <mitr@redhat.com> Signed-off-by: Lokesh Mandvekar <lsm5@fedoraproject.org>
@mtrmac all comments should be addressed now. Marking ready for review. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
Get rid of
skopeo.spec.rpkg
in favour ofrpm/skopeo.spec
which gets synced with fedora dist-git on every upstream release. The version in the new spec file is set to0
by default and gets updated by packit automatically on every packit task.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.
This patch incurs zero additional cost to upstream as the downstream
tasks occur only after upstream release and do not block anything
upstream.
[NO NEW TESTS NEEDED]