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

Add missing pthreadVC2.dll for Windows builds #367

Closed
wants to merge 9 commits into from

Conversation

wkitka
Copy link

@wkitka wkitka commented Jun 17, 2020

Add pthreadVC2.dll to package_data in setup.py for Windows' builds.
Fix #355

@wkitka wkitka marked this pull request as draft June 17, 2020 14:22
@wkitka wkitka changed the title WIP: PthreadVC2 Add pthreadVC2.dll. Jun 19, 2020
@wkitka wkitka marked this pull request as ready for review June 19, 2020 14:19
@andygotz
Copy link

Dear @wkitka , does this fix #355? It seems like it does.
Can @reszelaz and @ldoyle test this path to see if it fixes their problem on Windows?

@wkitka
Copy link
Author

wkitka commented Jun 22, 2020

Hi @andygotz,
Yes, it fixes #355.

@ajoubertza ajoubertza changed the title Add pthreadVC2.dll. Add missing pthreadVC2.dll for Windows builds Jun 24, 2020
Copy link
Member

@ajoubertza ajoubertza left a comment

Choose a reason for hiding this comment

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

This looks like a very simple solution in the end. Nice work!
(And sorry for the delay reviewing this)

We just need someone with Windows to confirm that the builds work, or have you tested all of them on a Windows system??

setup.py Outdated
Comment on lines 500 to 508
if WINDOWS:
package_data = {
'tango.databaseds': ['*.xmi', '*.sql', '*.sh', 'DataBaseds'],
'tango': ['*.dll'],
}
else:
package_data = {
'tango.databaseds': ['*.xmi', '*.sql', '*.sh', 'DataBaseds'],
}
Copy link
Member

Choose a reason for hiding this comment

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

Not critical, but it could be a little simpler:

Suggested change
if WINDOWS:
package_data = {
'tango.databaseds': ['*.xmi', '*.sql', '*.sh', 'DataBaseds'],
'tango': ['*.dll'],
}
else:
package_data = {
'tango.databaseds': ['*.xmi', '*.sql', '*.sh', 'DataBaseds'],
}
package_data = {
'tango.databaseds': ['*.xmi', '*.sql', '*.sh', 'DataBaseds'],
}
if WINDOWS:
package_data['tango'] = ['*.dll']

Copy link
Author

Choose a reason for hiding this comment

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

Hi!
Thanks for the review! I pushed changes with your suggestion.
I tested packages for python 2.7, 3.6 and 3.7 as described in #355 :

  • create a new virtual environment
  • conda install numpy
  • pip install "new PyTango package from appveyor"
  • python > import tango > it works!

It would be nice to include such test in appveyor. I'll create issue for that later.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for notes on testing. Even better would be to run the full PyTango test suite in AppVeyor.

Could you please run the the tests manually in your conda test environment? In this related PR, this comment noted that there were only errors when actually exercising a device.

Choose a reason for hiding this comment

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

@wkitka I have stupid question - how does one do: pip install "new PyTango package from appveyor"?

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately event subscription fails with errors:

  • Python 2.7:
PyTango.DevFailed: DevFailed[
DevError[
    desc = Device  not found
  origin = Util::get_device_by_name()
  reason = API_DeviceNotFound
severity = ERR]

DevError[
    desc = Device  not found
  origin = DServer::event_subscription
  reason = API_DeviceNotFound
severity = ERR]

DevError[
    desc = Failed to execute command_inout on device , command ZmqEventSubscriptionChange
  origin = Connection::command_inout()
  reason = API_CommandFailed
severity = ERR]

DevError[
    desc = Device server send exception while trying to register event
  origin = EventConsumer::connect_event()
  reason = API_DSFailedRegisteringEvent
severity = ERR]
]
Assertion failed: Successful WSASTARTUP not yet performed (..\..\..\src\signaler.cpp:377)
  • Python 3.7
MemoryError 
Assertion failed: Successful WSASTARTUP not yet performed (..\..\..\src\signaler.cpp:377)

Looks like it's connected to ZMQ. I'll try to solve that.

Copy link
Author

Choose a reason for hiding this comment

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

@andygotz you need to download artifact from AppVeyor:
https://ci.appveyor.com/project/tiagocoutinho/pytango/builds/33728106
and install it.
Example for Python 3.7:
artifacts: https://ci.appveyor.com/project/tiagocoutinho/pytango/builds/33728106/job/96bh0m43mol99xbi/artifacts
command: pip.exe install pytango-9.3.3.dev0-cp37-cp37m-win_amd64.whl

Copy link
Member

Choose a reason for hiding this comment

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

@wkitka I realise that we've never tried the PyTango test suite in Windows before, so maybe it never worked. You could try the 9.3.1 version for reference. It would be interesting to know if the problems you see above were there before.

Copy link
Author

Choose a reason for hiding this comment

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

I wasn't able to quickly run PyTango test suite on Windows so I used custom script to test event subscription. Event subscription works in PyTango 9.3.1 on Windows.
There is one interesting thing: I get Assertion failed: Successful WSASTARTUP not yet performed (..\..\..\src\signaler.cpp:377) error during closing script (PyTango 9.3.1 on Windows). It probably means that event subscription's errors in this PR aren't related to ZMQ.
I created #369 - PyTango test suite in AppVeyor.

Copy link
Member

Choose a reason for hiding this comment

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

@wkitka Thanks. Does event subscription work with your custom test script and the new build of 9.3.3dev0+ on Windows?

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately, they don't. I'll look into that.

@wkitka wkitka marked this pull request as draft June 26, 2020 11:50
@reszelaz
Copy link
Contributor

Can @reszelaz and @ldoyle test this path to see if it fixes their problem on Windows?

Hi,
I'm just back from a longer break, is there still anything I can help with?
Cheers!

@ajoubertza
Copy link
Member

I'm just back from a longer break, is there still anything I can help with?

Hi @reszelaz. I don't think there is anything that needs testing at the moment. @wkitka got the build to complete with a pthread DLL included, but reported that events didn't work when testing those builds on Windows. That issue would need to be resolved first. If you want to help with fixing the underlying issue, that would probably be welcomed by @wkitka.

@wkitka
Copy link
Author

wkitka commented Aug 12, 2020

Can @reszelaz and @ldoyle test this path to see if it fixes their problem on Windows?

Hi,
I'm just back from a longer break, is there still anything I can help with?
Cheers!

Hej @reszelaz,
thanks for the proposition! I'll let you know when it'll be ready to test.

@dmiego
Copy link

dmiego commented Oct 21, 2020

Hello.
Do you have any updates?

I've found an issue similar to the mentioned above zeromq/czmq#1788
It seems like it's actually related to ZMQ, and I get such problems often, on Pytango 9.2.* too.

@t-b
Copy link

t-b commented Nov 17, 2020

@Diego91RA Could you create a separate issue? Do you have a recipe?

@dmiego
Copy link

dmiego commented Nov 17, 2020

@t-b I suppose it's cppTango issue in the https://github.com/tango-controls/cppTango/blob/tango-9-lts/cppapi/client/event.cpp#L1830, if I understand correctly comments from czmq.

Anyway, we have it only with PyTango, so I create it here.

@t-b
Copy link

t-b commented Nov 17, 2020

@Diego91RA From just guessing I would say we need to explicitly shutdown the zmq context.

@dmiego
Copy link

dmiego commented Nov 17, 2020

@t-b Done. #390

@ajoubertza
Copy link
Member

Closing this as we have a better solution in #395. No need for 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.

Windows: DLL load fail with 9.3.2 64bit wheel
6 participants