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

Linked to static Pthread_win. Solves #355 #358

Closed
wants to merge 2 commits into from
Closed

Linked to static Pthread_win. Solves #355 #358

wants to merge 2 commits into from

Conversation

ldoyle
Copy link

@ldoyle ldoyle commented May 25, 2020

To avoid DLL dependency in python package, and since all other libs are statically linked, too. Solves #355

(Untested since I would have to set up a build env, hope the CI test will do. If not, sorry, this is my first PR, I can manually compile it locally if necessary)

Things to consider:

  • You often read never to mix static and dynamic linking. Here, all the other libs are static, too, so this should be fine, correct?
  • If not, keeping the dynamic link is also fine, the DLL could be added to setup.py in package_data (and maybe to appveyor.yml to copy the right architecture/version).
  • To test the linking, it might be necessary to call the exact functions introduced in 9.3.2 which use Pthreads to be sure there are no segfaults/memory errors (not an expert here)

To avoid DLL dependency in python package, and since all other libs are statically linked, too.
@ldoyle
Copy link
Author

ldoyle commented May 26, 2020

Update after partial Appveyor CI crash and quick test of the py27-x64 wheel.
Evidently this is not the way to go:

  • Something seems wrong with the static lib pthreadVC2-s.lib, maybe it was compiled with a different or wrong dependency to MSVCRT than the rest of the project, for all py3.X builds, causing the Appveyor build to crash (py27 works because it uses a different MSVC compiler version I presume)
  • The py27-32/64bit wheels compiled OK, but the test with py27-x64 failed:
  • While I can install the package just fine and create a DeviceProxy to access attributes and the State() function, event subscription fails with either DevFailed('Device not found') (although working in PyTango 9.3.1 build) or with a Memory-Error exception or by crashing Python entirely (segfault?)

If no one has other opinions, I would cancel this PR and attempt the alternative path to distribute the pthread DLL (this is also recommended by their doc over static linking).

@ajoubertza
Copy link
Member

ajoubertza commented May 27, 2020

Thanks for trying this out, @ldoyle. I agree that statically linking the pthread library seems like a good approach. Not sure what is going wrong here. Although you say the pthread docs recommend against that - can you provide a link?

Some notes:

  • The pthread libraries are also built by AppVeyor. You can see the details here https://github.com/tango-controls/Pthread_WIN32. Maybe something is different there, compared to the pytango build options. Those same pthread libraries are then used in the compilation of the C++ libtango that we use for each pytango build. The cpptango build includes both static and dynamically linked versions of its library.
  • To test a function that uses pthread, you can run tango.is_omni_thread().

I also tried adding pthread to the libraries passed to the Python extension, but that didn't help either.

@NexeyaSGara As our Windows build expert, do you have any suggestions for us?

@NexeyaSGara
Copy link
Contributor

Hi,

For the pthread builds, I use nmake as pointed in the documentation.
So I'm in "default" compilation mode.
Maybe we have some define in the pytango build that are messing with the pthread behaviour.

So maybe we will have to edit the Makefile of pthread to add some definition for the compiler.

Make the build environment/ compiler flags etc as close to cppTango as possible to ensure good linking.
@ldoyle
Copy link
Author

ldoyle commented Jun 11, 2020

I haven't got it running yet, but looking at the Cmake process I have a feeling that the precompiler defs have to be cleaned up, anyway.

  • Since both cppTango and PyTango download the same Pthreads_Win, I don't think the build problem lies there.
  • @ajoubertza The Pthreads_Win websites states "[...] pthreads is normally implemented as a DLL. This has some notable advantages from the Win32 point of view, but it also more closely models existing pthread libraries on UNIX [...] (e.g. libpthread.so). Please note [...] the library can also be built for static linking if necessary." [1] So it's a recommendation, but I would still go for static build here.
  • What is the reason for using Pthreads in Pytango in the first place? In this particular case, <pthread.h> is included by omnithread.h, however I think this an error due to the missing precompiler def __WIN32__. If __WIN32__ is defined, omnithread.h instead includes windows threads. According to [2], this is a standard definition by clang+mingw, but not in Visual Studio (which only defines _WIN32, and CMakeLists.txt adds WIN32).
    ** Could this be the reason why PThreads is used here?
  • @NexeyaSGara do you remember why __POSIX_NT__ was added to the PyTango precompiler defs, as it is not present in cppTango definitions? Inside omnithread.h it has a similar but different effect to __WIN32__, seemingly also eliminating the need for the PThreads reference. Would the __WIN32__ definition be the more appropriate here?
  • Either way, it should be the same defintions as cppTango, otherwise e.g. the core of PyTango linking to cppTango uses Pthreads, while the newly added is_omni_thread uses Windows threads (as seems to be the case when setting __WIN32__ or __POSIX_NT__). This might be the reason why my first tests failed.
  • @NexeyaSGara maybe you know: Also in cppTango, what is the reason for having Pthreads? According to the Tango documentation it should only be necessary on Unix systems, and all uses of Pthreads inside cppTango source I can find are disabled if _TG_WINDOWS_ is set, and any use of omnithread.h would suffer from the same problem described above, correct?

If I find the spare time I will try to compile cppTango and PyTango manually from source, but maybe someone more experienced can say if this is at all the right direction.

[1] https://sourceware.org/pthreads-win32/ -> How does it work?
[2] https://web.archive.org/web/20140625123925/http://nadeausoftware.com/articles/2012/01/c_c_tip_how_use_compiler_predefined_macros_detect_operating_system#WindowsCygwinnonPOSIXandMinGW

@andygotz
Copy link

@ldoyle thanks for the feedback and tips. I hope someone will be able to pick this issue up soon and provide a solution. I have asked @piogor to help us.

@ajoubertza
Copy link
Member

  • @ajoubertza The Pthreads_Win ... So it's a recommendation, but I would still go for static build here.
    Thanks for the details.
  • What is the reason for using Pthreads in Pytango in the first place?

Pthreads only came in while trying to get the Windows builds to work with omnithreads.h and the related C++ code. It is quite possible that the precompiler defs are incorrect, and that we don't need pthreads at all. I think you are on the right track. I imagine you've already looked the comments on PR #327 for clues.

Unfortunately the original PRs to get Windows builds working in are incredibly long and complicated, with many dead ends. Started with #176, and then that was continued and finalised in #277.

@ajoubertza
Copy link
Member

Closing this as we have a better solution in #395. No need for the pthread.dll.

@ajoubertza ajoubertza closed this Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants