modules: Report switching module streams #1285
Conversation
pkratoch
force-pushed
the
report-switch-stream
branch
from
1449173 to
64b8252
Compare
| transaction.p_impl->problems = ret; | ||
| return transaction; |
There was a problem hiding this comment.
Is there some reason to return here?
It seems that the approach in Goal::resolve() is rather to gather all the errors and report them together.
There was a problem hiding this comment.
Yes, if it is not possible to switch module streams, but the operation requires it, it's not possible to resolve modules, therefore we don't know what modules are active and what rpms are used, so it doesn't make sense to resolve the rest of the goal.
The same case is when the modules can't be resolved for other reasons, but it's mostly covered in #1277 .
This is consistent with DNF4 behavior.
There was a problem hiding this comment.
I see, could you add a comment with that explanation please?
| for (auto & name_streams : module_db->get_all_newly_switched_streams()) { | ||
| TransactionModule tsmodule( | ||
| name_streams.first, | ||
| name_streams.second.first + " -> " + name_streams.second.second, |
There was a problem hiding this comment.
I would prefer if this formatting was done in libdnf-cli because this way other users of the library might have to parse the value to split it again (for example some graphical front-end).
However storing it in a separate field would require breaking ABI in TransactionModule so we would have to move it to the dnf-5.2.0.0 branch. What do you think?
There was a problem hiding this comment.
Sure, I can change the TransactionModule and move it to the dnf-5.2.0.0.
There was a problem hiding this comment.
Please also think that modules might obsolete each other. Therefore module switch might also obsolete something else like RPMs.
| } | ||
| ret |= GoalProblem::MODULE_CANNOT_SWITH_STREAMS; | ||
| transaction.p_impl->problems = ret; | ||
| return transaction; |
There was a problem hiding this comment.
There might be a problem because if the operation was called with requesting of a debugsolver data they will be not created.
There was a problem hiding this comment.
May I ask why you prefer to not continue and collect all problems?
There was a problem hiding this comment.
The writing of debugsolver data from module solver was not implemented and not called anywhere yet, so this PR doesn't affect that. I believe that this needs to be separate for modules and that it doesn't make sense to continue with resolving the rpm goal if there is a fatal error when resolving modules. But I might be missing something.
There was a problem hiding this comment.
I think it would be good to create an issue for the debugsolver data for modules.
There was a problem hiding this comment.
I was thinking about RPM debugsolve code bellow. I completely forget about that modular debugsolv data are not created
There was a problem hiding this comment.
DNF tries to report all problems at once. If I am not mistaken the problem will suggest to allow module switching by configuration option modification. Think that user will enable module stream switching for the next run and then it will receive another set problems with RPMs.
The second user is API user - even when DNF5 goal.resolve() returns some error then API user can still perform the transaction. But if we will skip some step then the transaction will be incomplete.
Think that user even don't want to run a transaction - they only want to see what happened - what request requires and so on - then it would be good to provide maximum results if it is possible. Switching stream is bad thing, but not a dead end like solver dependency error.
There was a problem hiding this comment.
Ah, ok, I had the assumption that switching streams is a problem like the solver error, so we should not continue. I'll look into it.
| _("Error: It is not possible to switch enabled streams of a module unless explicitly enabled via " | ||
| "configuration option module_stream_switch.\nIt is recommended to rather remove all installed " | ||
| "content from the module, and reset the module using 'dnf module reset <module_name>' command. After " | ||
| "you reset the module, you can install the other stream.")); |
There was a problem hiding this comment.
It is recommended to rather remove all installed "
"content from the module, and reset the module using 'dnf module reset <module_name>' command. After "
"you reset the module, you can install the other stream."
What about to remove it. It is a rudiment and after we have module-swich command in DNF4 this approach was obsoleted. Anyway we discovered that this suggestion is even harmful because modules also contains packages from base and they removal would result in a malfunction system.
There was a problem hiding this comment.
Ah, ok, I didn't know it's not the recommended approach anymore. I'll remove it.
|
Closing, since I opened a new PR against the dnf-5.2.0.0 branch: #1308 |
No description provided.