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

Add options to handle static and static runtime builds. #299

Draft
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

gautierbureau
Copy link
Member

@gautierbureau gautierbureau commented Mar 19, 2021

Signed-off-by: Gautier Bureau gautier.bureau@gmail.com

Please check if the PR fulfills these requirements (please use '[x]' to check the checkboxes, or submit the PR and then click the checkboxes)

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Enables to build only static/shared versions of the library and use static runtime on windows

What is the current behavior? (You can also link to an open issue here)
Static and shared versions are always built

What is the new behavior (if this is a feature change)?
Choose a specific version static/shared or both. Use a static boost library. Use static runtime on windows.

sebalaig and others added 26 commits January 21, 2021 13:05
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
…evel (#242) (#251)

Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
… higher (#264)

Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
* Always use stdcxx::optional instead of direct usage of boost::optional (#221)
* Use optional::operator() to check that an optional is set or not (#216)

Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
…tions (#220) (#268)

Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
…#228) (#266)

Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Mathieu BAGUE <mathieu.bague@rte-france.com>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Mathieu BAGUE <mathieu.bague@rte-france.com>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Mathieu BAGUE <mathieu.bague@rte-france.com>
…tions (#281)

Signed-off-by: Luma <zamarrenolm@aia.es>
Signed-off-by: Mathieu BAGUE <mathieu.bague@rte-france.com>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
* Add Bus::getP() and Bus::getQ()
* Add Network::getCountries()
* Rename HvdcLine::*NominalVoltage into HvdcLine::*NominalV
* Rename getNominalVoltage into getNominalV and setNominalVoltage into setNominalV

Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
@gautierbureau gautierbureau force-pushed the static_runtime_msvc branch 3 times, most recently from a7d0604 to 20039c8 Compare March 19, 2021 11:17
gautierbureau and others added 2 commits March 19, 2021 14:42
Signed-off-by: Gautier Bureau <gautier.bureau@gmail.com>
Signed-off-by: Sébastien LAIGRE <slaigre@silicom.fr>
@@ -36,9 +61,28 @@ if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU" OR "${CMAKE_CXX_COMPILER_ID}" STRE
# set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "${CMAKE_CXX_FLAGS_RELWITHDEBINFO} -D_GLIBCXX_DEBUG")
elseif (MSVC)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd4244 /wd4250 /wd4251 /wd4267 /wd4275")
if (NOT BUILD_SHARED_LIBS AND BUILD_STATIC_LIBS)
if (MSVC_STATIC_RUNTIME_LIBRARY)
set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")

Choose a reason for hiding this comment

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

This variable has been added in cmake v3.15 (https://cmake.org/cmake/help/git-stage/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html). Is there an equivalent for older version?

CMakeLists.txt Outdated
else ()
set(Boost_USE_STATIC_LIBS OFF)
set(Boost_USE_STATIC_RUNTIME OFF)
endif ()

Choose a reason for hiding this comment

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

I propose to set Boost_USE_STATIC_RUNTIME according to the value of MSVC_STATIC_RUNTIME_LIBRARY even if BOOST_STATIC_LIBS is not set.

set(Boost_USE_STATIC_LIBS ${BOOST_STATIC_LIBS})
set(Boost_USE_STATIC_RUNTIME ${MSVC_STATIC_RUNTIME_LIBRARY})

Copy link
Member Author

Choose a reason for hiding this comment

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

done

CMakeLists.txt Outdated
option(BUILD_SHARED_LIBS "Enable/disable build of shared libraries." ON)
option(BUILD_SHARED_EXTENSIONS "Enable/disable build of shared extensions." ON)
option(BUILD_STATIC_LIBS "Enable/disable build of static libraries." ON)
option(BUILD_STATIC_EXTENSIONS "Enable/disable build of static extensions." ON)

Choose a reason for hiding this comment

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

It's arguable, but I think we should not allow people to mix static and dynamic libraries. I propose to keep only BUILD_SHARED_LIBS and BUILD_STATIC_LIBS. I wonder if we should add an option for each extension library

if (BOOST_STATIC_LIBS AND BUILD_SHARED_LIBS)
set(BOOST_STATIC_LIBS OFF)
message(WARNING "BOOST_STATIC_LIBS disabled if you want to build shared libraries. Use this option with BUILD_SHARED_LIBS=OFF")
endif ()

Choose a reason for hiding this comment

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

I would raise an error in that case instead of making an arbitrary choice.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

CMakeLists.txt Outdated

if (BUILD_TESTS AND NOT BUILD_SHARED_LIBS)
message(FATAL_ERROR "BUILD_TESTS is ON but BUILD_SHARED_LIBS is OFF, you should turn ON BUILD_SHARED_LIBS or turn off BUILD_TESTS.")
endif ()

Choose a reason for hiding this comment

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

Maybe we have to do something similar for the examples. Maybe we can find a way to link the examples and the tests to the IIDM library, using a postfix for example (empty or -static).

Mathieu BAGUE and others added 3 commits March 25, 2021 07:11
Signed-off-by: Mathieu BAGUE <mathieu.bague@rte-france.com>
* Make registerExtension public
* Add an example to show how to use an XIIDM extension

Signed-off-by: Mathieu BAGUE <mathieu.bague@rte-france.com>
Signed-off-by: Gautier Bureau <gautier.bureau@gmail.com>
Signed-off-by: Gautier Bureau <gautier.bureau@gmail.com>
@gautierbureau
Copy link
Member Author

gautierbureau commented Apr 7, 2021

Following a discussion with @mathbagu this PR will be put on hold indefinitely. Some conclusions on the proposed solution and future work that could be bone:

  • as it is now the unit tests can now be launched with a static or shared build but with separate cmake build directories. Previously unit tests were only tested against the shared versions of the library.
  • One issue with the proposed solutions is that the methods ExtensionProviders::loadExtensions and ExtensionProviders::loadLibrary are not tested anymore because the class ExtensionFixture is not used anymore to load extensions but rather the registerExtensionmethod is used.
  • Following the two previous issues we stated that unit tests should be launched in static and shared versions in the same cmake build and that there should be distinct target to test them to have code compiled in one version of the target and not the other. For exemple for the shared unit tests ExtensionFixture should still be used and for the the static version a new class similar to what it is proposed here should be used to registerExtension.

@gautierbureau gautierbureau marked this pull request as draft April 7, 2021 13:27
@gautierbureau gautierbureau requested review from mathbagu and removed request for mathbagu April 7, 2021 13:27
@sonarcloud
Copy link

sonarcloud bot commented Apr 7, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mathbagu mathbagu changed the base branch from integration/v1.4.0 to integration/v1.5.0 April 12, 2021 10:11
Base automatically changed from integration/v1.5.0 to main January 10, 2022 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants