Skip to content

Conversation

@nick-falco
Copy link
Collaborator

@nick-falco nick-falco commented Jan 30, 2020

What does this PR do?

Fix for issue #296 - Updates construct package version to 2.9.45. The previous version was 2.5.1 and was extremely out of date.

Relevant Issues?

#296

Checklist

  • This PR is not directly related to an existing issue (which has no PR yet).
  • All tests pass.
  • Any new or changed features have are documented.
  • Changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .

Copy link
Contributor

@dsentinel dsentinel left a comment

Choose a reason for hiding this comment

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

Awesome thanks!

- pip=19.1.1
- pip:
- construct==2.5.1
- construct==2.9.45
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't pin this if we don't have to.
Will this not work with 2.10.56 -- the latest?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The PH5 web services are currently using 2.9.45, as I believe that was the latest until recently, so I want to make them match. I haven't looked at 2.10.56 yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh sure, I'm saying if it works without the pin leave it out and let the latest or what ever else may need get used.
If it's not backwards compatible add a minimum version. Ideally best to keep them as general as possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like version 2.10 drops support for python 2.7

https://construct.readthedocs.io/en/latest/transition210.html

We'll want to pin it to the latest version < 2.10

@nick-falco
Copy link
Collaborator Author

This is proving to be more difficult than I hoped. All construct.BitField values need to be replaced by construct.BitsInteger as indicated by the construct 2.8 release notes.

Unfortunately this BitsInteger type does not handle endianness in the same way as the retired BitField type. As the documentation notes, little-endianness is only defined for multiples of 8 bits, so some of the fields don't transfer easily to the BitsInteger type.

For example see the following:
https://github.com/PIC-IRIS/PH5/blob/master/ph5/core/segd_h.py#L215-L220
These fields are not a multiple of 8 bits.

Not to sure how we should proceed. We are going to definitely need to be using then new library for when we update PH5 to Python 3.

@dsentinel Any thoughts on this?

@dsentinel
Copy link
Contributor

Bummer, let me have a look...

This is proving to be more difficult than I hoped. All construct.BitField values need to be replaced by construct.BitsInteger as indicated by the construct 2.8 release notes.

Unfortunately this BitsInteger type does not handle endianness in the same way as the retired BitField type. As the documentation notes, little-endianness is only defined for multiples of 8 bits, so some of the fields don't transfer easily to the BitsInteger type.

I think we're ok. Endianness has no meaning for fields <=8 bits long. I think the construct document is stating this in a strange way.
The old swapped=swap arguments for fields <=8 bits long had no effect, and could have been left off.

For example see the following:
https://github.com/PIC-IRIS/PH5/blob/master/ph5/core/segd_h.py#L215-L220
These fields are not a multiple of 8 bits.

Not to sure how we should proceed. We are going to definitely need to be using then new library for when we update PH5 to Python 3.

@dsentinel Any thoughts on this?

@timronan
Copy link
Collaborator

Where are we at with this PR? Should it be abandoned or does it need to be completed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants