-
Notifications
You must be signed in to change notification settings - Fork 72
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
bindings: Fix and enable builds with Go bindings #1089
base: main
Are you sure you want to change the base?
Conversation
@@ -50,7 +50,7 @@ option(WITH_DNF5DAEMON_TESTS "Build dnf5daemon tests (requires configured dbus)" | |||
option(WITH_SANITIZERS "Build with address, leak and undefined sanitizers (DEBUG ONLY)" OFF) | |||
|
|||
# build options - bindings | |||
option(WITH_GO "Build Go bindings" OFF) | |||
option(WITH_GO "Build Go bindings" ON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we probably don't want to turn it on by default just yet.
I have spent some time trying it out and I don't think the bindings actually work.
Were you able to use them?
I would like to set up some tests the way we have for Python or Perl before we enable them.
@@ -101,7 +101,7 @@ struct Vars { | |||
/// @param name Name of the variable | |||
const Variable & get(const std::string & name) const { return variables.at(name); } | |||
|
|||
static std::unique_ptr<std::string> detect_release(const BaseWeakPtr & base, const std::string & install_root_path); | |||
static std::string detect_release(const BaseWeakPtr & base, const std::string & install_root_path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot do that, it would break API.
Either we could target this PR to the dnf5-5.2.0.0
branch (which is a new major release with breaking changes) or we have to find another way. Perhaps we could simply ignore the function in go bindings for now.
@@ -33,8 +33,9 @@ namespace libdnf5 { | |||
/// Base class for configurations objects | |||
class Config { | |||
public: | |||
#ifndef SWIGGO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather keep theseSWIGGO
conditionals only in the interface files.
I think it could look something like:
#if defined(SWIGGO)
%ignore libdnf5::Config::opt_binds;
#endif
@@ -53,8 +53,13 @@ class RepoCache { | |||
public: | |||
using RemoveStatistics = RepoCacheRemoveStatistics; | |||
|
|||
/// The name of the attribute used to mark the cache as expired. | |||
/// The name of the attribute used to mark the cache as expired. | |||
#ifndef SWIGGO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well, please keep the SWIGGO
conditions only in the *.i
files.
Fixes #719
Ok, disclaimer, I am neither a SWIG expert nor a dnf one but I was just poking around the codebase.
With that being clear, its a rough take on exposing the API.
std::unique_ptr
, which I don't know if important or not.constexpr const char*
which is a bit annoying but can be worked aroundDefinitely need some help.