| sitter |
We were expecting to find a Release file, but package archives have
moved from Release files to InRelease files, which have inlined GPG
signature.
Moreover, we try to get the release date from distro-info/ubuntu.csv and prefer it over the [In]Release file.
We get the release Id and Codename from /etc/os-release, which is more modern than lsb-release.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
This might look hacky, skipping two lines, but the alternative would be:
// read the relase file - FileFd fd(releaseFile.toStdString(), FileFd::ReadOnly); + // This is what apt-pkg does in debReleaseIndex::Load(), but we are + // interested only in the Date field here. + FileFd fd; + if (!OpenMaybeClearSignedFile(releaseFile.toStdString(), fd)) { + return QDateTime(); + } pkgTagFile tag(&fd);
or using apt-pkg debReleaseIndex::Load(), which does the same.
It works, but now we are not just reading a file. OpenMaybeClearSignedFile is creating a temporary copy of the file InRelease with stripped signature.
Moreover, we are reading it for every package (e.g. in Muon when showing the description of a distro supported package), although releaseDate applies to the whole distribution.
I vote for skipping lines.
Canonical provides critical updates for acct until Donnerstag, 27. April 2023.
This patch brings back the date, which was missing.
Good catch.
If I was you I'd not skip lines though :P
What I would do is refactor this. The problem is that we parse the same file N times, when it really doesn't need to be. One and the same release file would not change between package A and B, so it really only should be read once and conceivably held in a static in-memory cache until the qapt::cache changes (or reloads) (or perhaps a qtimer runs out to throw it out of cache). Then one could use the slightly more expensive, readable and reliable code involving OpenMaybeClearSignedFile instead of manually mangling InRelease to drop the signature.
Overall I think refactoring is the way to go here. The current code offers bad performance whether you use OpenMaybeClearSignedFile or not. For every package loaded we need to build the uri, then stat() the file to see if it exists, then if it exists we need to open it and read a fair amount of bytes before we get to the field we actually care about. It's a waste of resources really.
That said, I am fine with skipping lines.
But, the current code breaks the release file lookup for repos which only have _Release, so that needs fixing at the very least.
| src/package.cpp | ||
|---|---|---|
| 120–121 | I don't think this is right. A repository may still have a Release file. It may *only* have a Release file and be valid still. The repo metadata generally is fully backwards compatible to the 90's So, the local cache may be either _Release or _InRelease. And there probably needs to be a FileExists check in here to determine which to use. | |
| 576 | As a side effect of my remark on getReleaseFileForOrigin, this likely needs to be conditional to releaseFile.endsWith("_InRelease") as the code needs to be able to parse Release and InRelease files. Also, I kinda feel like like the data needs more editing. InRelease has PGP header followed by the content, followed by a PGP signature block at the end. The current code only strips the header and then assumes that pkgTagFile() will still correctly handle a (technically) not entirely valid InRelease file (it now has no header but still a signature block). That seems fairly naughty. This may work now, we cannot really assume that it will still do so in a year though. We could ignore this if you want to work on refactoring the file reading, once refactored the skipping would go away, so long-term reliability of this is less of a concern. | |
Keep the release date cached in the backend
Also, try to get the release date from a csv file like ubuntu-support-status
does. If we don't get it, we fallback to the [In]Release file from the Apt
archive like we did before.
Although we get the distribution Id from lsb-release instead of hard coding
it, this is still an Ubutnu-only feature. The Supported: field in package
listings is non-standard. Moreover, https://wiki.ubuntu.com/SecurityTeam/FAQ
says the field "may be inaccurate".
Requires Qt 5.8 for QDateTime::fromSecsSinceEpoch().
| src/backend.cpp | ||
|---|---|---|
| 402 | We cannot return here on error, otherwise we miss the fallback to InRelease below. Should be split to getReleaseDateFromDistroInfo() and getReleaseDateFromArchive()? | |
| src/backend.cpp | ||
|---|---|---|
| 402 | Oh, that is indeed very true. Splitting sounds like a good idea, also makes it easier to see what's going on when reading the code in the future I guess 👍 | |
The csv parsing could be made more explicitly reliable, but I doubt it makes much difference in the grand scheme of things so I think this is good to land as-is. Personally I'd change the csv parsing though.
| src/backend.cpp | ||
|---|---|---|
| 141 | I kinda think we maybe should read the column location from the file itself. The first line should be the headers. It's not a huge deal as I expect the format has not ever changed and never will ^^ So something like line = info_stream.readLine();
split = line.split(QLatin1Char(','));
const int codenameColumn = split.indexOf("codename");
Q_ASSERT(codenameColumn != -1);
const int releaseColumn = split.indexOf("release");
Q_ASSERT(releaseColumn != -1);
do {
line = info_stream.readLine();
QStringList split = line.split(QLatin1Char(','));
if (split.at(codenameColumn) == releaseCodename) { | |