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

Allow static libs #528

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow static libs #528

wants to merge 1 commit into from

Conversation

hefroy
Copy link
Contributor

@hefroy hefroy commented Sep 27, 2023

  • Remove SHARED option from add_library to allow static libs, CMake then respects the standard BUILD_SHARED_LIBS option.
  • Set the default value of BUILD_SHARED_LIBS to ON.
  • Modify plugin_manager_impl to call the plugin init function for built-in plugins via a hook, whilst still supporting external shared library plugins.
  • Add plugin init hook function to each built-in plugin.
  • Add plugin libs to link libs in CMake export config when static libs are enabled.

@hefroy hefroy changed the title Allow static libs WIP: Allow static libs Sep 28, 2023
@hefroy hefroy changed the title WIP: Allow static libs Allow static libs Sep 28, 2023
@hefroy hefroy changed the title Allow static libs WIP: Allow static libs Sep 28, 2023
@hefroy hefroy changed the title WIP: Allow static libs Allow static libs Sep 28, 2023
@goncaloalmeida
Copy link
Contributor

goncaloalmeida commented Nov 10, 2023

we will check this change internally to see if this is causing any other problems
@hefroy coul dyou please rebase your PR to last version of vsomeip-lib? Thanks!
cc @fcmonteiro @DiogoPedrozza

Copy link
Contributor

@goncaloalmeida goncaloalmeida left a comment

Choose a reason for hiding this comment

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

please rebase/fix merge issues

@hefroy
Copy link
Contributor Author

hefroy commented Nov 13, 2023

Rebased ✔️

@goncaloalmeida
Copy link
Contributor

Rebased ✔️

thanks! I will test this PR internally.

Copy link
Contributor

@goncaloalmeida goncaloalmeida left a comment

Choose a reason for hiding this comment

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

These changes are not building on the windows env.:
CMake Error at test/network_tests/CMakeLists.txt:4264 (add_dependencies): The dependency target "routingmanagerd" of target "build_network_tests" does not exist.

@hefroy
Copy link
Contributor Author

hefroy commented Dec 13, 2023

These changes are not building on the windows env.: CMake Error at test/network_tests/CMakeLists.txt:4264 (add_dependencies): The dependency target "routingmanagerd" of target "build_network_tests" does not exist.

This seemed to be caused by some other issue on the master branch when I last rebased the branch. I've just rebased it again and now I don't see that CMake error and for me the Windows build compiles ok.

@kheaactua
Copy link
Contributor

I created a similar patch for 3.1.20, I was never really happy with it, but maybe we can compare patches.

I don't remember all the details right now, but my CMake changes were much more extensive, and I had to fix the way the singletons (logger, security, etc) were created and remove any lib constructor/destructor. In the end it seemed to work but I had a segfault on exit in examples/routingmanagerd

- Remove SHARED option from add_library to allow static libs,
  CMake then respects the standard BUILD_SHARED_LIBS option.
- Set the default value of BUILD_SHARED_LIBS to ON.
- Modify plugin_manager_impl to call the plugin init function
  for built-in plugins via a hook, whilst still supporting
  external shared library plugins.
- Add plugin init hook function to each built-in plugin.
- Add plugin libs to link libs in CMake export config when
  static libs are enabled.
@hefroy hefroy force-pushed the allow-static-libs branch from 8a5eb0b to eca0636 Compare October 21, 2024 12:04
@kheaactua
Copy link
Contributor

I created a similar patch for 3.1.20, I was never really happy with it, but maybe we can compare patches.

I don't remember all the details right now, but my CMake changes were much more extensive, and I had to fix the way the singletons (logger, security, etc) were created and remove any lib constructor/destructor. In the end it seemed to work but I had a segfault on exit in examples/routingmanagerd

I think the most recent vsomeip release only has [one lib destructor here](https://github.com/search?q=repo%3ACOVESA%2Fvsomeip+__attribute__&type=code, CommonAPI (capicxx-core-runtime) has two lib constructors though (not sure if you're using CommonAPI)

Looking at the vsomeip one, it could probably be ignored..

@hefroy
Copy link
Contributor Author

hefroy commented Oct 21, 2024

@kheaactua:

In the end it seemed to work but I had a segfault on exit in examples/routingmanagerd

I also found the same, and have opened #789 separately to fix a couple of use-after-free destruction order issues.
I've just rebased this PR to master and tested it again with #789 also applied - routingmanagerd shuts down without segfault.

Looking at the vsomeip one, it could probably be ignored

Agreed.

CommonAPI (capicxx-core-runtime) has two lib constructors though (not sure if you're using CommonAPI)

Yes I am also using CommonAPI and have two PRs for adding static library support - probably better to discuss those over there:
COVESA/capicxx-core-runtime#42
COVESA/capicxx-someip-runtime#27

@hefroy
Copy link
Contributor Author

hefroy commented Dec 16, 2024

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.

3 participants