Fix Broken Logic in bootstrap.py #7088
Conversation
There was a problem hiding this comment.
Nice, this looks much better and makes a lot of sense! I think you just need to fix some lint/formatting errors to make CI happy.
I am wondering if we're better off just doing os.file.exists('/usr/bin/virtualenv') or whatever instead of even bothering to look at the apt-cache...
nathandyer
force-pushed
the
fix-broken-dependency-check
branch
from
abaeb48
to
ab30b3b
Compare
|
Thanks for reviewing @legoktm! I believe I have appeased the linter gods, so this is ready for another look at your convenience. As for your note about possibbly just checking if the virtualenv exists, my gut instinct is that that may not be sufficient to ensure a consistent state, because I can imagine a situation where someone might have a virtual environment created, but have somehow uninstalled one of the dependencies along the way, which just checking for the virtual environment wouldn't catch. |
There were a few areas of broken logic in the is_missing_dependency function, which would result in broken or unidentified behavior if the underlying apt-cache was stale or non-existant. If, while doing a search of the apt-cache, it only located a subset of the package list, or if it were completely unable to find a package (such as in the event of a missing cache), it would still return False, because the grep operation didn't find the "Installed: (none)" string it was looking for. So in those situations, even though the dependencies were missing, it did not return True as it should have. The way the dependencies were listed at the top, with spaces inserted instead of tabs, meant that when we split the string using a space character as the delimiter to form the command, there were dozens of empty strings, which apt-cache would attempt to search for, and would turn up the same error as a missing package, making it impossible to determine when packages were missing. This change eliminates those extra spaces, simplifies the logic to prefer built-in string-checking in Python rather than shelling out to grep, and fixes the broken return logic so it identifies when dependencies are missing due to not being in the apt cache at all.
zenmonkeykstop
force-pushed
the
fix-broken-dependency-check
branch
from
5dc6ac7
to
f41185e
Compare
|
(rebased to pull in a dependency bump and clear static analysis CI job) |
Status
Ready for review
Description of Changes
Fixes #7084
There were a few areas of broken logic in the is_missing_dependency function, which would result in broken or unidentified behavior if the underlying apt-cache was stale or non-existent.
If, while doing a search of the apt-cache, it only located a subset of the package list, or if it were completely unable to find a package (such as in the event of a missing cache), it would still return False, because the grep operation didn't find the "Installed: (none)" string it was looking for. So in those situations, even though the dependencies were missing, it did not return True as it should have.
The way the dependencies were listed at the top, with spaces inserted instead of tabs, meant that when we split the string using a space character as the delimiter to form the command, there were dozens of empty strings, which apt-cache would attempt to search for, and would turn up the same error as a missing package, making it impossible to determine when packages were missing.
This change eliminates those extra spaces, simplifies the logic to prefer built-in string-checking in Python rather than shelling out to grep, and fixes the broken return logic so it identifies when dependencies are missing due to not being in the apt cache at all.
Testing
Note: This test plan requires Tails 5.20
./securedrop-admin setup./securedrop-admin setup./securedrop-admin setup, and confirm that you are no longer prompted for an admin password, and that it sees the dependencies as having already been installed