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

Parsing a specfile multiple times leaves the lua tables dirty #185

Closed
hroncok opened this issue Oct 3, 2024 · 3 comments · Fixed by #186
Closed

Parsing a specfile multiple times leaves the lua tables dirty #185

hroncok opened this issue Oct 3, 2024 · 3 comments · Fixed by #186
Assignees
Labels
bug Something isn't working needs-infra-deployment Changes that need to be deployed in infrastructure

Comments

@hroncok
Copy link
Contributor

hroncok commented Oct 3, 2024

So, I recently discovered a weird sitation.

Consider https://src.fedoraproject.org/rpms/fedora-obsolete-packages with this:

%obsolete_ticket https://bugzilla.redhat.com/show_bug.cgi?id=2307970
%obsolete gimp-save-for-web 0.29.3-20
%obsolete_ticket https://bugzilla.redhat.com/show_bug.cgi?id=2307966
%obsolete gimp-layer-via-copy-cut 1.6-27
%obsolete_ticket https://bugzilla.redhat.com/show_bug.cgi?id=2307966
%obsolete gimp-layer-via-copy-cut 1.6-30

Regular RPM passing errors like this -- this is expected and correct:

$ rpmspec -P fedora-obsolete-packages.spec
error: gimp-layer-via-copy-cut obsoleted multiple times (1.6-27 and 1.6-30).
error: lua script failed: [string "obsolete"]:27: error expanding macro

However, rpmautospec (e.g. via fedpkg verrel) errors like this:

$ fedpkg verrel
Could not execute verrel: Couldn’t parse spec file fedora-obsolete-packages.spec:
error: gimp-save-for-web obsoleted multiple times (0.29.3-20 and 0.29.3-20).
error: lua script failed: [string "obsolete"]:27: error expanding macro

This is certainly unexpected: gimp-save-for-web is not obsoleted multiple times.

Adding this to the beginning of the spec:

%{lua:obs = {}}

Makes the problem go away.

What happens is that the specfile does not suspect it will be parsed twice with the same Lua state. But rpmautospec does that in

for spec_candidate in (abridged.name, str(specfile)):

...which is kinda my doing :/

The easy solution is to move the macro definitions into the for:

rpm.addMacro("_invalid_encoding_terminates_build", "0")
# rpm.addMacro() doesn’t work for parametrized macros
rpm.expandMacro(f"%define {AUTORELEASE_MACRO} {autorelease_definition}")
rpm.addMacro("autochangelog", "%nil")
rpm.addMacro("__python", f"/usr/bin/python{python_version}")
rpm.addMacro("python_sitelib", f"/usr/lib/python{python_version}/site-packages")
rpm.addMacro("_sourcedir", f"{path}")
rpm.addMacro("_builddir", f"{path}")

And call rpm.reloadConfig() after each attempt.

However, that might be overkill. I will see if we can "restart" the Lua state somehow.

@pmatilai
Copy link

pmatilai commented Oct 4, 2024

It's not just Lua state, spec parsing also affects global macro state so parsing specs without resetting the entire rpm context in between can and will yield different results. So, rpm.reloadConfig() before another parse (whether the same spec or different) is the right thing to do, as blunt as that hammer may seem.

I would love to have a have the entire spec parse context isolated to the spec object itself, but that's a long road.

@pmatilai
Copy link

pmatilai commented Oct 4, 2024

Come to think of it, this little fact that you need to call rpm.reloadConfig() probably isn't documented anywhere...

hroncok added a commit to hroncok/rpmautospec that referenced this issue Oct 4, 2024
hroncok added a commit to hroncok/rpmautospec that referenced this issue Oct 4, 2024
hroncok added a commit to hroncok/rpmautospec that referenced this issue Oct 4, 2024
nphilipp pushed a commit to hroncok/rpmautospec that referenced this issue Oct 10, 2024
@nphilipp
Copy link
Member

Good catch, thanks for digging into the problem and implementing a solution!

@nphilipp nphilipp self-assigned this Oct 10, 2024
@nphilipp nphilipp added bug Something isn't working needs-infra-deployment Changes that need to be deployed in infrastructure labels Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-infra-deployment Changes that need to be deployed in infrastructure
Projects
Development

Successfully merging a pull request may close this issue.

3 participants