Skip to content

Conversation

@dmnks
Copy link
Contributor

@dmnks dmnks commented Nov 27, 2025

Another take at #4053.

@dmnks dmnks requested a review from a team as a code owner November 27, 2025 16:43
@dmnks dmnks requested review from pmatilai and removed request for a team November 27, 2025 16:43
Turns out some platforms (such as AIX) don't implement these.

Don't compile it just yet, it needs some fixing up, which will be done
in the next commit.

Related: rpm-software-management#4053

This reverts commit fa06b68.
Fix minor issues with the original code reverted in the previous commit
that prevented it from compiling cleanly (mostly around the const usage)
and actually enable it if no system implementation is available (such as
on AIX).

Note that this new source file is tiny so it's included in libmisc
instead of a new target, and librpm is linked against it.

Related: rpm-software-management#4053
@dmnks
Copy link
Contributor Author

dmnks commented Nov 27, 2025

Note that it's split into two commits (revert of the original + fixups) to make reviewing easier. I can squash it afterwards (or we can leave it this way if you prefer so).

@pmatilai
Copy link
Member

pmatilai commented Nov 28, 2025

Hmm. The idea of reviving the original assumed the original implementation made sense, but it doesn't, really. Which must've contributed to it getting deleted.

Thing is, this is an incredibly fiddly and fragile tangle of ifdefs and weird symbols scattered around, and impossible to test outside some specific platform that doesn't match the previous tangle of stuff.

The sane way to do such portability wrappers is to have proper functions that are always compiled on all platforms, and then handle all the platform specifics inside that one source file, and provide them in the public API. For rpm this would mean something like rgetprogname() and rsetprogname() in librpmio. include/rpm/rpmutil.h is a fine place for these. And none of these to-link-or-not-to-link and weird _rpmxgrubfargh stuff 😄 It'll also make system.h smaller, which is always a good sign.

@dmnks
Copy link
Contributor Author

dmnks commented Nov 28, 2025

Yep, I did test this by removing the cmake & header conditionals but otherwise agree, it's kinda messy this way. The precedent where we use our own r*() wrappers makes sense, indeed. Let's try that, then 👍

@dmnks dmnks marked this pull request as draft November 28, 2025 10:49
@pmatilai
Copy link
Member

FWIW, my recollection of the original implementation was that it did something like that, but it was far messier than my memory (a rare occurence, that 😆 )

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.

2 participants