Shared/static switching on windows

[ltjax]

This is the PR regarding issue #656 . It's not done yet, but I want to see what the CI says so far. The PR itself is not laser focused, I did adhere to the boy scout rule where appropriate (and where I needed it to get this to build)

[ltjax]

@bourtemb Any ideas for why the one test on the one OS fails on travis? Was it something that I broke or a flaky test?

[bourtemb]

@bourtemb Any ideas for why the one test on the one OS fails on travis? Was it something that I broke or a flaky test?

This ring_depth test is currently a known unstable test, so you probably didn't break anything there.

[ltjax]

I'm confused - why did the check for 85e301e fail, and without an error message - while the link that was originally shown for the appveyor CI https://ci.appveyor.com/project/Ingvord/cpptango/builds/31233181 succeeded? They seem to be different appveyor accounts, and the failing one is @bourtemb 's - what is going on?

Either way, this is almost done, I just don't know what to do with the install step. It's somewhat impractical to supply both static and dynamic libs together in the same package - is that something you can live without? I think it is much more likely that people will use release and debug together on windows (since that is how VS operates, by default) than static and dynamic, so I advise to just make them separate packages.

[bourtemb]

I'm confused - why did the check for 85e301e fail, and without an error message - while the link that was originally shown for the appveyor CI https://ci.appveyor.com/project/Ingvord/cpptango/builds/31233181 succeeded?

It failed after 1h execution (appveyor limit). It took too long to compile everything. Re-enabling pre-compiled headers seems like a good move to reduce the compilation time.
win32-msvc15 build got lucky to succeed after 59min 10s.

They seem to be different appveyor accounts, and the failing one is @bourtemb 's - what is going on?

There are different appveyor accounts triggered by tango-controls/cppTango indeed.
I don't know how appveyor is dealing with that. It probably shows a link to the build link to the account for which the build finished first.

Either way, this is almost done, I just don't know what to do with the install step. It's somewhat impractical to supply both static and dynamic libs together in the same package - is that something you can live without? I think it is much more likely that people will use release and debug together on windows (since that is how VS operates, by default) than static and dynamic, so I advise to just make them separate packages.

We will raise this question at the next Tango kernel teleconf meeting next Thursday if we don't get any answer as comment on this PR.
I am not considering myself as a Tango on Windows user so I cannot speak for these users.

[ltjax]

Okay, I think this is mostly done now. Finally. There's still lots to be improved on the CMake build system (e.g. it should do the file preprocessing in targets, so you can have actual incremental builds, and we could build the different variants in different appveyor jobs), but I can only do so much with limited time ;-)
Does the release process still work like this?

[ltjax]

We will raise this question at the next Tango kernel teleconf meeting next Thursday if we don't get any answer as comment on this PR.

@bourtemb did you get any news on this?
If my current way is alright, I would hate to delay this PR. It was a ton of work and is probably prone to conflicts the longer it stays stale.

[bourtemb]

We will raise this question at the next Tango kernel teleconf meeting next Thursday if we don't get any answer as comment on this PR.

@bourtemb did you get any news on this?

No. Sorry. I forgot to ask the question. In any case, I think there were only Linux users at the last Tango kernel teleconf meeting so we would probably not have any answer anyway.

I just sent another e-mail on the Tango mailing list, asking for feedback.
@NexeyaSGara , do you see any potential issue with what @ltjax is proposing?

If my current way is alright, I would hate to delay this PR. It was a ton of work and is probably prone to conflicts the longer it stays stale.

I understand you. Thank you for the time spent on this.
So if I understand well, you're proposing to provide a package with dynamic libraries, containing the debug and release versions in the same package, and to provide a separate package for static libraries, containing also release and debug versions in the same package?

[bourtemb]

Either way, this is almost done, I just don't know what to do with the install step. It's somewhat impractical to supply both static and dynamic libs together in the same package - is that something you can live without? I think it is much more likely that people will use release and debug together on windows (since that is how VS operates, by default) than static and dynamic, so I advise to just make them separate packages.

@ltjax, we talked about that during the Tango Kernel Teleconf meeting today.
Since there was no feedback from the Tango on Windows community, we have decided to give you the green light to proceed as you suggested.

[ltjax]

Either way, this is almost done, I just don't know what to do with the install step. It's somewhat impractical to supply both static and dynamic libs together in the same package - is that something you can live without? I think it is much more likely that people will use release and debug together on windows (since that is how VS operates, by default) than static and dynamic, so I advise to just make them separate packages.

@ltjax, we talked about that during the Tango Kernel Teleconf meeting today.
Since there was no feedback from the Tango on Windows community, we have decided to give you the green light to proceed as you suggested.

Thanks for that! I think this is done from my end. Travis is randomly failing again, but I didn't change anything there.

[mliszcz]

I attempted to review this PR - and just left one comment regarding the project call. I know it was there before but I'm just thinking if it can be removed. Maybe someone else will recall why it was there.

The clean-up you did in Windows-related parts is definitely a good step. However I cannot comment on the changes from functional point of view due to lack of any experience with building and using Tango on Windows.

[mliszcz]

LGTM, thanks!

[t-b]

Overall a definite improvement over the old code. And now even with release and debug builds!!

Some nits:

• Please rebase against tango-9-lts to get rid of the unnecessary merge commits
• MSVC9/10/12 is not supported anymore. So we also don't need to handle that in the following locations:

configure/CMakeLists.txt:        if(MSVC90)
configure/CMakeLists.txt:        endif(MSVC90)
configure/CMakeLists.txt:        if(MSVC10)
configure/CMakeLists.txt:        endif(MSVC10)
configure/CMakeLists.txt:        if(MSVC12)
configure/CMakeLists.txt:        endif(MSVC12)
configure/cmake_win.cmake:    if(MSVC90)
configure/cmake_win.cmake:    endif(MSVC90)
configure/cmake_win.cmake:    if(MSVC10)
configure/cmake_win.cmake:    endif(MSVC10)
configure/cmake_win.cmake:    if(MSVC12)
configure/cmake_win.cmake:    endif(MSVC12)
configure/cmake_win_defs.cmake:if(MSVC14)
configure/cmake_win_defs.cmake:endif(MSVC14)
configure/cpack_win.cmake:if(MSVC90)

Found with git grep --ignore-case -P "msvc[0-9]+" :^log4tango.
9904f75 (appveyor.yml: Remove support for old MSVC versions, 2020-01-21) has the list of supported MSVC versions.

Pushed out into #719.

• configure/cpack_win.cmake has some odd logic by first setting the values and then overriding them if required. How about

$git diff .
diff --git a/configure/cpack_win.cmake b/configure/cpack_win.cmake
index 3179cfd6..42477b4d 100644
--- a/configure/cpack_win.cmake
+++ b/configure/cpack_win.cmake
@@ -1,19 +1,17 @@
 
 set(COMPILER_TAG ${CMAKE_VS_PLATFORM_TOOLSET})
 string(TOLOWER ${CMAKE_BUILD_TYPE} BUILD_TYPE_TAG)
-set(ARCH_TAG "x86")
-set(VARIANT_TAG "static")
-
-if(MSVC90)
-    set(COMPILER_TAG "v90")
-endif()
 
 if(CMAKE_CL_64)
-    set(ARCH_TAG "x64")
+  set(ARCH_TAG "x64")
+else()
+  set(ARCH_TAG "x86")
 endif()
 
-if (TANGO_BUILD_SHARED)
+if(TANGO_BUILD_SHARED)
    set(VARIANT_TAG "shared")
+else()
+  set(VARIANT_TAG "static")
 endif()
 
 set(CPACK_PACKAGE_FILE_NAME libtango_${LIBRARY_VERSION}_${COMPILER_TAG}_${ARCH_TAG}_${VARIANT_TAG}_${BUILD_TYPE_TAG})

instead.

• The commit message of dc5f3d9 (Do not link against omniORBs MSVC stubs, 2020-03-04) should explain why that is a good change. This is not obvious now and will be even worse in 5 years.

[ltjax]

Overall a definite improvement over the old code. And now even with release and debug builds!!

Some nits:

* Please rebase against tango-9-lts to get rid of the unnecessary merge commits

* The commit message of [dc5f3d9](https://github.com/tango-controls/cppTango/commit/dc5f3d942e2c887c22d475e4d89f1c48da4917fd) (Do not link against omniORBs MSVC stubs, 2020-03-04) should explain why that is a good change. This is not obvious now and will be even worse in 5 years.

Okay, so we tried the rebase today, and it does not seem practical to go through the whole branch as there are conflicts in almost every commit, and I basically have to redo all the work I did in the merged. We gave up after 7/21. I guess the (painful) lesson learned is that mixing merging and rebasing is a very bad idea.

So my suggestion would be to use a squashed commit instead, but then do not have a specific commit for the removal of msvcstub.
So either, I remove that from this PR and we handle it in another change/PR or I add a source-code comment in that file. One of those options okay for you?
To be honest, I only removed msvcstub because I started developing this patch with the conan versions of omniORB and that was not even exporting msvcstub, simply because I was following the docs and I did not get any linker errors

[bourtemb]

So my suggestion would be to use a squashed commit instead, but then do not have a specific commit for the removal of msvcstub.

A squashed commit sounds like the easiest and fastest solution indeed.
We can always add a comment in the squashed commit message explaining the removal of msvcstub at the moment of the merge.

[ltjax]

Alright, let's do that then. I can obviously not edit the merge message, but I can supply a text:
An alternative for the regular omniORB libraries, the msvcstub contains only stubs and makes it possible to link the orbcore DLL under MSVC++ 5.0 due to destructor inlining behavior.

[t-b]

@ltjax Thanks for trying,