-
Notifications
You must be signed in to change notification settings - Fork 58
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
Move package NEVR parsing to builder #2800
Conversation
Some of the backend tests are failing. That's because we need to add {
"name": "example",
"epoch": null,
"version": "1.0.14",
"release": "1",
"exclusivearch": [],
"excludearch": []
} The problem is, this test directory is for RPM build, not SRPM. We are only abusing it for testing SRPM builds as well. So this will require a bit more changes. But the code itself works fine. |
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.
thanks for the PR!
I just have one question below but apart from that +1
""" | ||
Queries a package for its name and evr (epoch:version-release) | ||
""" | ||
cmd = ['rpm', '-qp', '--nosignature', '--qf', |
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.
What happens if specfile parsing fails for some reason (other than no spec file found), would this backup approach fail too? Are we giving up the backup approach and we trust specfile library in every case?
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 don't know :-).
But we should cover it in tests for sure.
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.
@praiskup, as @nikromen pointed out, I completely removed the pkg_name_evr_from_srpm
function and rely solely on parsing version and name from the spec file. Do want to go with this? Or should I move the pkg_name_evr_from_srpm
function to builder as well and use it if something goes wrong when parsing the spec file?
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.
That was kind of a safety belt (I still don't trust python-specfile that much, also - newly it will be run on multiple architectures). But I won't argue too much on this :-)
It contains `results.json` generated by code from fedora-copr/copr#2800
+1, but these two are worth fixing
|
64acf99
to
e5d33ab
Compare
Fixed linter errors and tests, PTAL |
rpmbuild/copr-rpmbuild.spec
Outdated
BuildRequires: python3-backoff >= 1.9.0 | ||
|
||
%if 0%{?rhel} == 8 | ||
BuildRequires: %{python_pfx}-specfile |
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 submited the issue packit/specfile#255 for 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 tested a bit specfile v0.20 on epel8 and wasn't able to make it work. So I don't think there's an easy way to update specfile into EPEL8 (missing setup.py, incompatible (too new) typing, I stopped there...).
The question is - is the python-specfile in epel 8 "new enough" for us (100% working, but slow), or does this actually mean we have to drop the support for EPEL8?
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.
Getting to EPEL8 https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2023-8a36e6e848 so we can probably drop the %if/%else
@FrostyX @nikromen can you give +1 to https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2023-8a36e6e848? |
e5d33ab
to
e605925
Compare
e605925
to
e970e2c
Compare
I added a simple beaker test and the |
Fix #2790
Fix #2777