Skip to content
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 check for installed apt packages (#3032) #3033

Merged
merged 2 commits into from Sep 20, 2018

Conversation

Projects
None yet
5 participants
@aharrison24
Copy link
Contributor

commented Jun 12, 2018

Fix for issue (close #3032)

  • Refer to the issue that supports this Pull Request.

  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.

  • I've read the Contributing guide.

  • I've followed the PEP8 style guides for Python code.

  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one. Also adding a description of the changes in the changelog.rst file. https://github.com/conan-io/docs

  • Changelog: Fix: Fixed AptTool at SystemPackageTool to improve the detection of an installed package.

@ghost ghost added the contributor pr label Jun 12, 2018

@CLAassistant

This comment has been minimized.

Copy link

commented Jun 12, 2018

CLA assistant check
All committers have signed the CLA.

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2018

HI @aharrison24
Thanks for this contribution!

It seems that the tests are not passing. One test failure is obvious, it is checking the command generated, and should be easy to fix. The other one, I don't know without running. Please check them.

Also, it would be necessary to check the CLA, for some reason it is not ok. Typically this happen when the commits are done with an email different to the github one. It can be fixed following the link in the CLAassistant section above.

@aharrison24 aharrison24 force-pushed the aharrison24:feature/fix_systempackagetool branch from b67d93f to 67821a3 Jun 13, 2018

Fix check for installed apt packages
Previous check would return true if a package was installed in
the past, but subsequently removed.

Discussion at:
https://stackoverflow.com/questions/1298066/check-if-a-package-is-installed-and-then-install-it-if-its-not

@aharrison24 aharrison24 force-pushed the aharrison24:feature/fix_systempackagetool branch from 67821a3 to e0c2fb3 Jun 13, 2018

@aharrison24

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2018

@memsharded Sorry for my schoolboy errors! I think everything should be fixed now.

Do you need a changelog PR to be raised now as well? I wasn't sure what version number to put it under. 1.4.5?

@danimtb danimtb added this to the 1.8 milestone Aug 30, 2018

@ghost ghost assigned memsharded Sep 11, 2018

@ghost ghost added stage: review and removed contributor pr labels Sep 11, 2018

@memsharded
Copy link
Contributor

left a comment

I have solved the conflict from latest changes, to prepare this to be merged for next release.

It is a bit annoying that it is necessary such a command for getting this info about installation of a package. I have also had a look, and even most tutorials doesn't explain this behavior. I hope this doesn't break, for example, if the tools are localized, and the output message is not "ok installed".

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

Sorry for the delay in handling this, btw, I missed the notification about the update for the broken tests.

@memsharded memsharded assigned lasote and unassigned memsharded Sep 11, 2018

@aharrison24

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

Yes, it does feel fragile to look for the "ok installed" message in the output. I've searched around a bit, but haven't been able to find a more robust solution. All of the options seem to rely on grepping for English words like "install".

It might be safer to stick with the existing solution rather than merging this PR. At least the existing solution does the right thing in most cases, and behaves identically under different locales. The solution proposed in this PR could break entirely on systems that don't output that "ok installed" message in English.

@@ -135,7 +135,7 @@ def install(self, package_name):
_run(self._runner, "%sapt-get install -y %s%s" % (self._sudo_str, recommends_str, package_name))

def installed(self, package_name):
exit_code = self._runner("dpkg -s %s" % package_name, None)
exit_code = self._runner("dpkg-query -W -f='${Status}' %s | grep -q \"ok installed\"" % package_name, None)

This comment has been minimized.

Copy link
@lasote

lasote Sep 12, 2018

Contributor

What is this ${Status} about?

This comment has been minimized.

Copy link
@aharrison24

aharrison24 Sep 14, 2018

Author Contributor

It's part of a format string which is passed to dpkg-query.

-f is the short form of the --showformat command:
http://manpages.ubuntu.com/manpages/cosmic/man1/dpkg-query.1.html

This comment has been minimized.

Copy link
@lasote

lasote Sep 17, 2018

Contributor

Probably I'm dumb but where is the string passed to format the ${Status}

This comment has been minimized.

Copy link
@aharrison24

aharrison24 Sep 17, 2018

Author Contributor

There's no string substitution or formatting happening in python. The '${Status}' string gets passed directly to the dpkg-query command. At the command line you would use something like:

dpkg-query -W -f='${Status}' libudev-dev | grep -q "ok installed"

This comment has been minimized.

Copy link
@aharrison24

aharrison24 Sep 17, 2018

Author Contributor

I'm wondering whether a more portable test would be something like:

dpkg -l <package-name> 2>/dev/null | grep -q ^ii

This comment has been minimized.

Copy link
@lasote

lasote Sep 20, 2018

Contributor

Sorry for the confusion, I was trying to run it in a docker container but wrote up the command incorrectly. I think dpkg-query -W -f='${Status}' libudev-dev | grep -q "ok installed" is working well.

This comment has been minimized.

Copy link
@lasote

lasote Sep 20, 2018

Contributor

Note: I confirmed that in a linux with different locale the message is still the same:
French:

root@c018a9e1ca3c:/# dpkg-query 
dpkg-query : erreur : requiert une option d'action

Utiliser --help pour de l'aide sur la recherche de paquets.
root@c018a9e1ca3c:/# dpkg-query -W -f='${Status}' zlib1g 
install ok installedroot@c018a9e1ca3c:/# 

Russian:

root@d947354f90b4:/# dpkg-query 
dpkg-query: ошибка: укажите требуемое действие

Используйте параметр --help для вывода справки по запросам пакетов.
root@d947354f90b4:/# dpkg-query -W -f='${Status}' zlib1g 
install ok installedroot@d947354f90b4:/# 

@lasote lasote merged commit 42767e9 into conan-io:develop Sep 20, 2018

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Sep 20, 2018

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Fix check for installed apt packages (conan-io#3033)
Previous check would return true if a package was installed in
the past, but subsequently removed.

Discussion at:
https://stackoverflow.com/questions/1298066/check-if-a-package-is-installed-and-then-install-it-if-its-not
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.