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

Add rpmautospec plugin #1253

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

nphilipp
Copy link
Contributor

@nphilipp nphilipp commented Nov 9, 2023

This plugin preprocesses spec files using %autorelease and/or %autochangelog in the mock chroot prior to building SRPMs. The rpmautospec package will be installed into the chroot if necessary.

/cc @sgallagher

@nphilipp nphilipp marked this pull request as draft November 9, 2023 14:27
@nphilipp
Copy link
Contributor Author

nphilipp commented Nov 9, 2023

The rpm-build:fedora-rawhide-x86_64:mock check failed because the unit tests depend on the rpmautospec-core Python package which needs to be available as an RPM package in Fedora, see fedora-infra/rpmautospec-core#10, and mock.spec needs to be adapted, too.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

vcs-diff-lint found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@praiskup
Copy link
Member

Thank you for your contribution! Nice.

Can you please document the plugin https://rpm-software-management.github.io/mock/#plugins ?

Some of the new PyLint warnings are too pedantic (I'm going to tweak the
rules), though some are worth fixing (plus I'd vote for having at least
some of those non-test doc strings written, not a hard requirement).

The rpmautospec package will be installed into the chroot if necessary.

But we also require it also host because of
from rpmautospec_core import specfile_uses_rpmautospec.

Have you considered calling the host tooling (on the sources stored in-chroot)?

@praiskup
Copy link
Member

Also, can you please write a release notes entry, template:

$ cat releng/release-notes-next/autospec.feature 
New plugin ... [has been implemented][PR#1254] ...
...

mock/tests/compat.py Fixed Show resolved Hide resolved
mock/tests/plugins/test_rpmautospec.py Fixed Show resolved Hide resolved
mock/tests/plugins/test_rpmautospec.py Fixed Show resolved Hide resolved
mock/tests/plugins/test_rpmautospec.py Fixed Show fixed Hide fixed
@praiskup
Copy link
Member

Can you please also add a "commented out" plugin-config into the docs file ./mock/docs/site-defaults.cfg? We document all the available options there.

mock/tests/conftest.py Fixed Show fixed Hide fixed
mock/tests/conftest.py Fixed Show fixed Hide fixed
mock/mock.spec Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Dec 4, 2023

Hello @nphilipp! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 98:21: W503 line break before binary operator

Line 199:21: W503 line break before binary operator

Comment last updated at 2023-12-07 13:16:31 UTC

@nphilipp
Copy link
Contributor Author

nphilipp commented Dec 4, 2023

Hello @nphilipp! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

* In the file [`mock/py/mockbuild/plugins/rpmautospec.py`](https://github.com/rpm-software-management/mock/blob/0fd14985906a58701acc8f6697735195364b1a60/mock/py/mockbuild/plugins/rpmautospec.py):

Line 98:21: W503 line break before binary operator

* In the file [`mock/tests/plugins/test_rpmautospec.py`](https://github.com/rpm-software-management/mock/blob/0fd14985906a58701acc8f6697735195364b1a60/mock/tests/plugins/test_rpmautospec.py):

Line 199:21: W503 line break before binary operator

This actually conforms to what PEP8 says about breaking lines around binary operators: https://peps.python.org/pep-0008/#should-a-line-break-before-or-after-a-binary-operator

@praiskup
Copy link
Member

praiskup commented Dec 7, 2023

Two missing things:

  • No matching package to install: 'python3-rpmautospec-core' in EPEL 8.
  • documenting why we need to run the in-chroot autospec tooling

@praiskup praiskup mentioned this pull request Dec 7, 2023
4 tasks
@nphilipp nphilipp marked this pull request as ready for review December 7, 2023 12:32
@nphilipp
Copy link
Contributor Author

nphilipp commented Dec 7, 2023

I’ve set the PR to be ready for review (which I should have done a while ago already).

Two missing things:

* `No matching package to install: 'python3-rpmautospec-core'` in EPEL 8.

The new package update has been in testing for a couple of days: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2023-9e47a8b73c

I’m trying to get karma for it 😉.

* documenting why we need to run the in-chroot autospec tooling

I'll do this (and squash the commits into one).

This plugin preprocesses spec files using %autorelease and/or
%autochangelog in the mock chroot prior to building SRPMs. The
`rpmautospec` package will be installed into the chroot if necessary.

Co-authored-by: Nils Philippsen <[email protected]>
Signed-off-by: Stephen Gallagher <[email protected]>
Signed-off-by: Nils Philippsen <[email protected]>
@nphilipp
Copy link
Contributor Author

nphilipp commented Dec 7, 2023

@praiskup the update should be available with the next EPEL8 compose and I’ve added the reasons for doing this in the chroot as a comment.

Copy link
Member

@praiskup praiskup left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

@praiskup praiskup merged commit 21581da into rpm-software-management:main Dec 8, 2023
16 checks passed
@praiskup
Copy link
Member

Have you considered calling the host tooling (on the sources stored in-chroot)?

Just getting back to this; I still think this is a bad idea.

The Mock's mission should be to abstract from the in-chroot nuances, and rely on "easily fixable" on-host tooling (by example, that way you can bootstrap a new distribution without having the autospec tooling built yet, or build fedora packages with autospec against distributions that do not support it).

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.

4 participants