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

Open
wants to merge 25 commits into
base: master
Choose a base branch
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.

@conan-center-bot

This comment has been minimized.

@AbrilRBS AbrilRBS requested review from uilianries and removed request for AbrilRBS and danimtb February 3, 2024 23:13
Comment on lines +100 to +101
vre = VirtualRunEnv(self)
vre.generate(scope="build")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is normally not required for CMake projects. What is the purpose of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is need when option with_fmu=True. Then, during the build process some executables need runtime access to what can be found in the virtual run environment.

@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!

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.

None yet

7 participants