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.

Sign up for GitHub

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

Jump to bottom

Do not export internal symbols in shared object files #1307

Merged
merged 9 commits into from
Jun 10, 2024
Merged

Do not export internal symbols in shared object files #1307

merged 9 commits into from
Jun 10, 2024

Conversation

jrohel
Copy link
Contributor

@jrohel jrohel commented Mar 12, 2024 •
edited
Loading

Closes #1304

PR disables C++ unit tests for libdnf5, libdnf5-cli, dnf5 cpr_plugin C++. These unit tests use private symbols that are no longer accesible. It is a temporarily solution until the unit tests are modified.

ppisar
ppisar reviewed Mar 13, 2024
View reviewed changes
include/libdnf5/defs.h Outdated
#ifndef LIBDNF5_DEFS_H
#define LIBDNF5_DEFS_H

#define PUBLIC_API __attribute__((visibility("default")))
Copy link
Contributor

Choose a reason for hiding this comment

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

attribute((visibility())) is a compiler-specific extension. E.g. GCC and Clang have them. Other compilers might not have. Consider guarding the definition with GNUC preprocessor condition.

@jrohel jrohel force-pushed the feature/hide_internal_symbols branch from 8749722 to f89223e Compare March 22, 2024 08:19
@jrohel jrohel force-pushed the feature/hide_internal_symbols branch from f89223e to 94f3049 Compare April 11, 2024 11:19
@jrohel jrohel force-pushed the feature/hide_internal_symbols branch 2 times, most recently from 0cb8168 to a46b6d1 Compare May 21, 2024 12:46
@jrohel jrohel added blocked Further work on issue or PR is blocked by something else and removed work in progress labels May 21, 2024
@jrohel jrohel force-pushed the feature/hide_internal_symbols branch from a46b6d1 to bd98ccb Compare May 23, 2024 10:33
@jrohel jrohel removed the blocked Further work on issue or PR is blocked by something else label May 23, 2024
@jrohel jrohel mentioned this pull request May 23, 2024
Merged
@jan-kolarik jan-kolarik self-assigned this May 27, 2024
@jrohel jrohel force-pushed the feature/hide_internal_symbols branch from bd98ccb to b77ef6a Compare May 28, 2024 09:16
@jrohel
Copy link
Contributor Author

jrohel commented May 28, 2024

I did a rebase to resolve the conflicts.

@jan-kolarik
Copy link
Member

There is some issue with loading builddep plugin: Cannot load dnf5 plugins: builddep_cmd_plugin.so. Though it rather seems like an issue with some older build from this PR as the version is 5.2.1.0-1.20240521115637370466.pr1307.37.g0cb81683.fc41. Maybe you would have to create a new PR, I've also encountered something similar recently. It was a Packit-related issue. It could be the same problem here.

jan-kolarik
jan-kolarik reviewed May 29, 2024
View reviewed changes
dnf5/include/dnf5/context.hpp Show resolved Hide resolved
bindings/libdnf5/advisory.i Outdated Show resolved Hide resolved
@jrohel jrohel force-pushed the feature/hide_internal_symbols branch from b77ef6a to a57932c Compare May 30, 2024 12:52
@jan-kolarik
Copy link
Member

Thanks, looks good. Just a rebase is probably needed to fix tests.

@jrohel jrohel force-pushed the feature/hide_internal_symbols branch from a57932c to 35030a2 Compare May 31, 2024 12:41
@jrohel
Copy link
Contributor Author

jrohel commented May 31, 2024

The Locker class has been moved to the public API in upstream.
So, I did a rebase and modification of this PR.

@jan-kolarik
Copy link
Member

jan-kolarik commented Jun 4, 2024 •
edited
Loading

The current tests issues are connected with the new rpm 4.19.91-6 in Rawhide. A subsequent update 4.19.91-7 should fix the issues (tested locally). Waiting to get into compose and we can re-run the tests...

@jan-kolarik
Copy link
Member

Sorry about that, but could you rebase again please? From the other PRs, it seems that the CI is finally working again...

@jrohel jrohel force-pushed the feature/hide_internal_symbols branch from 35030a2 to b9be10d Compare June 6, 2024 08:55
@jrohel
dnf5: Define macros for assigning symbol visibility
e21bd20
@jrohel
dnf5 app, dnf5 plugins: Do not export private symbols
929760e
@jrohel
Do not export symbols from private "/common/utils"
7a86447 
Libraries and applications (libdnf5.so, dnf5, dnf5daemon-client...) use
(and statically link) private utilities from "/common/utils".
And they also publicly export the symbols of these private utilities.

This modification hides the symbols of utilities linked from
"/common/utils".
@jrohel jrohel force-pushed the feature/hide_internal_symbols branch from b9be10d to 7a86447 Compare June 10, 2024 09:30
@jan-kolarik jan-kolarik added this pull request to the merge queue Jun 10, 2024
Merged via the queue into rpm-software-management:main with commit 9b8f32d Jun 10, 2024
3 of 15 checks passed
kontura added a commit to kontura/dnf5 that referenced this pull request Jun 10, 2024
@kontura
Mark goal_action_is_replay as LIBDNF_API to fix build
362e2c2 
PR that adds the API (rpm-software-management#1452)
was merged right before rpm-software-management#1307
so its missing there.
@kontura kontura mentioned this pull request Jun 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppisar ppisar ppisar left review comments

@jan-kolarik jan-kolarik jan-kolarik approved these changes

Assignees

@jan-kolarik jan-kolarik

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Do not export internal symbols in shared object files (libraries)
3 participants
@jrohel @jan-kolarik @ppisar