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

build: Add CMake option to use external expat #134

Merged

Conversation

uilianries
Copy link
Contributor

Hello all!

This PR adds the CMake option FMILIB_SYSTEM_EXPAT, to be able to consume expat from system, provided by a package manager.

The official CMake module for Expat is FindEXPAT and provides both EXPAT_INCLUDE_DIRS and EXPAT_LIBRARIES.

By default, this option is disabled, so it should not break users.

The entire build log using the system expat is here:

fmilib-system-expat-build.log

It's possible to see:

-- Found EXPAT: /usr/lib/x86_64-linux-gnu/libexpat.so (found version "2.1.0")

Related to #100

Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@modelonrobinandersson
Copy link
Collaborator

Hi @uilianries and thank you for your PR. There is just something I have to check on my end, I will get back to you on Monday again.

@uilianries
Copy link
Contributor Author

@modelonrobinandersson Thank you! Please, take your time to validate this change, I don't want to break users 😅

@modelonrobinandersson
Copy link
Collaborator

Hi @uilianries, I just need a few more days as I am trying to sort something out on my end, I will be back as soon as possible.

@modelonrobinandersson
Copy link
Collaborator

Hi again @uilianries, in order to accept contributions to FMIL there is the following requirement: https://github.com/modelon-community/contributing
I will only be able to merge your request after this process has been finished, let me know if you have any questions.

@uilianries
Copy link
Contributor Author

@modelonrobinandersson Hello, is it possible to send electronic signed instead? I don't have a printer.

@modelonrobinandersson
Copy link
Collaborator

@modelonrobinandersson Hello, is it possible to send electronic signed instead? I don't have a printer.

Hi! Yes that works as well.

@uilianries
Copy link
Contributor Author

@modelonrobinandersson Hello again, I just sent the email for the CLA, but there is not technical issue on Modelon side. I opened a separated issue reporting the case: modelon-community/contributing#2

@filip-stenstrom
Copy link
Collaborator

To be clear, I will review this, but need to finish some other work first. Will look at this when I can.

Copy link
Collaborator

@filip-stenstrom filip-stenstrom left a comment

Choose a reason for hiding this comment

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

Good addition. FMIL should have better dependency management and this is one step in that direction.

Things were made a bit complicated since we already have e.g. FMILIB_FIND_PACKAGE_ZLIB which behaves slightly different (bundles zlib with FMIL). But we think not doing that (like done here), and instead listing dependencies is the way forward.

I've added suggestions for most changes.

Thanks for contributing, and sorry for delay in reviewing.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Config.cmake/fmixml.cmake Show resolved Hide resolved
Config.cmake/fmixml.cmake Outdated Show resolved Hide resolved
Config.cmake/fmixml.cmake Show resolved Hide resolved
uilianries and others added 5 commits November 7, 2024 21:19
Co-authored-by: Filip Stenström <filip.stenstrom@modelon.com>
Co-authored-by: Filip Stenström <filip.stenstrom@modelon.com>
Co-authored-by: Filip Stenström <filip.stenstrom@modelon.com>
Co-authored-by: Filip Stenström <filip.stenstrom@modelon.com>
@uilianries
Copy link
Contributor Author

@filip-stenstrom Thank you for your review. All committed all your suggestions! Please, review again when possible. Don't worry about the delay, as maintainer of open source project too, I totally understand 😉

@filip-stenstrom
Copy link
Collaborator

Thanks, all good now. Merging.

@filip-stenstrom filip-stenstrom merged commit e871e37 into modelon-community:master Nov 8, 2024
1 check passed
@jschueller
Copy link
Contributor

jschueller commented Nov 28, 2024

hi @uilianries, do you plan to do zlib/minizip too ?

@uilianries
Copy link
Contributor Author

@jschueller That's possible.

@uilianries uilianries deleted the feature/external-expat branch November 28, 2024 14:26
@jschueller
Copy link
Contributor

that would be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants