Run transaction.test() before downloading and installing packages #1458
Conversation
| @@ -363,6 +363,12 @@ void Context::Impl::download_and_run(libdnf5::base::Transaction & transaction) { | |||
| base.get_config().get_tsflags_option().set(libdnf5::Option::Priority::RUNTIME, "test"); | |||
| } | |||
|
|
|||
| if (transaction.test() != libdnf5::base::Transaction::TransactionRunResult::SUCCESS) { | |||
There was a problem hiding this comment.
I think it would be good to perform the test before download, but I don't think it is a good idea to call transaction.test() before RPMs are downloaded, because they are required for the test. The code also does not consider use case with downloadonly option.
There was a problem hiding this comment.
Could you please help me to figure out where exactly the downloaded RPMs are required for the test to run?
It isn't really clear for me unfortunately. I don't see anything related to it in libdnf5::base::Transaction::Impl::_run()
Also the downloadonly option is apparently unaltered because Context::Impl::download_and_run() isn't called for dnf5 download package_name. I'm not that fluent in DNF though... feel free to correct me here.
Should we rather have something like transaction.lock_test() that only check for permissions and don't rely on the whole transaction.test() functionality (which as you said requires RPMs to be downloaded).
P.S.
Rebased on top of main as download_and_run just got some updates.
Also closed/reopened the PR due to miss click
krydos
force-pushed
the
check-permissions-early
branch
from
372aae2
to
5bbc3fe
Compare
This should help to spot the permissions issue early in the process
|
Hi, thanks for your PR. I've reviewed it as I've just started working on issue #849. Based on the discussion there, I'm considering a more robust solution that would take place after parsing the command line but before downloading any metadata. I plan to check if the command and options provided by the user would cause any system state changes (including |
|
@jan-kolarik, It definitely sounds way better than what I have here. Thanks! |
Here it is: #1504. |
|
@krydos Anyway, thanks for your effort. It is greatly appreciated! Looking forward to any future contributions. 🙂 |
|
@jan-kolarik thank you for taking it further! I really miss this dnf5's behavior |
Hi,
I recently faced the issue described in #849 and here on bugzilla about "too late" permissions check while installing/upgrading packages.
I've solved it by calling
Transaction::test()in the beginning ofContext::Impl::download_and_run().It is clearly stated in the
test()definition that it is redundant to calltest()beforerun()but without calling it we should wait for download process to finish.Another possible inconvenience is when
Transaction::test()returns nonTransactionRunResult::SUCCESSI do not customize the error message. Let me know if you think I should check for error types and maybe customize the error message based on that.Please let me know if you think it can/should be implemented in a different way and share your recommendations.
Thanks!