-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix arch used Apt.check with host_pacakge=False, when cross-compiling #19074
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 arch used Apt.check with host_pacakge=False, when cross-compiling #19074
Conversation
| if host_package: | ||
| check_arch = self._arch or self._conanfile.settings_build.get_safe('arch') | ||
| else: | ||
| check_arch = self._conanfile.settings_build.get_safe('arch') |
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.
Hi @sveinuah,
Thanks a lot for the contribution. I think we could simplify the code to this?
| if host_package: | |
| check_arch = self._arch or self._conanfile.settings_build.get_safe('arch') | |
| else: | |
| check_arch = self._conanfile.settings_build.get_safe('arch') | |
| check_arch = self._arch if host_package else self._conanfile.settings_build.get_safe('arch') |
Because when host_package=True, we should use the host architecture (self._arch) and not fallback to settings_build.
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.
That was my original thought as well, but the test_tools_check tests failed when I tried. I kept the old implementation in the host_package branch, in case there was a specific reason for it.
test/integration/tools/system/package_manager_test.py:449: AssertionError
____________ test_tools_check_with_version[Apt-dpkg-query -W -f='${Architecture} ${Version}\\n' package | grep -qEx '(amd64|all) 0.1']
...
E assert "dpkg-query -...one|all) 0.1'" == "dpkg-query -...d64|all) 0.1'"
E Skipping 60 identical leading characters in diff, use -v to show
E - ep -qEx '(amd64|all) 0.1'
E ? ^^^^^
E + ep -qEx '(None|all) 0.1'
E ?
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. It may just have been a missing setting in the test. Adding the patch below, makes the tests pass.
Should I add this to the PR, and apply your suggestion?
Adding
diff --git a/test/integration/tools/system/package_manager_test.py b/test/integration/tools/system/package_manager_test.py
index e6b40c51a..ad9360731 100644
--- a/test/integration/tools/system/package_manager_test.py
+++ b/test/integration/tools/system/package_manager_test.py
@@ -439,7 +439,7 @@ def test_tools_install_archless_with_version(tool_class, result):
])
def test_tools_check(tool_class, result):
conanfile = ConanFileMock()
- conanfile.settings = Settings()
+ conanfile.settings = MockSettings({"arch": "x86_64"})
conanfile.conf.define("tools.system.package_manager:tool", tool_class.tool_name)
with mock.patch('conan.ConanFile.context', new_callable=PropertyMock) as context_mock:
context_mock.return_value = "host"
@@ -463,7 +463,7 @@ def test_tools_check(tool_class, result):
])
def test_tools_check_with_version(tool_class, result):
conanfile = ConanFileMock()
- conanfile.settings = Settings()
+ conanfile.settings = MockSettings({"arch": "x86_64"})
conanfile.conf.define("tools.system.package_manager:tool", tool_class.tool_name)
with mock.patch('conan.ConanFile.context', new_callable=PropertyMock) as context_mock:
context_mock.return_value = "host"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.
Yes, I think that the test should have the MockSettings({"arch": "x86_64"} defined, that's the reason the test was failing.
1fe966a to
3dda692
Compare
Apt.check(<pkg>, host_package=False) added the host arch, instead of the build arch to the dpkg query, if used in a recipe during cross-building conan install invocations. This fix ensures that the `settings_build.arch` is used to make the query in this context. Issue: conan-io#19073
3dda692 to
ef8ff91
Compare
The mocked settings was missing an `arch`, causing `_SystemPackageManagerTool._arch` to be unset in the tests, leading to erroneous test failures.
ef8ff91 to
9679012
Compare
|
Updated PR with the |
Changelog: Bugfix: Fixes an issue where the Apt packages for the build arch would be reported missing, in cross-compiling scenarios, even though they are installed.
Docs: Omit
Fixes #19073