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

[SofaExporter] Modularize (+minor dependency cleaning) #915

Merged
merged 9 commits into from Feb 13, 2019

Conversation

5 participants
@damienmarchal
Copy link
Contributor

commented Feb 2, 2019

Hey,

Waiting for FOSDEM to start...so let's make a small cleaning PR.


This PR:

  • builds with SUCCESS for all platforms on the CI.
  • does not generate new warnings.
  • does not generate new unit test failures.
  • does not generate new scene test failures.
  • does not break API compatibility.
  • is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

damienmarchal added some commits Feb 2, 2019

[SofaExporter] Move all files to be compliant with recommanded direct…
…ory structure

The flat structure in our plugins was problematic because it was
favoring the leaking of includes out of their packages.

By putting the sources in the
src/ModuleName/*.h and exporting the src as the include path in the CMAKeLists.txt
allows to write
#include <ModuleName/thisfile.h> which is clearer.
@epernod

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2019

Thank you for your contribution. Could you please check the CI , I think there is à problem

[CMakeLists.txt] Change the ordering of SofaGUI inclusion.
Because SofaGUI is en dependency of SofaPython it needs
to be putted before.
@damienmarchal

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

Hi Erik,

Thanks for the feedback. I reorder the SofaGUI Cmake inclusion order to have it compiling.

@epernod

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

what is the goal here in fact? to put all exporter as a plugin in a next PR?
I don't understand the class move from modules/SofaExporter/ to modules/SofaExporter/src/SofaExporter/

@damienmarchal

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

Hello Erik,

As said in the PR description. This PR is converting the code into a properly package module/plugin.
The move of the files is to enforce package encapsulation.

Currently in Sofa a module/plugin that can be search through the find_package() CMake macro. A lot of
modules are exporting "{CMAKE_CURRENT_DIRECTORY}../" when they are imported.
Example:
SofaExporter
SofaPython

If a third module SofaX is just doing find_package(SofaExporter) it is possible to write
#include <SofaExporter/aaa.h>
which is expected....but also
#include <SofaPython/bbb.h>
This work because of the ../ in SofaExporter allows the SofaX to include SofaPython which is bad as this breaks package encapsulation.

The solution to fix that is to have the files to include in a subdirectory of the module/plugin.
So in our case SofaExporter is exporing its include path as "{CMAKE_CURRENT_DIRECTORY}/src/"
If done that we... the SofaX module can still do
#include <SofaExporter/aaa.h>
But the #include <SofaPython/bbb.h> will fails.

@VLinternship

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

[ci-build][with-scene-tests]

@guparan

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

I'm a bit confused about config.h inclusion pipeline. Could you explain the necessity of having both initModule.h and config.h headers?
see #904

@damienmarchal

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

To me these have two totally different usages.

config.h is containing the different parameters related to the module...and it should be included in every .h of a module because this is where the SOFA_MODULE_API is implemented.

Appart from that is the initModule.cpp/initModule.h which contains the defintion/declaration for functions needed to intialize the module.

In the past initModule.h was containing the initModule() function...but when it is a real plugin we are using the Plugin API and dlopen for that. If we want to allow people to use the module without using the dynamic library we add into initModule.h the declaration of the "C" function that are in initModule.cpp

@untereiner

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Hi guys :)
You could let cmake generate for you for each module the config.h (basically only defines for import/export symbols rules, If I am right) using generete_export_header.

@damienmarchal

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

Hi @untereiner, thanks for the tip.

@damienmarchal

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

[ci-build][with-scene-tests]

@guparan guparan merged commit 383d691 into sofa-framework:master Feb 13, 2019

6 of 7 checks passed

Jk2/[with-regression-tests] Missing.
Details
Jk2/Dashboard Builds triggered.
Details
Jk2/[with-scene-tests] Triggered in latest build.
Details
Jk2/centos_clang-5_options Build OK. FIXME: 0 unit tests, 31 scene tests
Details
Jk2/mac_clang-3.5_options Build OK. FIXME: 1 unit tests, 33 scene tests
Details
Jk2/ubuntu_gcc-5.4_options Build OK. FIXME: 0 unit tests, 31 scene tests
Details
Jk2/windows7_VS-2015_options_amd64 Build OK. FIXME: 1 unit tests, 31 scene tests
Details

@damienmarchal damienmarchal deleted the SofaDefrost:cleanFOSDEM branch Feb 25, 2019

@guparan guparan added this to the v19.06 milestone Jun 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.