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

Support apt dependencies in a file #15

Merged
merged 14 commits into from
Jul 29, 2020
Merged

Support apt dependencies in a file #15

merged 14 commits into from
Jul 29, 2020

Conversation

chapulina
Copy link
Contributor

Move apt dependencies to a file. The advantage is that the dependencies can be declared once, and multiple actions / CIs can make use of it.

We had also considered keeping all apt dependencies in a central place. This is still up for discussion. Personally, I think that the closer it is to the code, the easier it is to maintain. The change of a dependency will often happen together with a change to find_package on the source code.

This keeps support for the old way of specifying dependencies so we can migrate to the new style without breaking many things on the way.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Copy link
Contributor

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

Personally, I think that the closer it is to the code, the easier it is to maintain. The change of a dependency will often happen together with a change to find_package on the source code.

We still need to get a consensous on this one, in the opposite side of the balance you have: how many branches and repositories do you have to change when a new base dependency needs to be included or change its name?

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina mentioned this pull request Jul 21, 2020
@chapulina chapulina changed the base branch from chapulina/groups to master July 21, 2020 22:04
@mjcarroll
Copy link
Contributor

We still need to get a consensous on this one, in the opposite side of the balance you have: how many branches and repositories do you have to change when a new base dependency needs to be included or change its name?

I would think that you would really only be introducing a new dependency or changing a dependency in a single version of the software. A good recent example would be changing sdformat10 to use tinyxml2. In the in-repo case, it would really only have to be updated in the effected branch (or the effected version branch + master, depending on branching strategy)

I'm less familiar with the case that a base dependency changes in an already-released distro, that's probably something that you've had to deal with, though. How frequently does that happen?

@chapulina
Copy link
Contributor Author

how many branches and repositories do you have to change when a new base dependency needs to be included or change its name?

I believe that if we have a central location vs per-package locations, with a central location you always have 1 extra repository to update. Keep in mind that the source repository always needs to be updated anyway.

The advantage of the per-package location is more evident if we also update Jenkins CI to use packages.apt (debs) and dependencies.yaml (source) from the source repo.

Let's say we want to bump ign-gui4 to use ign-rendering4 instead of ign-rendering3. I think these repos need to be modified:

Repo Central (current) Per-package (proposal)
source ✔️ find_package ✔️ find_package + others
release-tools ✔️ ❌ Jenkins would use ☝️
homebrew ✔️ ✔️
gazebodistro ✔️ ✔️
release ✔️ ✔️

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@j-rivero
Copy link
Contributor

I believe that if we have a central location vs per-package locations, with a central location you always have 1 extra repository to update. Keep in mind that the source repository always needs to be updated anyway.

I think we can find use cases where the source repository does not need to be updated. I.e: we lack in all packages support for generating docs and we want to add doxygen to all CI build to enable doc testing.

I'm less familiar with the case that a base dependency changes in an already-released distro, that's probably something that you've had to deal with, though. How frequently does that happen?

Don't have any data to make a consistent answer but I would say that are not very frequent.

Another possible problems of removing release-tools/dependencies_archive.bash is that it has conditionals on other variables, not only in the major branch: there are conditionals for Ubuntu/Debian release names, conditionals in certain platforms, how are they going to work if we migrate to a flat file with package names?

@chapulina
Copy link
Contributor Author

we lack in all packages support for generating docs and we want to add doxygen to all CI build to enable doc testing.

Ok, I hadn't considered this case. I think these could be added here directly to entrypoint.sh, like we're adding cppcheck & others. On Jenkins it could be equally hardcoded.

there are conditionals for Ubuntu/Debian release names, conditionals in certain platforms, how are they going to work if we migrate to a flat file with package names?

Good point, maybe we'll need to version packages.apt for different platforms. When there's no difference, they could just symlink to each other.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

how are they going to work if we migrate to a flat file with package names?

Renamed the file to packages-bionic.apt so we can differentiate the versions: 2c851a1 . Once we support focal, we can create a packages-focal.apt symlink to that. What do you think, @j-rivero ?

Used on these PRs:

@j-rivero
Copy link
Contributor

Renamed the file to packages-bionic.apt so we can differentiate the versions: 2c851a1 . Once we support focal, we can create a packages-focal.apt symlink to that. What do you think, @j-rivero ?

I think that it can solve the main problems of flexibility based on distribution name.

The only problem I can see is that we have no automatic (no action require) path to migration to new distributions if the package set if the same, the symlink must be created. How about looking for a non versioned file with dependencies (like before the last commit) and fallback to use the new distribution filenames if the first one does not exist? This way we can keep ignition packages using the same file unless needed and don't add an extra action unless required (I learnt how convenient this is while maintaining -release repositories that always require new symlinks).

@chapulina
Copy link
Contributor Author

How about looking for a non versioned file with dependencies (like before the last commit) and fallback to use the new distribution filenames if the first one does not exist?

Ok, how about always installing all packages.apt AND packages-<version>.apt?

I think this would make it easier to traverse all dependencies, as we're doing now.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

how about always installing all packages.apt AND packages-.apt?

Done in 1ddf4a6, let me know what you think.

@j-rivero
Copy link
Contributor

Ok, how about always installing all packages.apt AND packages-<version>.apt?

wow, good solution

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.

3 participants