gazebo-tooling / action-gz-ci

MIT License
5 stars 4 forks source link

Support apt dependencies in a file #15

Closed chapulina closed 4 years ago

chapulina commented 4 years ago

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.

mjcarroll commented 4 years ago

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 commented 4 years ago

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 :heavy_check_mark: find_package :heavy_check_mark: find_package + others
release-tools :heavy_check_mark: :x: Jenkins would use :point_up:
homebrew :heavy_check_mark: :heavy_check_mark:
gazebodistro :heavy_check_mark: :heavy_check_mark:
release :heavy_check_mark: :heavy_check_mark:
j-rivero commented 4 years ago

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 commented 4 years ago

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.

chapulina commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

chapulina commented 4 years ago

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

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

j-rivero commented 4 years ago

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

wow, good solution