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

ruby bindings: Fix namespacing for libdnf5 classes/modules #2021

Merged
merged 2 commits into from
Jan 31, 2025

Conversation

jackorp
Copy link
Contributor

@jackorp jackorp commented Jan 24, 2025

Closes: #1779

The ruby/CMakeLists.txt also defined -module twice. Once "by hand" appending the raw -module ${MODULENAME} and then it is also added via the set_property for SWIG_MODULE_NAME. Once is enough, so I just deleted appending it to CLI and just adjusted the set_property call. This results in all constants in Ruby now prepended with Libdnf5, therefore preventing top-level constant collision (and allowing me to get on with my project).

By the way since setting SWIG_MODULE_NAME appends -module to swig CLI, it is overriding what is present in the files in the %module directive. Both Perl and Ruby are setting -module which makes specifying the module path via %module relatively useless even if there seems to be intent, judging by the fact that they are guarded in module files via #if defined.

JFTR not sure if there is or isn't possibility or some sane way of flattening the namespaces, Logger::Logger::$LOG_LEVEL is a bit unwieldy.

`%module "libdnf5/$module"` is not a valid swig module definition and results in
bogus code being generated. It is currently overridden from the
commandline with the `-module` option passed to swig.
Using just ${MODULE_NAME} in binding's CMakeLists.txt will result in the
CLI appending -module which overrides definition in files. Prepend
${LIBRARY_NAME} to ${MODULE_NAME} to namespace the libdnf5 API in Ruby,
otherwise there is high and real risk of constant collision.

For example Ruby's default gem 'logger' also specifies Logger constant
however as a class and not a module unlike libdnf5. Trying to use
Logger constant if we have both libdnf5's logger and Ruby's logger required
in Ruby code will result in an exception without namespacing.

Inspired by the contents of a similar function in perl5's
CMakeLists.txt.
@jrohel
Copy link
Contributor

jrohel commented Jan 30, 2025

As a result, all constants in Ruby are now appended with Libdnf5, which prevents top-level constants from colliding (and I can continue my project).

This seems like the right modification. In other languages, we use the libdnf5 namespace. I'm wondering why we don't have that from the start for ruby as well.

I'm also considering how many ruby API users we have. This is an API breaking change. It's easily fixable on the user side. But it is a change.

@jrohel jrohel self-assigned this Jan 30, 2025
@jackorp
Copy link
Contributor Author

jackorp commented Jan 30, 2025

This seems like the right modification. In other languages, we use the libdnf5 namespace. I'm wondering why we don't have that from the start for ruby as well.

Seems since "Initial project template" 5bb316d ruby/CMakeLists.txt was wrong or MODULE_NAME had different value than intended judging by the following being present:

+    set_property(SOURCE ../../${LIBRARY_NAME}/${MODULE_NAME}.i PROPERTY SWIG_MODULE_NAME ${MODULE_NAME})
+    set(CMAKE_SWIG_FLAGS -module ${MODULE_NAME} -ruby)
+    swig_add_library(${TARGET_NAME} LANGUAGE ruby SOURCES ../../${LIBRARY_NAME}/${MODULE_NAME}.i)

For comparison, relevant section of perl5 cmake in that same commit:

+    set_property(SOURCE ../../${LIBRARY_NAME}/${MODULE_NAME}.i PROPERTY SWIG_MODULE_NAME ${LIBRARY_NAME}::${MODULE_NAME})
+    swig_add_library(${TARGET_NAME} LANGUAGE perl SOURCES ../../${LIBRARY_NAME}/${MODULE_NAME}.i)

perl does not have the set() directive and set_property() contains ${LIBRARY_NAME}::${MODULE_NAME} instead of just ${MODULE_NAME}

Not sure if there is more history of this somewhere but 2020 is the most I see for these files in this repository.

I'm also considering how many ruby API users we have. This is an API breaking change. It's easily fixable on the user side. But it is a change.

That I do not know, but I can say confidently if they are using libdnf5, they aren't using the default/bundled logger gem that comes with Ruby :D (unless they are jumping through some serious hoops to make it work).

@jrohel
Copy link
Contributor

jrohel commented Jan 31, 2025

Thank you for the PR.

We will have to move libdnf5 classes/modules out of the global namespace. This will cause the API change. That's not good. But doing it later will be even more of a pain for all.
Ruby support is in a "best effort" state. From that perspective, it's best to make the change as soon as possible.
I will merge this PR.

@jrohel jrohel added this pull request to the merge queue Jan 31, 2025
Merged via the queue into rpm-software-management:main with commit 875db84 Jan 31, 2025
11 of 17 checks passed
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.

Ruby bindings RFE: libdnf5 modules and classes should be namespaced.
2 participants