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

fmilib: add recipe for version 2.4.1 #20256

Closed
wants to merge 25 commits into from

Conversation

joakimono
Copy link
Contributor

@joakimono joakimono commented Oct 2, 2023

Specify library name and version: fmilib/2.4.1

fmilibrary is a software package that enables integration of Functional Mock-up Units (FMUs). This library is an implementation of the FMI open standard.

This package is a dependency of the software provided by Open Simulation Platform.


@joakimono joakimono marked this pull request as draft October 2, 2023 08:55
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@joakimono joakimono marked this pull request as ready for review October 30, 2023 13:13
@github-actions
Copy link
Contributor

Hooks produced the following warnings for commit 1cf10e7
fmilibrary/2.4.1@#04070e62827fcbbbbbc509f1b9d3d51c
post_package(): WARN: [APPLE RELOCATABLE SHARED LIBS (KB-H077)] install_name dir of these shared libs is not @rpath: libfmilib_shared.dylib
post_package(): WARN: [MISSING SYSTEM LIBS (KB-H043)] Library '.\bin\fmilib_shared.dll' links to system library 'shlwapi' but it is not in cpp_info.system_libs.

@conan-center-bot

This comment has been minimized.

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

Thank you for your pull request. I see many patches that could be sent to the upstream as well. Using find_package is not only a recommendation by Cmake part but also much safer than mimicking where artifacts are installed.

recipes/fmilibrary/all/conanfile.py Outdated Show resolved Hide resolved
recipes/fmilibrary/all/conanfile.py Outdated Show resolved Hide resolved
recipes/fmilibrary/all/conanfile.py Outdated Show resolved Hide resolved
recipes/fmilibrary/all/conanfile.py Outdated Show resolved Hide resolved
recipes/fmilibrary/all/conanfile.py Outdated Show resolved Hide resolved
recipes/fmilibrary/all/conanfile.py Outdated Show resolved Hide resolved
@uilianries uilianries self-assigned this Oct 30, 2023
@conan-center-bot

This comment has been minimized.

    - Simplify patches
    - Download minizip sources instead of patch
      - Write replace in file for a few minizip versions, 1.2.13 and 1.3.1
    - Use CMakeDeps cmake_target_property instead of patching dependency target names
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@joakimono joakimono requested a review from valgur June 21, 2024 14:10
Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Hi! Sorry for the delay. I have some concerns/suggestions and questions, would love to get some insight on some of the changes :)

Comment on lines 151 to 158
copy(self, "fmiModel*.h", self.dependencies["fmi1"].cpp_info.components["modex"].includedirs[0],
path.join(self.build_folder, "fmis", "FMI1"))
copy(self, "fmiPlatformTypes.h", self.dependencies["fmi1"].cpp_info.components["cosim"].includedirs[0],
path.join(self.build_folder, "fmis", "FMI1"))
copy(self, "fmiFunctions.h", self.dependencies["fmi1"].cpp_info.components["cosim"].includedirs[0],
path.join(self.build_folder, "fmis", "FMI1"))
copy(self, "*.h", self.dependencies["fmi2"].cpp_info.includedirs[0],
path.join(self.build_folder, "fmis", "FMI2"))
Copy link
Member

Choose a reason for hiding this comment

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

Any insights into why these folders can not to out of source for the build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no reason. I moved them to the source folder instead.

Comment on lines 107 to 122
minizip_code = {
"1.3.1":
{
"url": "https://zlib.net/fossils/zlib-1.3.1.tar.gz",
"sha256": "9a93b2b7dfdac77ceba5a558a580e74667dd6fede4585b91eefb60f03b72df23"
},
"1.2.13":
{
"url": "https://zlib.net/fossils/zlib-1.2.13.tar.gz",
"sha256": "b3a24de97a8fdbc835b9833169501030b8977031bcb54b3b3ac13740f846ab30"
}
}
get(self,
**minizip_code[str(self.dependencies["minizip"].ref.version)],
pattern="*/minizip/*",
strip_root=True, destination=path.join(self.build_folder))
Copy link
Member

Choose a reason for hiding this comment

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

Afaik, the library needs the actual source code for minizip instead of relying in the compiled library (Would appreciate insight into this too!)

You actually get access to the dependency's conandata thru the self.dependencies accessor, so this could be simplified to something like

Suggested change
minizip_code = {
"1.3.1":
{
"url": "https://zlib.net/fossils/zlib-1.3.1.tar.gz",
"sha256": "9a93b2b7dfdac77ceba5a558a580e74667dd6fede4585b91eefb60f03b72df23"
},
"1.2.13":
{
"url": "https://zlib.net/fossils/zlib-1.2.13.tar.gz",
"sha256": "b3a24de97a8fdbc835b9833169501030b8977031bcb54b3b3ac13740f846ab30"
}
}
get(self,
**minizip_code[str(self.dependencies["minizip"].ref.version)],
pattern="*/minizip/*",
strip_root=True, destination=path.join(self.build_folder))
minizip_version = self.dependencies["minizip"].ref.version
get(self,
**self.dependencies["minizip"].conandata["sources"][minizip_version],
pattern="*/minizip/*",
strip_root=True, destination=path.join(self.build_folder))

which allivieates some of your concerns - I would still like some way to avoid this, but alas seems like this will be that way to go forward

Copy link
Contributor Author

@joakimono joakimono Jun 23, 2024

Choose a reason for hiding this comment

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

fmilib introduce minizip.c and minunz.c as functions instead of applications by changing their main functions. To do this, fmilib needs the minizip sources. It also uses other functionality in minizip, so the precompiled library is also needed. In patch file *002*.patch, you can see the new header files for the patched functions.

Your suggestion worked nicely, thank you!

    - Fetch minizip source info from conan_data
    - Copy fmi headers into source folder rather than build folder
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 5 (ad6a4bb2f343e088f11072250ca2e14450f2cf9d):

  • fmilib/2.4.1:
    All packages built successfully! (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 5 (ad6a4bb2f343e088f11072250ca2e14450f2cf9d):

  • fmilib/2.4.1:
    All packages built successfully! (All logs)

@joakimono
Copy link
Contributor Author

Hi! Sorry for the delay. I have some concerns/suggestions and questions, would love to get some insight on some of the changes :)
@AbrilRBS: Hi, have I addressed your suggestions and questions, or do you have further inquiries?

@AbrilRBS
Copy link
Member

AbrilRBS commented Jul 3, 2024

Hi @joakimono sorry for the radio silence - I've been talking with @uilianries and we're checking some changes to the recipe to make it a bit more maintenable, sorry that I didn't communitcate it better!

@joakimono
Copy link
Contributor Author

Hi @joakimono sorry for the radio silence - I've been talking with @uilianries and we're checking some changes to the recipe to make it a bit more maintenable, sorry that I didn't communitcate it better!

Hi @AbrilRBS, have you been able to check the changes for maintainability yet? I'm was hoping to get this recipe merged soon.

@uilianries
Copy link
Member

@joakimono Sorry our delay, I spent some hours analyzing fmi and it's really complicated to get unvendorized as I see, that's why you needed to apply so many patches. Only AUR (Arch Linux) and vcpkg are packaging fmilib as well, and both are not trying to unvendor everything due complexity/fragility.

I'll bring this PR to be discussed with Conan team, but personally I'm more inclined to avoid all those patches related to unvendor. This is not the first recipe in CCI that we have this situation (see BehaviorTree). I would suggest discussing first with the upstream, to enable external dependencies instead via CMake options, it could be re-used by any user in the future.

Using Conan package instead of those vendorized is good as we can control the dependencies, but not always the author wants it, we had a project using vendorized dependencies and the author did not want to use external instead, to keep all aligned and tested with that specific versions, so users would have the same behavior always.

@joakimono
Copy link
Contributor Author

Thank you for your reply. I agree that the option to unvendor dependencies is best handled by the upstream package. I posted an issue on this, modelon-community/fmi-library#100, last November, but I do not think it is prioritized. I will see if I can provide a pull request to fmi-library to achieve it, perhaps it would expedite matters.

@uilianries
Copy link
Member

@joakimono Thank you for your feedback and pointing that issue. I'll try to take a look during this weekend as side-project too. 😄

@uilianries
Copy link
Member

I sent the first PR to separate Expat this weekend: modelon-community/fmi-library#134

I think the best way is sending separated PRs, otherwise it would be hard not only to be tested, but also reviewed, as there are several dependencies. Once accepted/merged, I can send a next PR.

Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Oct 14, 2024
Copy link
Contributor

This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions.

@github-actions github-actions bot closed this Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants