Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python3 support #33

Merged
merged 1 commit into from
Mar 6, 2020
Merged

Python3 support #33

merged 1 commit into from
Mar 6, 2020

Conversation

mmisono
Copy link

@mmisono mmisono commented Feb 12, 2020

This PR adds python3 support to ubpf-(dis)assembler.py. nosetest gives the same result with both python2 (2.7.13) and python3 (3.6.9) on my environment (Ran 858 tests, OK (SKIP=475)).

Signed-off-by: Masanori Misono [email protected]

@coveralls
Copy link

coveralls commented Feb 12, 2020

Coverage Status

Coverage decreased (-0.1%) to 96.561% when pulling 31af4ae on mmisono:python3 into 9d10569 on iovisor:master.

Copy link
Collaborator

@pchaigno pchaigno left a comment

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 and sorry for the late review.

I left a few comments below. My main concern is the new six dependency. I would prefer to avoid adding it as a dependency if the only use is detecting Python2 vs. Python3. In a lot of cases, there are solutions that are compatible with both versions (see https://python-future.org/compatible_idioms.html for some examples).

Could you also update the Travis CI's matrix to add Python3 tests?

requirements.txt Show resolved Hide resolved
bin/ubpf-assembler Outdated Show resolved Hide resolved
test_framework/test_assembler.py Outdated Show resolved Hide resolved
test_framework/test_disassembler.py Outdated Show resolved Hide resolved
ubpf/assembler.py Show resolved Hide resolved
test_framework/test_elf.py Outdated Show resolved Hide resolved
@mmisono
Copy link
Author

mmisono commented Feb 28, 2020

@pchaigno
Thanks for the review. Reexamined the code and I removed the six dependency. Also added the python3 test in CI. Please take a look when you have time. (I'll squash the commits once the review is completed.)

.travis.yml Show resolved Hide resolved
Copy link
Collaborator

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Thanks a lot @mmisono! This is looking good :-)

I just have two minor comments left and I think we'll be good to merge.

.travis.yml Outdated
include:
- name: python 2.7
os: linux
dist: xenial
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move these configurations out of the build matrix to avoid duplicating it?

.travis.yml Outdated
- pip install --user -r requirements.txt
- pip install --user cpp-coveralls
- python$PYTHON -m pip install --user -r requirements.txt
- python$PYTHON -m pip install --user cpp-coveralls
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe defining PYTHON as python{2,3} and then using $PYTHON -m pip ... would be a bit cleaner?

Add python3 support to ubpf-(dis)assembler.py
Also add python3 test in CI.

Signed-off-by: Masanori Misono <[email protected]>
@mmisono
Copy link
Author

mmisono commented Mar 3, 2020

@pchaigno
Updated the travis.yaml and squashed the commits. I think now it is ready to be merged.

Copy link
Collaborator

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Thanks a lot @mmisono!

@pchaigno pchaigno merged commit 4cbf799 into iovisor:master Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants