Install a list of system packages

[uilianries]

Support a list of package to be installed by SystemPackageTools. The current behavior only supports a single package, or its variants by a list. However, most system package managers accept a list of different names e.g. apt install vim nano emacs

Changelog: Feature: System package tools can install a list of different packages.
Docs: conan-io/docs#1937
fixes #6961
fixes #6960

• 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.

^{Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.}

[uilianries]

Hey, why pr-merge didn't restart?

[danimtb]

Probably an issue with the Ci regarding the log. cc/ @czoido

[memsharded]

The problem with this is that it implements the functionality correctly, but it would probably not solve the underlying issue. I expect that many users will still do the install(["nano vim emacs"]), and will not read the documentation deeply enough to realize there is an opt-in all=True argument they need to pass if they want all packages installed.

The only way to achieve this is breaking the contract. We could do it gradually, and propose something like a new install_packages method, that will try to install all of them, and if some want to propose variants, it could do in a nested tuple or list. Or any other new interface, while we deprecate the old one.

I am approving, but will ask for further reviews.

The SystemPackageTool IMHO is a monster which probably needs a whole refactor.

[jgsogo]

The extra line in the docs is quite unfortunate so we cannot change without breaking

besides that, the examples in the docs are not taking advantage of that feature, and the PR in CCI is not using it either:

😞 😞 😞

I agree it is more expected to install all the packages from the list than to return as soon as one of them is installed. I'm trying to think about a different implementation, but I cannot figure out how not to break existing behavior (yes, I'm not a big fan of adding extra arguments).

If we were 100% sure we will have SystemPackageTools in Conan v2.0, I would propose a different approach:

def system_requirements(self):
   spt = SystemPackageTools(v2_behavior=True)  # CONAN_V2_MODE activates it by default
   spt.install('pkg_a')  # <-- installs the only one package

   # Breaking change activated by the `v2_behavior=True`)
   spt.install(['pkg_a', 'pkg_b', 'pkg_c'])  # <-- installs three different packages (this would be the new CONAN_V2 behavior)

   # Using a list of lists: If a list of alternatives is provided, Conan installs only one from each group
   spt.install([('pkg_a-alt1', 'pkg_a-alt2'), 'pkg_b', 'pkg_c' ])  # <-- installs one package from each group
   spt.install([('pkg_a-alt1', 'pkg_a-alt2'), ])  # <-- installs one package from each group

IMHO it is more natural: every entry in the list installs one package, if alternatives are given for entry only one of them is installed.

---

I'm not asking for these changes, just proposing the alternative and, if we agree with it, then we can change the PR.

[memsharded]

Yes, I also like the syntax:

spt.install([('pkg_a-alt1', 'pkg_a-alt2'), 'pkg_b', 'pkg_c' ])

But I wouldn't like to have the flag explicit here: SystemPackageTools(v2_behavior=True), as this will be very ugly in Conan 2.0. In any case, I would say a syntax that could remain with a new method, something like:

spt = SystemPackageTools()
spt.install_pkgs(("alt1", "alt2", "alt3"), "pkg2", "pkg3")  # or install_system...

There is a chance if we replace the whole SystemPackageTools with a new alternative when we do the "tools" general refactor, and we take advantage of the change, but that would be just another extra effort, not sure it is worth.

[jgsogo]

Adding an explicit flag or adding a dedicated method is more or less the same (IMHO it's more explicit the flag than the method, unless we find a name other than install that we want for that method in the future). Both the flag and the new method require to modify recipes now (if needed) and both alternatives can be marked as deprecated in Conan v2 and move the functionality to the preferred implementation (remove the flag, or back to install method).

Whatever is used it is ok for me, I'll review again when the PR is updated.

[memsharded]

Adding an explicit flag or adding a dedicated method is more or less the same (IMHO it's more explicit the flag than the method, unless we find a name other than install that we want for that method in the future). Both the flag and the new method require to modify recipes now (if needed) and both alternatives can be marked as deprecated in Conan v2 and move the functionality to the preferred implementation (remove the flag, or back to install method).

I was thinking of a new method name that can remain forever, so it is only necessary to change it once, before Conan 2.0, but no necessary to deprecate again and move back after 2.0.

[jgsogo]

SystemPackageTool already has some kind of cache and now we need to name a new method... 😄

Yes, totally, if that is the method we will use forever, much better.

[uilianries]

I vote for the new method name. Why? When reading the documentation, method are empathized, using bold color, but for parameters you have to read to find any detail, so anyone who reads the docs, will find the new method name easily. For Conan 2.0, we can break and use only one method, the tuple for variants seems be more correct.

[jgsogo]

If we go for a new method, I prefer the suggested install_pkgs over install_all. Taking into account that the method will last forever and it will be the only method in Conan 2.0 I feel like the suffix _all is a bit unexpected. wdyt?

[uilianries]

I agree, but I prefer the full name install_packages, no abbreviations

[memsharded]

The all argument is not used anymore?

[lasote]

Agree with the previous comments, add a new test for the (2).

[uilianries]

This new commit is able to support (1) (2) (3) options:

spt.install_packages("pkg-variant1") # (1) String input
spt.install_packages(["pkg-variant1", "otherpkg", "thirdpkg"]) # (2) List of strings: all must be installed
spt.install_packages([("pkg-variant1", "pkg-variant2"), "otherpkg", "thirdpkg"]) # (3) List with alternatives

I still use a different when installing multiple packages, because it is faster install all packages at once.

[jgsogo]

Do we want to consider here a list too?

package_install([['pkg1-var1', 'pkg1-var2'], 'pkg2'])

[uilianries]

That's a good question. I think only tuple keep more explicit, but on the other hand, people need to read to documentation to understand that only tuple is allowed. I think accepting list should not be a problem, I'll update it.

[jgsogo]

If we are accepting the syntax install_packages("pkg1 pkg2"), please add it to the docstring.

But, IMHO, we should avoid that syntax because it can be confusing: what does it mean?

• will it install pkg1 and pkg2 or just one of them?
• what if I use install_packages(["pkg1 pkg2", "pkg3 pkg4"])?

[uilianries]

If we are accepting the syntax install_packages("pkg1 pkg2"), please add it to the docstring.

Sure, indeed the docstring is outdated since the last commit.

what if I use install_packages(["pkg1 pkg2", "pkg3 pkg4"])?

I would like to avoid it, but is totally valid for the current behavior, it will install all packages.

[jgsogo]

All packages? pkg1, pkg2, pkg3 and pkg4? It doesn't match the expectations when you use a list of tuples...

[uilianries]

install_packages(["pkg1 pkg2", "pkg3 pkg4"]) will install all packages pkg1, pkg2, pkg3 and pkg4, yes. Variants should be a nested list or a tuple in a list, like:

install_packages([["pkg1 pkg2"], "pkg3 pkg4"])

[jgsogo]

IMHO, it is very confusing if install_packages(["pkg1 pkg2", "pkg3 pkg4"]) installs all packages, but install_packages([("pkg1", "pkg2"), ("pkg3", "pkg4")]) install only some of them. I'd totally forbid (raise) the syntax with blank-separated packages (pkg1 pkg2).

...but this is my point of view, what do other reviewers think about it? cc/ @memsharded

[memsharded]

I agree with @jgsogo

Space separated list introduce another way to do the same thing, but the semantics are now not clear and difficult to dissambiguate. I think it is better to have the following logic:

• Each argument could be either a string, or a list/tuple.
• If a string, that is a package name to install
• If a list/tuple, that is a set of alternatives, install the first successful one.

[uilianries]

If a string, that is a package name to install

Considering the previous case, it's a string and contains more than one package. It's totally valid for install(), it will install a variant, but only because install() only installs variants, not all packages. I'll update to set is as invalid, but it won't follow the same design as did for install().

[uilianries]

Done, now ["foo bar"] and "foo bar" are considered invalid. Actually doesn't matter if you pass a string or list, it's convert to a list at the first step.

[uilianries]

New change: install_packages installs only what is not installed on the system, for example, if pass ["vim", "wget", "curl"], but "wget" is already installed, it will install only "vim" and "curl". However, if force is True, it will install all packages listed, including "wget" again.

Why? It's the natural behavior when installing packages when we don't force. Also, it's faster, as the system don't need to process and print "package xyz already installed".

[jgsogo]

Looking good. Let's finish this feature and then we can refactor this class a little bit

[uilianries]

One approval! Maybe it will be merged before US election ending!