Skip to content
This repository has been archived by the owner on Jul 8, 2022. It is now read-only.

Allow turning shared library building off #763

Merged
merged 6 commits into from
Sep 15, 2020
Merged

Allow turning shared library building off #763

merged 6 commits into from
Sep 15, 2020

Conversation

t-b
Copy link
Collaborator

@t-b t-b commented Sep 10, 2020

No description provided.

t-b added 2 commits September 10, 2020 12:06
@t-b
configure/cmake_linux.cmake: Fix CURL variable checking
6d8f801 
It does not make sense to check

if(A)
...
elseif(A)
...
endif()

so let's change the elseif to an else.
@t-b
CMake on Linux: Only generate either the static or the dynamic library
0aefb31 
In af72afb (Generate static library and fix compilation definitions
(tango-controls#17) (tango-controls#437), 2018-05-15) support was added for building a static
library next to the dynamic one.

As most users want to have only either library we use the newly
introduced option BUILD_SHARED_LIBS to build either one. This also
solves issues when cross-compiling where you might not have shared
libraries for the target architecture.
@t-b t-b mentioned this pull request Sep 10, 2020
Merged
mliszcz
mliszcz approved these changes Sep 10, 2020
View reviewed changes
Copy link
Collaborator

@mliszcz mliszcz left a comment

Choose a reason for hiding this comment

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

LGTM!

@t-b t-b force-pushed the backport/allow-turning-shared-library-building-off branch from e0d0d60 to 74bbee3 Compare September 10, 2020 11:24
@t-b
Copy link
Collaborator Author

t-b commented Sep 10, 2020

@mliszcz Thanks for the review. I had to unforunately fix the PR as the tests were correctly not passing.

@bourtemb
Copy link
Member

@t-b, as you mentioned yesterday in the Kernel meeting, after the merge of this PR, only the shared library or the static library will be built, no longer both together.
So, in the past, we used to build both and then both were installed together.
If we want to continue to provide/install both static and shared cppTango libraries with the Tango Source Distribution, we will have to adapt the TangoSourceDistribution to do 2 cppTango builds and 2 installations steps, then?

@t-b
Copy link
Collaborator Author

t-b commented Sep 14, 2020

If we want to continue to provide/install both static and shared cppTango libraries with the Tango Source Distribution, we will have to adapt the TangoSourceDistribution to do 2 cppTango builds and 2 installations steps, then?

I don't think you really need two different builds, but yes something like that needs to be done.

If that is not what we want, BUILD_SHARED_LIBS could also only enable/disable shared library building and we always keep building the static library.

@lorenzopivetta
Copy link

If that is not what we want, BUILD_SHARED_LIBS could also only enable/disable shared library building and we always keep building the static library.

Hi @t-b may I suggest the opposite? We always build the shared lib and only build the static library if BUILD_STATIC_LIB is defined? Based on the assumption most of the users just need the shared libs... that's our case, at least.

@t-b
Copy link
Collaborator Author

t-b commented Sep 14, 2020

@lorenzopivetta

I suggest the opposite? We always build the shared lib and only build the static library if BUILD_STATIC_LIB is
defined? Based on the assumption most of the users just need the shared libs... that's our case, at least.

This would break @JeanLucPons use case as he requested in #740 (comment):

I don't have omniORB and zmq compiled in shared, so the compilation of tango SHARED failed and prevent the
build process to end properly.

So I think the ability to choose one of them is key to flexibility.

@lorenzopivetta
Copy link

Agreed. So, do wee need two defines, e.g. BUILD_STATIC_LIB and BUILD_SHARED_LIB, at least one mandatory?

@t-b
Copy link
Collaborator Author

t-b commented Sep 14, 2020

Agreed. So, do wee need two defines, e.g. BUILD_STATIC_LIB and BUILD_SHARED_LIB, at least one mandatory?

My proposal is to have BUILD_SHARED_LIBS, which is the variable CMake suggests. If true the shared library is built, if false the static library.

@lorenzopivetta
Copy link

This means that you either build the static or the shared lib. Fine with me. Which the default will be?

@t-b
Copy link
Collaborator Author

t-b commented Sep 14, 2020

This means that you either build the static or the shared lib. Fine with me. Which the default will be?

Yes, default will be shared.

bourtemb
bourtemb approved these changes Sep 14, 2020
View reviewed changes
Copy link
Member

@bourtemb bourtemb left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Just a small suggestion to improve a test in a bash script (Could be done in another PR because I think there are some other locations having similar code)

@t-b t-b mentioned this pull request Sep 15, 2020
Closed
@t-b t-b merged commit ef0c2be into tango-controls:9.3-backports Sep 15, 2020
@t-b t-b deleted the backport/allow-turning-shared-library-building-off branch September 15, 2020 11:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mliszcz mliszcz mliszcz approved these changes

@bourtemb bourtemb bourtemb approved these changes

Assignees
No one assigned
Labels
9.3 backport high priority Ready For Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@t-b @bourtemb @lorenzopivetta @mliszcz