package_manager version support added #18517
Conversation
| @@ -25,8 +25,6 @@ def system_requirements(self): | |||
| print("missing:", not_installed) | |||
| """)}) | |||
| client.run("create . --name=test --version=1.0 -s:b arch=armv8 -s:h arch=x86") | |||
| assert "dpkg-query: no packages found matching non-existing1:i386" in client.out | |||
There was a problem hiding this comment.
I've removed these lines because dpkg -l doesn't print anything if it doesn't find any package with the search pattern
| install_command = "{sudo}{tool} install -y {recommends}{packages}" | ||
| update_command = "{sudo}{tool} update" | ||
| check_command = "dpkg-query -W -f='${{Status}}' {package} | grep -q \"ok installed\"" | ||
| check_command = "dpkg -l | grep -q -E \"^ii\\s+\\b{package}\\s+\\b\"" |
There was a problem hiding this comment.
Does the change to check_command solve issues of the previous one, or is this change more to align it with the check_version_command?
There was a problem hiding this comment.
This change is necessary to fix a false positive issue we had before. It is also to align with the way of checking the version of a package.
If your package had a default architecture that was implicit and the others were explicit, the old check found the default package substring within an installation for another architecture:
For example: foo and foo:armv8. The old check said that foo was already installed if it found foo:armv8.
There was a problem hiding this comment.
https://man7.org/linux/man-pages/man1/dpkg-query.1.html
the query can be tailored to show the architecture as well -
I'm famously incapable to parse regex - so if we could keep a more readable thing that would be great. if the PR is fixing a pre-existing issue that is not aligned with the PR objective, it helps the review to mention it in the description
There was a problem hiding this comment.
Checking this again, I'm not sure I understand this change. If we are passing always the architecture now, can't we keep the check_command as it was with dpkg-query?
| arch_name=arch_name, | ||
| version="", | ||
| version_separator="") | ||
| if version and self.check_version_command: |
There was a problem hiding this comment.
Maybe we should we output a warning message when we are specifying versions but the package manager does not support that?
Co-authored-by: James <[email protected]>
Co-authored-by: Carlos Zoido <[email protected]>
| @pytest.mark.parametrize("tool_class, result", [ | ||
| (Apk, 'apk info -e package'), | ||
| (Apt, 'dpkg-query -W -f=\'${Status}\' package | grep -q "ok installed"'), | ||
| (Apt, 'dpkg -l | grep -q -E "^ii\\s+\\bpackage\\s+\\b"'), |
There was a problem hiding this comment.
Curiosity, but what is the reasoning behind this change? a regular expression is a tad harder to parse -
the code as it was was added to address an issue
#3033
#3032
just want to make sure we don't introduce regressions
also note that you can do for example dpkg-query -W -f='${version}' libudev-dev to get the version of a package
it can be combined with other things like ${Status}
changes are dpkg -l is fine, but may need to consider whether it causes performance issues due to repeated calls on systems that have many packages installed (dpkg -l gives you the list of all packages, dpkg-query can be tailored to output info of a specific package
There was a problem hiding this comment.
This change is to verify that the arch of the installed package is correct. for example:
If you want the foo packge and you have foo:arm64 installed, searching for foo using the current option find it as installed. With the second option, when checking that the name is strictly foo and has nothing else in front of the name, it doesn't find the wrong package and installs it.
| install_command = "{sudo}{tool} install -y {recommends}{packages}" | ||
| update_command = "{sudo}{tool} update" | ||
| check_command = "dpkg-query -W -f='${{Status}}' {package} | grep -q \"ok installed\"" | ||
| check_command = "dpkg -l | grep -q -E \"^ii\\s+\\b{package}\\s+\\b\"" | ||
| check_version_command = "dpkg -l | grep -E \"^ii\\s+\\b{package}\\s+\\b\" | grep -q \"{version}\"" |
There was a problem hiding this comment.
Isn't this doable with something like this?
check_version_command = "dpkg-query -W -f='${Version}\n' {package} | grep -qx '{version}'"
There was a problem hiding this comment.
Here we will have the same problem I mentioned above, there will be no discrimination based on architecture, so we will have false positives. (issues/18569)
Changelog: Feature: Added support to the system_package tool for defining the system package version to be installed
Changelog: Fix: Fixed an issue that caused APT packages without a defined architecture to be detected if one with the same name was installed for a different architecture.
Docs: conan-io/docs#4170
Close #17744
Close #18569
developbranch, documenting this one.