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

CMakeLists.txt: eliminate floating dependencies #2914

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

kanavin
Copy link
Contributor

@kanavin kanavin commented Feb 16, 2024

Prior to this, the respective build options would get enabled depending on whether or not these components were found on the build system.

I believe it's better to make them strict (add REQUIRED parameter), so that rpm builds are always deterministic, subject to cmake options.

Resolves: #2855

Copy link
Member

@pmatilai pmatilai left a comment

Choose a reason for hiding this comment

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

These are all optional dependencies and must remain that way, some of these dependencies aren't available outside Linux at all. Without the corresponding cmake options this is a no-go.

@Conan-Kudo
Copy link
Member

We didn't even do this with the Autotools build scripts, I don't think we should make it so strict in the CMake build script either.

Prior to this, the respective build options would get enabled depending
on whether or not these components were found on the build system.

I believe it's better to make them strict (add REQUIRED parameter),
so that rpm builds are always deterministic, subject to cmake options.

Resolves: rpm-software-management#2855
@dmnks
Copy link
Contributor

dmnks commented Mar 5, 2024

Now with the cmake options in place, this looks fine to me.

Copy link
Member

@Conan-Kudo Conan-Kudo left a comment

Choose a reason for hiding this comment

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

Sure, I suppose.

@dmnks
Copy link
Contributor

dmnks commented Mar 5, 2024

That said... This does make the cmake file a bit noisier. I wonder what the actual use cases are for disabling these (versus just not installing the given build dependency in the first place, assuming a clean build system or container is set up for the build).

A quick googling also reveals: https://blogs.kde.org/2011/11/11/cool-new-stuff-cmake-286-3-standard-way-disable-optional-packages/

@dmnks
Copy link
Contributor

dmnks commented Mar 5, 2024

OK, the above cmake feature seems to only work with find_package(), not pkg_check_modules()...

@pmatilai
Copy link
Member

pmatilai commented Mar 5, 2024

Yeah, while the patch here just mirrors what we already do, it does feel a bit draconian.

The autotools-era standard was to autodiscover unless overridden one way or the other, when moving to cmake I just took the shortest shortcut I could find: make most stuff mandatory, allow disabling. OTOH I don't think there were any switches for the IO stuff even in autotools, which is why those options were not replicated to cmake.

@kanavin
Copy link
Contributor Author

kanavin commented Mar 5, 2024

That said... This does make the cmake file a bit noisier. I wonder what the actual use cases are for disabling these (versus just not installing the given build dependency in the first place, assuming a clean build system or container is set up for the build).

The problem with not installing items is that one can't be certain if they are indeed not installed. They can be pulled in indirectly through some other dependency, or appear later on after a system update. That happens quietly, and will change the way rpm builds. The cmake option on the other hand guarantees that the feature will not be enabled (or conversely, if it is enabled via the option, then the build will fail if dependencies are absent).

@dmnks
Copy link
Contributor

dmnks commented Mar 5, 2024

The problem with not installing items is that one can't be certain if they are indeed not installed. They can be pulled in indirectly through some other dependency, or appear later on after a system update. That happens quietly, and will change the way rpm builds. The cmake option on the other hand guarantees that the feature will not be enabled (or conversely, if it is enabled via the option, then the build will fail if dependencies are absent).

Oh, indeed. Thanks for clarifying! It makes a lot of sense then.

@dmnks dmnks merged commit 26a1ccf into rpm-software-management:master Mar 6, 2024
1 check passed
@dmnks dmnks added build Build-system related RFE labels Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build-system related RFE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve build determinism (replace soft dependencies with strict ones)
4 participants