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

.copr: Cleanups #1812

Merged
merged 4 commits into from
Aug 7, 2023
Merged

.copr: Cleanups #1812

merged 4 commits into from
Aug 7, 2023

Conversation

martinpitt
Copy link
Contributor

While looking at moving to packit srpm/rpm builds, I came across a few cleanups which should be done to the current build script.

@martinpitt
Copy link
Contributor Author

Ah, checkout@v2 is also deprecated, see the warnings in current runs. Added another commit.

Copy link
Member

@WOnder93 WOnder93 left a comment

Choose a reason for hiding this comment

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

Most of the commits seem fine except one (and two minor things in the rest) - please see the review comments.

rpm -q rpm-build git-core || dnf install -y rpm-build git-core
rpm -q rpm-build git-core
Copy link
Member

Choose a reason for hiding this comment

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

Nack. The .copr scripts are primarily intended for use with COPR's "make srpm" method, where the script is expected to install its dependencies. It is reused for GitHub CI because it fits that use case as well and it means less duplicate code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok -- I was going to throw it out as the next step anyway, when moving the whole build-rpms workflow and copr handling to packit (I talked to @zpytela about it yesterday). See martinpitt#1 . I'm happy to revert this, it's not important at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

Copy link
Contributor Author

@martinpitt martinpitt Aug 2, 2023

Choose a reason for hiding this comment

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

While I'm at it, do you have a copr where you do builds from the "rawhide" branch, i.e. from landed commits? I'm happy to add that to the packit config, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a copr, but scratchbuilds are available at Checks -> Artifacts -> rpms.zip - is that what you meant?

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to keep .copr/ in the repo. I use it when I have a branch with some testing changes in a branch on my fork - having the .copr directory there allows me to set up automatic COPR builds without needing to add any special changes. I would like this to be preserved along with the Packit support. I guess the make-srpm.sh script will need to be massaged a bit to be usable for both, but hopefully it won't be too complicated.

There is also https://copr.fedorainfracloud.org/coprs/omos/selinux-policy-latest that is set to build the latest rawhide branch content after every push, but I think no one actually uses it (and I presume it would be made obsolete by the Packit support).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Packit COPRs also "just work" -- I forked the project and sent martinpitt#1 (to my own fork, as it's building on top of this), and if you follow the "rpm-build*" test contexts, you get to https://dashboard.packit.dev/results/copr-builds/912150 -- i.e. that just effortlessly works on forks as well, which is great for experiments and collaboration. That said, I can rework this to work in both cases, maybe with splitting up the script.

Your omos/selinux-policy-latest copr, you said "is set to" -- does copr itself provide automatic builds? If so, that will continue to work of course (I wasn't aware that COPR can do that). Otherwise we can add a copr on-commit job to do that, provided that you allow the "packit" user as a builder in your copr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a copr, but scratchbuilds are available at Checks -> Artifacts -> rpms.zip - is that what you meant?

@zpytela : No, that would be builds from the proposed branches, not after the merge to the main branch ("rawhide" or whichever the target is). I meant the kind of COPR that @WOnder93 mentioned above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this discussion isn't really relevant for this PR, but very much so for the next one that I'm going to send after this lands 😁 so thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FTR: I understand that you are cautious here. I'll rework the "packit copr" PR to keep .copr/ functional as-is, and instead provide an option for make-srpm.sh to skip the rpm-build step.

.github/workflows/build.yml Outdated Show resolved Hide resolved
.copr/make-srpm.sh Outdated Show resolved Hide resolved
Also rename it from `$dirname` to `$rootdir`, as that's too confusing
with the `dirname` command.
This catches a missing $1 argument early, instead of failing the `cp`
command at the very end.
As a user this isn't necessary, and potentially dangerous when running
this as root. This is a workaround which is specific to running
containers in GitHub workflows, so configure git there.
v2 has been deprecated for a long time, and is throwing warnings.
@martinpitt martinpitt requested a review from zpytela August 4, 2023 05:23
@martinpitt
Copy link
Contributor Author

@zpytela Any chance that this can be landed? This PR should be quite harmless. Thanks!

@zpytela
Copy link
Contributor

zpytela commented Aug 7, 2023

Merging now, thank you.

@zpytela zpytela merged commit d2c016a into fedora-selinux:rawhide Aug 7, 2023
2 checks passed
@martinpitt martinpitt deleted the copr-cleanups branch August 8, 2023 04:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants