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

utils/ordered_dict: don't use compat code for Python 3.7+ #708

Merged
merged 1 commit into from
Nov 3, 2020

Conversation

ueno
Copy link
Collaborator

@ueno ueno commented Oct 13, 2020

Description

This makes tlsfuzzer.utils.ordered_dict.OrderedDict return dict on Python 3.7, as suggested in #706.

Motivation and Context

Fixes #706.

Checklist

  • I have read the CONTRIBUTING.md document and my PR follows change requirements therein
  • the changes are also reflected in documentation and code comments
  • all new and existing tests pass (see Travis CI results)
  • test script checklist was followed for new scripts
  • new test script added to tlslite-ng.json and tlslite-ng-random-subset.json
  • new and modified scripts were ran against popular TLS implementations:
    • OpenSSL
    • NSS
    • GnuTLS
  • required version of tlslite-ng updated in requirements.txt and README.md

This change is Reviewable

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ueno)


tlsfuzzer/utils/ordered_dict.py, line 10 at r1 (raw file):

# Dict keeps insertion order in Python 3.7+:
# https://mail.python.org/pipermail/python-dev/2017-December/151283.html
if (sys.version_info.major, sys.version_info.minor) > (3, 6):

no need to remake the tuple, (3, 6, 7) > (3, 6) == True and (3, 5, 99) < (3, 6) == True
so it can be just sys.version_info > (3, 6)

Copy link
Collaborator Author

@ueno ueno left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @tomato42)


tlsfuzzer/utils/ordered_dict.py, line 10 at r1 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

no need to remake the tuple, (3, 6, 7) > (3, 6) == True and (3, 5, 99) < (3, 6) == True
so it can be just sys.version_info > (3, 6)

That's good; fixed.

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ueno)

a discussion (no related file):
looks like test cases will need slight adjustments for CI to pass


@ueno
Copy link
Collaborator Author

ueno commented Oct 14, 2020

a discussion (no related file):

Previously, tomato42 (Hubert Kario) wrote…

looks like test cases will need slight adjustments for CI to pass

What do you expect to fix them: add compatibility with the legacy OrderdDict methods, or align to the dict (and remove test cases for unused OrderdDict methods)?


Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ueno)

a discussion (no related file):

Previously, ueno (Daiki Ueno) wrote…

What do you expect to fix them: add compatibility with the legacy OrderdDict methods, or align to the dict (and remove test cases for unused OrderdDict methods)?

since the tests check if the code behaves as we need it to, I'd say that we should try to retain as much of them as possible
that being said, methods like itervalues and iterkeys are unused in tlsfuzzer code, so we can just skip those
fixing pop() and popitem() test case should be a simple case of not using a name for the argument


@tomato42 tomato42 added the maintenance issues related to project maintenance, CI, documentation, etc. label Oct 14, 2020
@tomato42
Copy link
Member

remember to rebase on top of master, there was a slight issue with py2.6 I had to fix to make the CI functional again

tomato42
tomato42 previously approved these changes Nov 2, 2020
Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Collaborator Author

@ueno ueno left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @tomato42)

a discussion (no related file):

Previously, tomato42 (Hubert Kario) wrote…

since the tests check if the code behaves as we need it to, I'd say that we should try to retain as much of them as possible
that being said, methods like itervalues and iterkeys are unused in tlsfuzzer code, so we can just skip those
fixing pop() and popitem() test case should be a simple case of not using a name for the argument

Sorry, I forgot the fact that OrderedDict is a subclass of dict and isinstance(d, dict) cannot be used as a check. I've rewritten it with type(d) is dict, which seems to work. Please check again.



tlsfuzzer/utils/ordered_dict.py, line 10 at r1 (raw file):

Previously, ueno (Daiki Ueno) wrote…

That's good; fixed.

Done.

@tomato42
Copy link
Member

tomato42 commented Nov 3, 2020

test-tls13-shuffled-extentions.py:stderr:Traceback (most recent call last):
test-tls13-shuffled-extentions.py:stderr:  File "scripts/test-tls13-shuffled-extentions.py", line 341, in <module>
test-tls13-shuffled-extentions.py:stderr:    main()
test-tls13-shuffled-extentions.py:stderr:  File "scripts/test-tls13-shuffled-extentions.py", line 180, in main
test-tls13-shuffled-extentions.py:stderr:    rev_ext.update(reversed(ext.items()))
test-tls13-shuffled-extentions.py:stderr:TypeError: 'dict_items' object is not reversible

Looks like we can't use the 3.7 implementation of the dict()

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@tomato42 tomato42 merged commit c306d68 into tlsfuzzer:master Nov 3, 2020
@tomato42
Copy link
Member

tomato42 commented Nov 3, 2020

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance issues related to project maintenance, CI, documentation, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tlsfuzzer/utils/ordered_dict.py depends on deprecated _dummy_thread module
2 participants