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

rpmspec: Use NEVRA for binary packages queries #2995

Closed
wants to merge 2 commits into from

Conversation

ffesti
Copy link
Contributor

@ffesti ffesti commented Mar 26, 2024

This fixes a regression that was present since 4.18 that printed them out as source/nosource packages as the headers are no proper binary packages. Setting the default query to "%{NEVRA}\n" forces the right output.

Resolves: #2819

@pmatilai
Copy link
Member

This wont fix it for other potential users of archsuffix though.
Let's at least see if there is a way to fix the issue at the source - maybe archsuffix can do better heuristics than the simple-minded headerIsSource() or .. something.

@ffesti
Copy link
Contributor Author

ffesti commented Mar 26, 2024

May be headerIsSource() should be more correct... I'll look into it

@ffesti
Copy link
Contributor Author

ffesti commented Mar 26, 2024

Now to something completely different...

What about just setting the SOURCERPM tag for binary packages that are parsed for querying? One could argue that we should move more of this kind of initialization to the parse stage so we get a more complete result earlier. But that is a crusade I am not willing to embark on for this kind of bug. There is also the question if that is really a smart move, given that with dynamic specs features we' rather set things late.

So this is probably the cleanest solution for now that won't affect anything else.

Avoid packages being detected as srpms erroneously

Resolves: rpm-software-management#2819
@pmatilai
Copy link
Member

Yeah, (looking into) populating SOURCERPM early was what I meant with fixing at the source. Based on a quick look, you could just move SOURCERPM insertion from processBinaryFiles() to, say, finalizeSpec() and be done with it with equal amount of work, without adding duplicate special paths.

@ffesti
Copy link
Contributor Author

ffesti commented Apr 2, 2024

OK, I looked into this and there are road blocks everywhere. Let's just stick to the PR as is. I generally agree that this is not the way to do this but the build code is an entangled mess and moving stuff round at this point is something we just should not do.

Looking at all the hidden assumptions and internal dependencies in the code is a story for another time and goes far beyond this issue.

runroot rpmspec --rpms \
-q /data/SPECS/hello.spec | grep src
runroot rpmspec --rpms \
-q /data/SPECS/hello.spec | cut -d. -f-2
Copy link
Member

Choose a reason for hiding this comment

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

Put these two checks into separate RPMTEST_CHECK() blocks, it makes the tests far more reliable when you don't need to guess which part output what. There are a bunch of tests that do this before we learned that you can have many checks inside a setup/clean pair 😅 but it's a bad practise we should not do in new tests.
Also, these are not writing anything so you don't need RPMDB_INIT (which has non-zero cost).

],
[0],
[hello-1.0-1.src
],
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 put this in the same SETUP/CLEANUP group as the --rpms tests, just in its own RPMTEST_CHECK() block. These have a not-entirely-trivial cost especially now in the containerized setup. This doesn't write anything either so RPMDB_INIT is unncessary (and lack of which actually guards against things unexpectedly starting to write stuff too).

@pmatilai
Copy link
Member

pmatilai commented Apr 2, 2024

The build code is a mess for sure, but adding hacks on top of hacks only makes it worse.

@pmatilai
Copy link
Member

pmatilai commented Apr 2, 2024

Took a quick look at my own suggestion in the earlier comment and it brings out some truly WTF failures 😆

@pmatilai
Copy link
Member

pmatilai commented Apr 2, 2024

Here's a simple and straightforward way source headers are always indentified as such right after parse: #3012

@pmatilai
Copy link
Member

pmatilai commented Apr 2, 2024

Aaargh, except that the issue here was not positively identifying source headers but binaries 🤦

@pmatilai
Copy link
Member

pmatilai commented Apr 2, 2024

Added a second commit there to deal with RPMTAG_SOURCERPM too:
cb47d1e

@ffesti
Copy link
Contributor Author

ffesti commented Apr 3, 2024

Merged #3012 instead

@ffesti ffesti closed this Apr 3, 2024
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.

rpmspec default output unexpected
2 participants