Fix Armbian release info

[pedrolamas]

Adds small fix for Armbian releases, as armbian_release files are not standard format and normally start with a comment.

(related to fluidd-core/fluidd#1385)

[HorlogeSkynet]

Hi @pedrolamas and thanks for your PR.
Even if a special case is required for Armbian, what about URL-parsing BUILD_REPOSITORY_URL and checking for "armbian" in path instead ?
I guess this would be a better heuristic than checking for a leading comment in such a file (?).

Moreover, if this distro is firstly detected as Debian thanks to os-release, we could add a condition to maybe only run this check on "Debian" systems.

Bye 👋

[pedrolamas]

Even if a special case is required for Armbian, what about URL-parsing BUILD_REPOSITORY_URL and checking for "armbian" in path instead ?

Granted, that probably would also work, but there is currently nothing parsing BUILD_REPOSITORY_URL so I avoided added extra complexity for such a specific issue.

I opted for a similar approach to the "cloudlinux" case just below it, where it is checking the "name" - in this case, I am checking the "id" (the extra check for "#" is just to narrow the case further down, the reality is that it is probably not even needed for "armbian" as they always have that comment on the top).

Moreover, if this distro is firstly detected as Debian thanks to os-release, we could add a condition to maybe only run this check on "Debian" systems.

That would be the case if the files weren't ordered alphabetically, and so "armbian-release" will be found before "os-release"

[HorlogeSkynet]

Hello @pedrolamas 👋

I took the liberty of pushing some changes to your branch; Please tell me whether you're okay with them 🙏

[pedrolamas]

Hi @HorlogeSkynet, I just checked and it looks perfectly fine! Thank you! 🙂

[HorlogeSkynet]

Hello @pedrolamas, dear @python-distro/maintainers,

Before merging this, what do you think about adding Armbian version to distro release info ?
I've ended up with the more or less minimal following patch (to add to this very branch) :

diff --git a/src/distro/distro.py b/src/distro/distro.py
index 1e01a68..0db2736 100755
--- a/src/distro/distro.py
+++ b/src/distro/distro.py
@@ -906,6 +906,9 @@ class LinuxDistribution:
         elif self.id() == "debian" or "debian" in self.like().split():
             # On Debian-like, add debian_version file content to candidates list.
             versions.append(self._debian_version)
+            if self._distro_release_info.get("id") == "armbian":
+                # On Armbian, add version from armbian-release file to candidates list.
+                versions.append(self._armbian_version)
         version = ""
         if best:
             # This algorithm uses the last version in priority order that has
@@ -1226,6 +1229,16 @@ class LinuxDistribution:
         except FileNotFoundError:
             return ""
 
+    @cached_property
+    def _armbian_version(self) -> str:
+        try:
+            with open(
+                os.path.join(self.etc_dir, "armbian-release"), encoding="ascii"
+            ) as fp:
+                return self._parse_os_release_content(fp).get("version", "")
+        except FileNotFoundError:
+            return ""
+
     @staticmethod
     def _parse_uname_content(lines: Sequence[str]) -> Dict[str, str]:
         if not lines:
@@ -1309,6 +1322,7 @@ class LinuxDistribution:
                 "name", ""
             ).startswith("#"):
                 distro_info["name"] = "Armbian"
+                distro_info["version"] = self._armbian_version
 
         # CloudLinux < 7: manually enrich info with proper id.
         if "cloudlinux" in distro_info.get("name", "").lower():
diff --git a/tests/test_distro.py b/tests/test_distro.py
index d362d59..4b492f7 100644
--- a/tests/test_distro.py
+++ b/tests/test_distro.py
@@ -1928,7 +1928,7 @@ class TestOverall(DistroTestCase):
             "like": "",
             "version": "10",
             "pretty_version": "10 (buster)",
-            "best_version": "10",
+            "best_version": "23.02.2",
             "major_version": "10",
             "minor_version": "",
         }
@@ -1937,6 +1937,7 @@ class TestOverall(DistroTestCase):
         desired_info = {
             "id": "armbian",
             "name": "Armbian",
+            "version": "23.02.2",
         }
         self._test_release_file_info("armbian-release", desired_info)
 
@@ -2383,6 +2384,12 @@ class TestRepr:
         repr_str = repr(distro._distro)
         assert "LinuxDistribution" in repr_str
         for attr in MODULE_DISTRO.__dict__.keys():
-            if attr in ("root_dir", "etc_dir", "usr_lib_dir", "_debian_version"):
+            if attr in (
+                "root_dir",
+                "etc_dir",
+                "usr_lib_dir",
+                "_debian_version",
+                "_armbian_version",
+            ):
                 continue
             assert f"{attr}=" in repr_str

Calling distro.version(best=True) or distro.distro_release_info["version"] would output the version specified in /etc/armbian-release (as updated tests show).
Simply calling distro.version() will still return 10 (as testing resources are based on Debian Buster in this case), so it would be backward-compatible and still honor Debian os-release file.

Thanks. bye 👋

[pedrolamas]

Thanks @HorlogeSkynet, I've taken a look and looks great, but should that be version_id instead of version?

[HorlogeSkynet]

Thanks @HorlogeSkynet, I've taken a look and looks great, but should that be version_id instead of version?

You're perfectly right ! Updated and pushed. I'd like to see a review from another @python-distro/maintainers before merging this 🙂
Thanks, bye 👋

[HorlogeSkynet]

Dear @python-distro/maintainers, up please 🙏

[HorlogeSkynet]

Up @python-distro/maintainers please 🙏

[nir0s-xpoz]

Well, assuming that we go with this very armbian-specific implementation, I think that it's fine.

My problem with such a distro-specific implementation is really a conceptual one.

When I started writing distro I wanted to find the most common patterns the different distributions used, which kept the implementation extremely clean. Everything revolved around generic files used by different distributions, so that if we found such type of file, we could parse it generically, and then any distribution that used the same pattern would receive the same treatment.

So, on one hand, I'd love it if we didn't specifically address "armbian" in the code, but rather address the TYPE of pattern they use, and then it would automatically understand that it's "armbian" that we're talking about, just like it does when we use os-release.

However (!), nothing in software is pure (it's incredible that the world works at all), so I'm also willing to accept the fact that we need a specific implementation for a specific use case, and if we identify a generic case, we can always change.