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

Proposal: integrate external libraries as subproject #432

Merged
merged 2 commits into from
Aug 11, 2023

Conversation

engelmi
Copy link
Member

@engelmi engelmi commented Aug 4, 2023

This PR is a based on the question in #430 and a proposal how to solve it.

Instead of inlining external libraries it is to dynamically integrate them to get the latest bugfixes etc.
The basic idea to achieve this is to fetch the external (git) project and integrate it into the hirte project using subproject(). Two mechanisms have been evaluated to fetch the git repos:

  1. mesons wrap dependency system
  2. gits submodules

Mesons wrap system integrates nicely in our current build tooling. However, since the projects get downloaded during meson setup (and is never committed) there are uncommitted changes which will not be used when building the srpm. Git submodules, on the other hand, don't have that issue. On git clone its necessary to fetch those as well, but they are "part" of the respective commit.

subprojects/hashmap.c.wrap Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
@engelmi
Copy link
Member Author

engelmi commented Aug 4, 2023

Pipeline is currently failing due to missing C++ dependencies required for the inih project. If this approach is approved, I'll fix this by either adding the dependencies to the build pipeline or a patch applied by meson on the fly to disable the inihReader (which requires the dependencies).

Copy link
Contributor

@dougsland dougsland left a comment

Choose a reason for hiding this comment

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

seems nice improve, just some questions

subprojects/hashmap.c.wrap Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
@engelmi engelmi force-pushed the integrating-external-projects branch from b9bcc2e to babdc1e Compare August 4, 2023 19:50
Copy link
Contributor

@ygalblum ygalblum left a comment

Choose a reason for hiding this comment

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

As discussed on the sync call, the build systems expect that all the required sources are part of the source code RPM. So, we cannot have meson download the files during build. Instead we should look int integrating with git-submodules

src/libhirte/common/cfg.c Outdated Show resolved Hide resolved
subprojects/hashmap.c.wrap Outdated Show resolved Hide resolved
@engelmi engelmi force-pushed the integrating-external-projects branch 3 times, most recently from f3b34e1 to 6bff112 Compare August 10, 2023 06:04
Copy link
Contributor

@ygalblum ygalblum left a comment

Choose a reason for hiding this comment

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

Minor nits, but otherwise LGTM

src/libhirte/common/cfg.c Show resolved Hide resolved
.gitconfig Outdated Show resolved Hide resolved
subprojects/README.md Show resolved Hide resolved
Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

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

This approach seems to me less clear than having copies of those projects directly in this project and what's worse for hashmap we need additional repo to have meson.build for the project.

But anyway if this is the way we want to proceed, please don't forget to adapt https://github.com/containers/hirte/tree/main/LICENSES/README.md, we need to mention correctly licenses of those subprojects

@engelmi
Copy link
Member Author

engelmi commented Aug 10, 2023

This approach seems to me less clear than having copies of those projects directly in this project

It is the first time for me using git submodules and its something one has to get used to, I think. But after a while, I think its even more clear than using an inline version - you have the reference to the exact location (incl. revision) to the included project. In the inlined version it is not directly visible that inih is actually from an external project nor which commit was used when copying or if there were custom changes (which we did for hashmap.c).

and what's worse for hashmap we need additional repo to have meson.build for the project.

I agree, that is not good and is rather a workaround - as mentioned in one of the comments above, I created a request in the original repo of hashmap.c to add a meson.build.
However, if that is a complete no-go, then we can switch the approach how to include it - from subproject to subdir like it was previously. I changed this to align (and simplify) how external projects are included.
Update: That is not really possible, unfortunately. subdir still requires a meson.build.

But anyway if this is the way we want to proceed, please don't forget to adapt https://github.com/containers/hirte/tree/main/LICENSES/README.md, we need to mention correctly licenses of those subprojects

Oh, completely missed that directory. Updated it. Could you have another look if the wording etc. is ok? @mwperina

@engelmi engelmi force-pushed the integrating-external-projects branch 2 times, most recently from 4c1eeb2 to 8f7aa51 Compare August 10, 2023 08:26
LICENSES/README.md Outdated Show resolved Hide resolved
@ygalblum
Copy link
Contributor

LGTM once the licensing reference discussion is resolved

@engelmi engelmi force-pushed the integrating-external-projects branch from 8f7aa51 to 7dd84ae Compare August 10, 2023 12:58
Instead of inlining external libraries it is
benefictial to dynamically integrate them to
get the latest bugfixes etc.
Git submodules provides a mechanism to define
these dependencies and use a specific version
of it.

Signed-off-by: Michael Engel <[email protected]>
Integrate the git submodules as subproject in
the meson.build as static library and pass down
default values for specific meson options to the
subprojects. In addition, the other tooling like
clang-tidy script needed to be adjusted to
include the libraries.

Signed-off-by: Michael Engel <[email protected]>
.gitmodules Show resolved Hide resolved
@engelmi engelmi force-pushed the integrating-external-projects branch from 7dd84ae to 37acc5f Compare August 10, 2023 18:18
@dougsland
Copy link
Contributor

@engelmi great stuff! LGTM

@dougsland dougsland self-requested a review August 11, 2023 14:11
@dougsland dougsland merged commit d5ceff4 into main Aug 11, 2023
10 checks passed
@dougsland dougsland deleted the integrating-external-projects branch August 11, 2023 14:14
mwperina added a commit to mwperina/bluechi that referenced this pull request Aug 18, 2023
After some recent changes (for example moving external libraries to
[subprojects](eclipse-bluechi#432)) we were
missing several dependencies required to build the project
successfully.

Signed-off-by: Martin Perina <[email protected]>
rhatdan pushed a commit that referenced this pull request Aug 18, 2023
* Add missing dependencies to README.developer.md

After some recent changes (for example moving external libraries to
[subprojects](#432)) we were
missing several dependencies required to build the project
successfully.

Signed-off-by: Martin Perina <[email protected]>

* Fix RPM package building documentation

1. Adds installation of required packages for RPM build
2. Updates the part around RPM build customization more clear

Signed-off-by: Martin Perina <[email protected]>

* Make building python3-bluechi conditional

For some build targets we don't want python3-bluechi to be built, so
this patch add `--without python` parameter to rpm-build command to
disable it.

Fixes: #396
Signed-off-by: Martin Perina <[email protected]>

---------

Signed-off-by: Martin Perina <[email protected]>
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