8000 Fix DevEncoded commands on Python 3 by ajoubertza · Pull Request #281 · tango-controls/pytango · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Apr 7, 2022. It is now read-only.

Fix DevEncoded commands on Python 3 #281

Merged
merged 5 commits into from
Jul 24, 2019
Merged

Fix DevEncoded commands on Python 3 #281

merged 5 commits into from
Jul 24, 2019

Conversation

ajoubertza
Copy link
Member
@ajoubertza ajoubertza commented Jun 25, 2019 •
edited
Loading

Work in progress.

  • Added unit test to ensure that DevEncoded (aka bytearray) commands work the same as the attributes.
  • Fix implementation so that tests pass.

Fixes: issue #280

@ajoubertza
Add unit test for DevEncoded commands
26ab592 
Ensure that DevEncoded (aka bytearray) commands work the same as
the attributes.
@ajoubertza ajoubertza mentioned this pull request Jul 17, 2019
Closed
15 tasks
ajoubertza
ajoubertza commented Jul 22, 2019
View reviewed changes
ext/server/command.cpp Outdated
#if PY_MAJOR_VERSION >= 3
boost::python::object encoded_data(
boost::python::handle<>(
PyMemoryView_FromMemory(
Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for working on this.

  • As mentioned in the issue thread, we have similar code in to_py.h, so I wonder about the difference.
  • Second point: that code doesn't do a Python 3 check, so wonder if we need it here?

pytango/ext/to_py.h

Lines 20 to 31 in 19e4c5d

struct DevEncoded_to_tuple
{
static inline PyObject* convert(Tango::DevEncoded const& a)
{
boost::python::str encoded_format(a.encoded_format);
bopy::object encoded_data = bopy::object(
bopy::handle<>(PyBytes_FromStringAndSize(
(const char*)a.encoded_data.get_buffer(),
(Py_ssize_t)a.encoded_data.length())));
boost::python::object result = boost::python::make_tuple(encoded_format, encoded_data);
return boost::python::incref(result.ptr());
}

@sdebionne
Copy link
Contributor

@ajoubertza The latest pipeline was set to build with tango 9.2.5a in .travis.yml. Now the test fail with a link error:

E   ImportError: /home/travis/build/tango-controls/pytango/tango/_tango.so: undefined symbol: _ZN5Tango10DeviceDatarsERPKNS_18DevVarBooleanArrayE

which let me think that the CI either used a cached build of pytango to run the tests (scarry) or that the build and test environments are different. Any clue?

@ajoubertza
Copy link
Member Author

which let me think that the CI either used a cached build of pytango to run the tests (scarry) or that the build and test environments are different. Any clue?

Yes, the builds do use caching, so as not to rebuild the tango wrapper extension unnecessarily. The build looks at the source files, but I guess it doesn't know if the conda environment changed.

You could try modifying the .travis.yml file. Maybe removing the cache: section? Or add a
rm -rf cached_ext just before mkdir -p cached_ext.

Do you really want to test against 9.2.5a?

tiagocoutinho
tiagocoutinho suggested changes Jul 23, 2019
View reviewed changes
@ajoubertza
Merge branch 'develop' into issue-280
275093a
@ajoubertza
Use PyBytes_FromStringAndSize for extracting DevEncoded Py2 and Py3
c131db3 
- No need to differentiate between Python versions.
- Changed namespace from `bopy` -> `boost::python` for
  consistency with surrounding code.

Also restore travis tests to use TANGO 9.3.2
@ajoubertza
Copy link
Member Author

@sdebionne I tried your changes in my environment (Docker container), with TANGO 9.3.2alpha.1 conda package. They worked fine.

Even after changes in c131db3, the extension was not recompiled by Travis. However, I see an option to delete the cache on Travis today (didn't see it yesterday). I deleted the cache for this PR but that didn't help either!

Finally read the Travis docs. Interestingly, if the PR branch doesn't have a cache yet, it takes one from the default branch.

I notice a bug in the .travis.yml file - when checking if the extenstion source files have changed, the diff was not recursive. Trying that next.

@ajoubertza
Use recursive diff to check Travis cached ext files
92dac48 
The `ext` directory has multiple levels, so we need to check all of
them for changes.
@ajoubertza
Copy link
Member Author

That worked. Tests passing at last.

@ajoubertza ajoubertza marked this pull request as ready for review July 23, 2019 21:02
@sdebionne
Copy link
Contributor

I had a day off yesterday. Glad to hear that travis config was buggy and fixed!

Do you really want to test against 9.2.5a?

Nope, it was more a desperate attempt to understand what was wrong with the pipeline and limit the differences between my local build env and the CI. Thanks for reverting this.

Everyting LGTM!

@ajoubertza ajoubertza requested a review from tiagocoutinho July 24, 2019 07:24
tiagocoutinho
tiagocoutinho approved these changes Jul 24, 2019
View reviewed changes
Copy link
Contributor
@tiagocoutinho tiagocoutinho left a comment

Choose a reason for hiding this comment

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

LGTM

@tiagocoutinho tiagocoutinho merged commit 69d95b7 into develop Jul 24, 2019
@tiagocoutinho tiagocoutinho deleted the issue-280 branch July 24, 2019 07:41
@ajoubertza ajoubertza mentioned this pull request Jul 24, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sdebionne sdebionne sdebionne left review comments

@tiagocoutinho tiagocoutinho tiagocoutinho approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ajoubertza @sdebionne @tiagocoutinho
0