manifest: packages processing should match yunohost

[selfhoster1312]

Since #118 was merged yesterday, i believe there is a mismatch between how package_linter and yunohost process the resources.apt.packages list in a string form.

From yunohost:

self.packages = [value.strip() for value in self.packages.split(",")]

In practice it should not change much because apt will gladly eat a space-separated string as multiple package arguments, but i'm not sure if it could affect something else.

[orhtej2]

Current behaviour yields the following:

>>> apt_dependencies = [value.strip() for value in "dep1 dep2".split(",")]
>>> print(apt_dependencies)
['dep1 dep2']

Behaviour revised in #118 yields the following:

>>> apt_dependencies = [value.strip() for value in re.split(" |,","dep1 dep2")]
>>> print(apt_dependencies)
['dep1', 'dep2']

The improper parsing of dependencies as a single item works as expected in AptDependenciesAppResource::provision_or_update because dependencies are joined by ' ' before being passed to ynh_install_extra_app_dependencies which properly parses the list.

[orhtej2]

As per https://github.com/YunoHost/yunohost/blob/51d8608b40e2e2acd76a0ce34ceca9dab0cf13d0/helpers/apt#L227, the list dep1 dep2 gets converted to dep1, dep2 as required by deb control format.

[selfhoster1312]

Thanks for explaining. Which way is the recommended way then? The example app showcases using a comma.

[alexAubin]

When designing packaging v2, I originally chose to stick to a flat list of packages (with comma ignored?), because it's simpler to write "foo bar baz" in everyday life than actual toml lists such as packages = ['foo', 'bar', 'baz'], even though it would be "better" ... but clearly that creates some "eh" feeling between 1) not having a proper list and 2) commas are ignored so it's sort of "not 100% formal" but the intention was to minimize packager frustration :sweat_smile:

Edit: and also the constrain was to be able to inject these in the ynh_install_app_dependencies without having to rewrite the helper which is still used by app v1

[selfhoster1312]

So if the stringy representation is a legacy, maybe the lint should warn when we don't have a proper list? That would make things more consistent and avoid errors. See #120

[alexAubin]

Meh I dunno, it's not really legacy, it's the current practice and I sort of chose to keep the "flat string" syntax to make it "less heavy" on the syntax (in terms of manually writing stuff for not-so-technical people)

We can decide that the new best practice is to use a proper list, but we shouldnt yolo-flag this as a Warning, that's gonna downgrade every app currently flagged level 8 to level 6 ...

I'm not sure what problem exactly we are trying to solve beside having one consistent notation, which surely is a noble goal and nice to have in a perfect world, but there are clearly more important stuff to work on

[alexAubin]

(Another thing waiting in the design decision was that toml auto-format creates that kinds of multi-line list : https://github.com/YunoHost-Apps/endi_ynh/blob/0ea0827fdd5aaf24a789d267bd199b8e8c1a3c5c/manifest.toml#L62 which is "hmpf")

[Salamandar]

(Another thing waiting in the design decision was that toml auto-format creates that kinds of multi-line list : https://github.com/YunoHost-Apps/endi_ynh/blob/0ea0827fdd5aaf24a789d267bd199b8e8c1a3c5c/manifest.toml#L62 which is "hmpf")

Actually I kinda prefer this format as it's alphabetically sortable (but also manually, by kind of dependency for example). Isn't there a way to configure toml auto-format to "not touch" lists ? I know things like clang-format can be configured as such.

All in all, one of the reasons I prefer lists over strings, is that string can be way longer than my editor's width.

We can decide that the new best practice is to use a proper list, but we shouldnt yolo-flag this as a Warning, that's gonna downgrade every app currently flagged level 8 to level 6 ...

Agreed though :D This is just a syntax thing, not a quality indicator.

It can be handled by autopatches, no ?

[alexAubin]

Sure but is it really that big of a deal, we have more important stuff to work on imho ...

[Salamandar]

Ah yeah, fully agree :D

[selfhoster1312]

Personal opinion: it's better to have one canonical way that is easier to lint/support. There's already many ways to do different things and we should focus on having a good supported way to do things, in general.

I understand the need to not break existing packages, so i'm proposing an alternative: how about deprecating string lists for manifest v3?

See c2c9a61fed0420e6e848ca6facdf31c8256d168c in #120

[alexAubin]

Really I can only reinstate that there are a gazillion more important things to do

This specific issue is just the tree hiding the forest : like, despite the fact that there are 2 or 3 supported syntax to declare the list of packages, it is still one of the most standardized things ever and one of the easiest stuff to "lint"

The current packaging format is still, despite all of the v1->v2 improvements, essentially a soup of bash scripts, and the linter heavily relies on brutal greps to look for good/bad practices ... So really wether we use a comma-separated string, or an actual list, is essentially bikesheding, and is like not really a concern compared to "getting rid of as much as the bash soup we can, while not loosing too much flexibility for complex edgecase", and considering we still have a bunch of work / years before the v1 era is still over, and even more to come before v3 is here and v2 is deprecated.

[Salamandar]

I fully agree and on this issue I don’t really care about what other packagers do as long as I can have a list on my packages :D