Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Apt install command to install all packages at once #953

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SubaruArai
Copy link
Contributor

See: #940

This should make the installation faster.
If this is successful, we might need to think about other package managers as well.

This should make the installation faster.
@SubaruArai SubaruArai marked this pull request as draft April 16, 2024 05:12
@cottsay
Copy link
Member

cottsay commented Apr 16, 2024

The original reason that rosdep behaves in this way is to ensure that packages will still get installed on a best-effort basis despite an issue with one of them.

If the install operation attempts to install 101 packages and one of them doesn't actually exist, the other 100 packages should still be installed prior to exiting. Some package managers (like dnf) support this behavior, so rosdep invokes the installation in a single command. At the time when apt support was implemented, there didn't appear to be a way to achieve that behavior.

@SubaruArai
Copy link
Contributor Author

The original reason that rosdep behaves in this way is to ensure that packages will still get installed on a best-effort basis despite an issue with one of them.
...
At the time when apt support was implemented, there didn't appear to be a way to achieve that behavior.

Interesting, thanks for the input!
Although I'm not really sure if I like this decision, I can get it.
(I personally prefer all or nothing (atomic) - which isn't achievable since we're using multiple package managers)

Anyways, we should be able to add such a feature with something like this.

I guess the real question will be - do we want to preserve this behavior, or speed-up the installation instead (especially in CI/CD systems)?
According to @tonynajjar, a 24 apt package + 1 pip package installation was reduced from 58s to 28s - which I think is a big deal in the context of CI systems. I also prefer failing fast (it looks like apt-get fails at the lookup stage) rather than figuring out one package went wrong after a while.

@cottsay What do you say? Keep the best-effort behavior, or ditch it for faster execution?

@tonynajjar
Copy link

tonynajjar commented Apr 17, 2024

I would really like to keep both features if possible. In the package.xml I also define dependencies on custom packages that are not released, for the purpose of colcon building. So when running rosdep, it fails to fetch these custom packages but still continues with the rest.
As for faster rosdep, it's obvious why we would want that - imagine all the CI time you would save in total across the world 😬

@tonynajjar
Copy link

@cottsay did you have the chance to think about it?

@cottsay
Copy link
Member

cottsay commented May 17, 2024

Keep the best-effort behavior, or ditch it for faster execution?

I do not believe that changing the existing best-effort behavior is a good idea, and I can't see myself approving a PR that changes that only to improve performance.

A change to a single-invocation that preserves the established behavior and keeps apt aligned with the other common package managers supported by rosdep would certainly get my attention.

@tonynajjar
Copy link

There is already the flag -r Continue installing despite errors, can't we apply the "one-shot" approach only if this flag is on?

@cottsay
Copy link
Member

cottsay commented May 17, 2024

FWIW, that would still change behavior that was established over a decade ago. I don't feel particularly strong one way or the other - maybe you might be able to convince other maintainers though 😅

Have either of you looked at how we might get apt to do what we want? 30 seconds of googling led me to a --ignore-missing flag that might help, though I haven't tried it.

To be clear, rosdep won't (shouldn't) try to install packages that are already installed. The established behavior mostly revolves around incorrect rules. It's pretty common that we'll have a ubuntu: [foo] rule where foo was available in older Ubuntu versions and then isn't available in a newer one. This behaves differently from an explicitly unavailable rule, where -r allows you to continue the rosdep operation despite lacking resolution for one or more keys.

@SubaruArai
Copy link
Contributor Author

It's been a while and I can't recall what I tried, but I remember that there wasn't a silver bullet - including the --ignore-missing flag.

Copy link

@130s 130s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the code. The current state of this PR would change rosdep's behavior as already discussed in the review comments section. I +1 for not changing the default behavior, but also personally +1 for adding an optional feature to faster installation.

I'm not a maintainer.

@tfoote tfoote mentioned this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants