Set arch to detected x86_64 microarchitecture if available#2591
Set arch to detected x86_64 microarchitecture if available#2591lleyton wants to merge 2 commits into
Conversation
|
Commenting as the x86_64_v3 tester for this, everything worked as expected running this build and I could install and run x86_64_v3 packages and below, but not any above what my CPU could handle. |
|
Noticed the copr and manifest plugins were failing with my change. I took a look and it seems like they're not using the basearch, even though it seems like that would be the correct behavior when looking at the dnf4 source:
I updated the plugins to use the basearch. I'm not sure why specifically the 43 test is failing, as I'm not able to reproduce the issue locally. Again, I'm not too familiar with the internals of dnf, so please let me know if I'm missing anything. |
Conan-Kudo
left a comment
Conan-Kudo
left a comment
There was a problem hiding this comment.
This looks pretty good to me at a first glance, I'll wait for someone else to also look over it before merging this.
Conan-Kudo
left a comment
Conan-Kudo
left a comment
There was a problem hiding this comment.
This code looks mostly fine, but the change of arch to be redefined to basearch is definitely wrong. There's already a basearch variable for that purpose.
In general, users should not expect subarch packages to be composed out into a separate repository. It's not required for DNF or RPM, as the code for selecting/filtering this should handle that gracefully.
| this->set_value("main", "name_version", name_version); | ||
|
|
||
| std::string arch = base.get_vars()->get_value("arch"); | ||
| std::string arch = base.get_vars()->get_value("basearch"); |
There was a problem hiding this comment.
This doesn't seem right. If something is requesting the actual architecture, it should still get that for this variable.
| arches = std::vector<std::string>(arch_option->get_value().begin(), arch_option->get_value().end()); | ||
| } else { | ||
| arches = {ctx.get_base().get_vars()->get_value("arch")}; | ||
| arches = {ctx.get_base().get_vars()->get_value("basearch")}; |
| auto & base = ctx.get_base(); | ||
|
|
||
| const auto & arch = base.get_vars()->get_value("arch"); | ||
| const auto & arch = base.get_vars()->get_value("basearch"); |
|
|
||
| auto * goal = ctx.get_goal(); | ||
| const auto & arch = base.get_vars()->get_value("arch"); | ||
| const auto & arch = base.get_vars()->get_value("basearch"); |
| arches = std::vector<std::string>(arch_option->get_value().begin(), arch_option->get_value().end()); | ||
| } else { | ||
| arches = {ctx.get_base().get_vars()->get_value("arch")}; | ||
| arches = {ctx.get_base().get_vars()->get_value("basearch")}; |
|
I'm starting to think there's something logically wrong that predates this pull request. We should not be assuming that This assumption used to be reliably disproven when Fedora had 32-bit x86 repositories, as the package architecture was i686, but the repository architecture (basearch) was i386. But none of Fedora's current architectures have this quality yet, so I think we've unwittingly messed up here. |
3 participants
The code for detecting the current microarchitecture is taken from RPM. See the associated comment in the diff. I have tested it, and it seems to allow installation of packages built against a target
x86_64_v*, when the system supports it. Additionally, it seems to prioritize packages that have the same microarch. Everything else seems to work, but please let me know if I'm missing anything, as I'm not too familiar with the internals of dnf5.Closes #1468.