-
Notifications
You must be signed in to change notification settings - Fork 99
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
Fix: let copr plugin to respect the installroot option #2081
base: main
Are you sure you want to change the base?
Fix: let copr plugin to respect the installroot option #2081
Conversation
@@ -37,11 +38,14 @@ namespace dnf5 { | |||
|
|||
std::filesystem::path copr_repo_directory() { | |||
std::filesystem::path result; | |||
libdnf5::Base base; |
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.
This line creates a new instance of the Base
class with the default settings.
We need to use the base
object from the current dnf5 application context. It is obtained in multiple places using the line auto & base = get_context().get_base();
. So we need to pass it here.
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.
ah yes I was testing it yesterday whether it works and the installroot was for some reason empty... that explains it, thanks! however when I try to compile this, I get this error:
/builddir/build/BUILD/dnf5-5.2.10.0-build/dnf5-5.2.10.0/dnf5-plugins/copr_plugin/copr_repo.cpp:42:19: error: ‘get_context’ was not declared in this scope; did you mean ‘gettext’?
42 | auto & base = get_context().get_base();
| ^~~~~~~~~~~
| gettext
[ 77%] Building CXX object dnf5/CMakeFiles/dnf5.dir/commands/autoremove/autoremove.cpp.o
I have no idea why - this is the first time I've got my hands dirty with cpp so I suppose it's something simple like missing some definition in .hpp file? Or do I need to pass it from somewhere as function argument?
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.
@nikromen
get_context
is a method of class Command
. It can be called from instances of the Command
class and derived classes. The copr_repo_directory
is not a class method. Therefore, get_context
must be used in the caller (class method) and then base
is passed to copr_repo_directory
as an argument.
Example solution:
- Add the base argument:
std::filesystem::path copr_repo_directory(libdnf5::Base & base)
. - In the methods of the classes derived from
Command
we will callcopr_repo_directory(get_context().get_base())
. - In the methods of the
CoprRepo
class, we will callcopr_repo_directory(*base);
- not derived from theCommand
class, but containing thebase
item.
df8515c
to
8972179
Compare
8972179
to
a04240c
Compare
This creates repofiles in
installroot
to respect its location. Howeverdnf4 copr
behaves the same way asdnf5 copr
with--use-host-config
:the repofile is created inside
/etc/yum.repos.d
, the installroot is ignored.But
dnf4 install buildtag --installroot=/tmp/dnf4
somehow uses the repofile in/etc/yum.repos.d
. The same steps with dnf5 will result inbecause dnf5 by default looks to the installroot. I don't know whether we want to change the default behaviour of
dnf copr
between versions 4 and 5, but I'd expect thednf5 copr
command to create repofile insideinstallroot
, so this PR should address this issue.Fix #1497