[Deprecated PR] Python 3- Step 2: Auto-code migration #574

gpotter2 · 2017-03-22T15:39:14Z

EDIT: That PR was splitted in smaller ones. This will stay open for legacy until all have been merged

Next part of supporting Python 3. This increases the compatibility between python 2 and 3, by using common methods + six.py module. It does not provide a suitable python 3 compatibility yet.

The PR:

used Python-modernize as a base to update scapy's code
manually fixed some conflicts/bugs
Included six.py in modules
fix some small bugs (windows)

codecov-io · 2017-03-22T15:54:48Z

Codecov Report

Merging #574 into master will decrease coverage by 0.15%.
The diff coverage is 63.41%.

@@            Coverage Diff             @@
##           master     #574      +/-   ##
==========================================
- Coverage   77.48%   77.33%   -0.16%     
==========================================
  Files         125      127       +2     
  Lines       30980    31633     +653     
==========================================
+ Hits        24005    24462     +457     
- Misses       6975     7171     +196

Impacted Files	Coverage Δ
scapy/layers/netflow.py	`100% <100%> (ø)`	⬆️
scapy/layers/radius.py	`100% <100%> (ø)`	⬆️
scapy/layers/tftp.py	`36.51% <100%> (+0.42%)`	⬆️
scapy/layers/mobileip.py	`100% <100%> (ø)`	⬆️
scapy/layers/tls/__init__.py	`57.14% <100%> (+7.14%)`	⬆️
scapy/as_resolvers.py	`90.29% <100%> (+0.19%)`	⬆️
scapy/layers/vxlan.py	`88% <100%> (+0.5%)`	⬆️
scapy/arch/common.py	`100% <100%> (ø)`	⬆️
scapy/contrib/eigrp.py	`73.02% <100%> (+0.38%)`	⬆️
scapy/layers/tls/all.py	`100% <100%> (ø)`	⬆️
... and 118 more

p-l-

I have not reviewed in entirely yet, but that's a good start!

p-l- · 2017-03-23T07:12:35Z

scapy/arch/linux.py

@@ -145,7 +147,7 @@ def attach_filter(s, bpf_filter, iface):
    nb = int(lines[0])
    bpf = ""
    for l in lines[1:]:
-        bpf += struct.pack("HBBI",*map(long,l.split()))
+        bpf += struct.pack("HBBI",*list(map(int,l.split())))


You don't need list() here, this should be enough: bpf += struct.pack("HBBI",*map(int, l.split()))

p-l- · 2017-03-23T07:20:06Z

scapy/as_resolvers.py

@@ -88,7 +90,7 @@ def resolve(self, *ips):
        for l in r.splitlines()[1:]:
            if "|" not in l:
                continue
-            asn,ip,desc = map(str.strip, l.split("|"))
+            asn,ip,desc = list(map(str.strip, l.split("|")))


Again, list() is not needed here.

p-l- · 2017-03-23T07:21:36Z

scapy/contrib/gtp.py

-                import gtp_v2
-                return gtp_v2.GTPHeader
-        return GTPHeader
+    @classmethod


Can you remove this windows EOL?

Any ideas how to do it ? I cannot see it using vim/windows and want to avoid having the entire file changed....

dos2unix?

Cannot do it without chaning the hole file... Will do this in a next PR :/ (5 files in total are badly encoded)

Files that have wrong line endings: gtp.py, ppi.py, ppi_cace.py, ppi_geotag.py, rsvp.py
Editing those files often creates messy eol like this one :(

rjarry · 2017-03-23T12:41:50Z

Oh yeah 👍

@gpotter2 what about adding:

from __future__ import unicode_literals

in all files?

This would require using b'byte strings' but this would be cleaner (I think).

rjarry

I did not have time to read all of this but it looks good.

Do you think this would be possible to split this commit a70a803 into several ones?

Maybe at least a separate commit for the print() function.

rjarry · 2017-03-23T12:47:30Z

scapy/base_classes.py

 import re,random,socket
 import types
+from six.moves import map
+from six.moves import range
+from six.moves import zip


Maybe merge the 3 import lines like this?

from six.moves import map, range, zip

rjarry · 2017-03-23T12:49:50Z

scapy/contrib/bgp.py

@@ -27,6 +28,9 @@
 from scapy.layers.inet6 import IP6Field
 from scapy.config import conf, ConfClass
 from scapy.error import log_runtime
+from six.moves import map
+import six
+from six.moves import range


Same here:

from six.moves import map, range

rjarry · 2017-03-23T12:53:06Z

scapy/layers/inet.py

+import six
+from six.moves import map
+from six.moves import range
+from six.moves import zip


from six.moves import map, range, zip

rjarry · 2017-03-23T12:53:39Z

scapy/layers/inet.py

-                                              r.sprintf("%-15s,IP.src% {TCP:%TCP.flags%}{ICMP:%ir,ICMP.type%}")))
+        return self.make_table(lambda s_r: (s_r[0].sprintf("%IP.dst%:{TCP:tcp%ir,TCP.dport%}{UDP:udp%ir,UDP.dport%}{ICMP:ICMP}"),
+                                              s_r[0].ttl,
+                                              s_r[1].sprintf("%-15s,IP.src% {TCP:%TCP.flags%}{ICMP:%ir,ICMP.type%}")))


Fix indentation here.

rjarry · 2017-03-23T13:06:23Z

scapy/layers/inet6.py

-                                              r.sprintf("%-42s,IPv6.src% {TCP:%TCP.flags%}"+
+        return self.make_table(lambda s_r: (s_r[0].sprintf("%-42s,IPv6.dst%:{TCP:tcp%TCP.dport%}{UDP:udp%UDP.dport%}{ICMPv6EchoRequest:IER}"), # TODO: ICMPv6 !
+                                              s_r[0].hlim,
+                                              s_r[1].sprintf("%-42s,IPv6.src% {TCP:%TCP.flags%}"+


Indentation here also :)

rjarry · 2017-03-23T13:09:17Z

scapy/layers/netbios.py

@@ -89,7 +90,7 @@ class NBNSRequest(Packet):
                   IntField("TTL", 0),
                   ShortField("RDLENGTH", 6),
                   BitEnumField("G",0,1,{0:"Unique name",1:"Group name"}),
-                   BitEnumField("OWNER_NODE_TYPE",00,2,{00:"B node",01:"P node",02:"M node",03:"H node"}),
+                   BitEnumField("OWNER_NODE_TYPE",00,2,{00:"B node",0o1:"P node",0o2:"M node",0o3:"H node"}),


I'm not sure I understand this. Are you sure this is octal?

Well no. But written like 01, it's interpreted as an octal...

@gpotter2 that's an error, can you use decimal values here?

rjarry · 2017-03-23T13:09:27Z

scapy/layers/netbios.py

@@ -131,7 +132,7 @@ class NBNSQueryResponseNegative(Packet):
                   IntField("TTL",0),
                   ShortField("RDLENGTH",6),
                   BitEnumField("G",0,1,{0:"Unique name",1:"Group name"}),
-                   BitEnumField("OWNER_NODE_TYPE",00,2,{00:"B node",01:"P node",02:"M node",03:"H node"}),
+                   BitEnumField("OWNER_NODE_TYPE",00,2,{00:"B node",0o1:"P node",0o2:"M node",0o3:"H node"}),


gpotter2 · 2017-03-23T17:17:01Z

@robin-jarry I know the commit is massive. As I said, it partially result from an auto tool, so I won't be able to split it :/

gpotter2 · 2017-03-23T18:24:42Z

@p-l- @robin-jarry

I've added an instruction in the .gitattributes file to auto fix EOL in the future...
Please ignore ed571b7. The entire file format was changed, that's why git is messed up

rjarry · 2017-03-23T18:38:41Z

@gpotter2, I had not seen @guedou comment on another PR which you closed: #567

I completely agree on the "divide and conquer" strategy. This is always better than large patches. It is easier to check for details in small patches, limiting the risks.

On the whole print() function thing, I think I prefer adding:

from __future__ import print_function

everywhere, even if this is quite cumbersome. It has the benefit of completely preventing the use of the old print statement. Also, defining a scapy_print function would require importing it too, so well...

@guedou, what do you think?

To all, what do you think about also adding:

from __future__ import unicode_literals

gpotter2 · 2017-03-23T19:03:57Z

I got a patch for the failing travis test...
Just waiting for #578 before pushing...

@robin-jarry

To all, what do you think about also adding:
from __future__ import unicode_literals

Why not... I don't really understand how useful it is ?

rjarry · 2017-03-23T22:23:58Z

@gpotter2

To all, what do you think about also adding:
from __future__ import unicode_literals

Why not... I don't really understand how useful it is ?

This makes python 2 behave like python 3 regarding string literals.

I found this article inspiring on that matter:

http://python-future.org/unicode_literals.html

gpotter2 · 2017-03-23T22:30:12Z

Thanks @robin-jarry
I'll check this tomorrow.

guedou · 2017-03-24T06:11:31Z

I think that it is too soon to add restrictions with __future__ imports. It might take some until the compatibility is achieved, and I would like that contributors keep writing code without restrictions. I am not convinced that the print one should be enforced on every file.

gpotter2 · 2017-03-25T00:37:54Z

It might take some until the compatibility is achieved, and I would like that contributors keep writing code without restrictions.

That's right. We can always add it later, once the compatibility will be really working.

gpotter2 · 2017-03-25T16:53:09Z

To all:

please ignore codecov this time....
I've already created the next part, based on my previous work:
[Proof of concept] Python 3- Step 3: Main core fixes #580

I'll won't add more fixes to this PR. Waiting for it to be reviewed. Every thing will be in parts 3+

I'm stopping there for now, waiting for all this to be approved + modified.

guedou · 2017-03-27T12:28:51Z

scapy/arch/__init__.py

@@ -16,6 +17,7 @@
 import scapy.config
 from scapy.pton_ntop import inet_pton
 from scapy.data import *
+from scapy.modules.six.moves import map


I think that it will be nice to rewrite the map calls using list comprehensions. I can do that in another PR if you want.

Yeah that would be great. I don't want to make overweight this one...

I am still working on it =/

p-l- · 2017-03-29T09:09:57Z

I agree with @robin-jarry regarding the use of from __future__ import print_function and from __future__ import unicode_literals.

guedou · 2017-03-29T09:46:26Z

@p-l- regarding print_function, it don't like that regular Python 2 users will be forced to use print(). I think that it is not necessary, and its might scare away contributors. That's why, my preference goes do scapy_print().

gpotter2 · 2017-04-17T10:28:01Z

Here is a nice and clean rebased state. (removed all merge commits + fixed conflicts).
Also, i would like to add
from __future__ import unicode_literals
in a future PR.

rjarry · 2017-04-17T10:36:12Z

@guedou, I don’t see why it would bother anyone to put parentheses arround print arguments. And this is the new official python syntax, I think everyone should get used to it.

https://pythonclock.org :-)

Also, using scapy_print() will require importing something anyway. Since there already is a future import for older python versions, why not use it?

guedou · 2017-04-17T11:31:44Z

As Scapy does not use print a lot, I think that there is no point in adding the print import on top of all files. Moreover, it's quite inconvenient for regular Python 2 users/contributors. That's something that we can discuss again when the "compatibility mode" will work fine. Who knows, we might end up supporting 2.7 as long as we did with 2.5 =)

gpotter2 · 2017-04-23T16:56:21Z

To all, @p-l- @guedou
There are not much left to add/correct I guess. If you still got any suggestion please tell me.

A last question: what do we do about print(), as every one does not agree yet on what we should do. I personally prefer using print() as for the same reasons than @rjarry... Which means I think we can leave what's in this PR as such.

Apart from that, it's quite painful to maintain this PR up to date with the rest of scapy, so it would be great if it could be merged or included quite soon :) Thanks !

guedou · 2017-04-23T17:57:19Z

As already suggested, it makes more sense to submit PR targeting single changes (for example, exceptions, range, ...) and update CONTRIBUTING as well for future reference. Such PR are easier to review and merge. Moreover, they tend to provide a cleaner commits history.

gpotter2 · 2017-04-24T16:20:50Z

@guedou You are right, but It's hard to split a PR like this one:

It's based on an automatic tool, which brings tons of fixes: it would be a giant pain to split this in parts :/ more than reviewing it.
I tried to make the commits as explicit as possible, so the reviewing would be easier...
Most of the commits are fixes for the automatic detection: it would not make any sense to split them in separate PRs.
Most of the changes are really easy to understand: reviewing this PR is long, but not hard.
Lastly, i will certainly fix part 3 into smaller parts, but part 2 is really made of basic changes, which (in my opinion) don't need separate PRs.

I know it's a hard work for you guys, and i'm sorry of that...

Apply python-modernize + applied small fix from secdev#571 to fix stderr not being logged

Also force git to correctly manage EOL in the future

b-x · 2017-04-24T17:38:54Z

@gpotter2 python-modernize has a nice -l and -f options, so it's not so hard to split this pull request

gpotter2 · 2017-04-24T17:42:48Z

Please don't make me do every thing again :(

guedou · 2017-04-25T14:48:26Z

@gpotter2 I really understand your frustration, but we need to find a way to merge your patches easily. Unfortunately, you would need to submit smaller PRs to do so, as we previously discussed. I can assure you that I will be able to review and merge the smaller ones quickly.

I consider that this one is a proof of concept that needs to be split for easier review and maintenance. We won't have a time to proof read all of the subsequent changes to make sure that everything is fine. This current PR as already some issues: for example from scapy.modules.six.moves import map is useless in AS_resolvers.py. If you fix it, we will need to read the changes again.

Could you submit the following small PRs?

new exception style
replace long with int
print as a function
filter() replaced with list comprehension

I am still working on rewriting the map() with list comprehension and I am planning to submit a PRs next week.

gpotter2 · 2017-04-25T18:19:52Z

OK.
@guedou I've splitted the thing in 3 PRs (one more is comming with filter() + Zopieux's fixes)

[Python 2 to 3] Exceptions, loops, isinstance and haskey #628
[Python 2 to 3] Remove longs and fix octals #629 (int/longs ==> may neally be merged instantly)
[Python 2 to 3] Main six fixes #630

guedou · 2017-06-16T09:16:28Z

@gpotter2 can we close this PR ?

gpotter2 force-pushed the scpy-30-prog branch from 079e094 to c7c42f1 Compare March 22, 2017 15:56

p-l- requested changes Mar 23, 2017

View reviewed changes

rjarry suggested changes Mar 23, 2017

View reviewed changes

gpotter2 changed the title ~~[convergence] Python 3- Migrate code~~ [convergence] Python 3- Step 2: Auto-code migration Mar 23, 2017

gpotter2 force-pushed the scpy-30-prog branch from ed571b7 to 472d540 Compare March 23, 2017 18:28

gpotter2 force-pushed the scpy-30-prog branch from 472d540 to 3a9d69a Compare March 23, 2017 22:26

gpotter2 force-pushed the scpy-30-prog branch 2 times, most recently from 99a3365 to 768557f Compare March 25, 2017 00:36

gpotter2 force-pushed the scpy-30-prog branch 3 times, most recently from f434855 to 7be0ac1 Compare March 25, 2017 12:31

gpotter2 mentioned this pull request Mar 25, 2017

[Proof of concept] Python 3- Step 3: Main core fixes #580

Closed

guedou reviewed Mar 27, 2017

View reviewed changes

gpotter2 force-pushed the scpy-30-prog branch from 77597cf to 3b7db2e Compare April 17, 2017 10:22

gpotter2 force-pushed the scpy-30-prog branch 8 times, most recently from 5bd1e8f to d8c4ee8 Compare April 22, 2017 21:52

gpotter2 added 9 commits April 24, 2017 18:25

Modernized + small integration fixes

5b01dbe

Apply python-modernize + applied small fix from secdev#571 to fix stderr not being logged

Fix windows tests

5ffa8f8

Fixed a pkcs1 bug (badly replaced int)

1d08c0a

Added doc in CONTRIBUTING

9d3460a

Import fixes + auto EOL

cd4ae38

Also force git to correctly manage EOL in the future

Included six

70fd5be

Fix badly detected octal: replaced by int

1d7e03d

Fix bug in utils/sane + badly replaced int

df114af

Fix imports and some lists usages (thanks @zopieux)

cba3f04

gpotter2 force-pushed the scpy-30-prog branch from d8c4ee8 to cba3f04 Compare April 24, 2017 16:26

gpotter2 mentioned this pull request Apr 25, 2017

[Python 2 to 3] Main six fixes #630

Merged

gpotter2 changed the title ~~[convergence] Python 3- Step 2: Auto-code migration~~ [Deprecated PR] Python 3- Step 2: Auto-code migration May 3, 2017

gpotter2 closed this Jun 16, 2017

gpotter2 deleted the scpy-30-prog branch September 23, 2017 14:56

[Deprecated PR] Python 3- Step 2: Auto-code migration #574

[Deprecated PR] Python 3- Step 2: Auto-code migration #574

Conversation

gpotter2 commented Mar 22, 2017 • edited

codecov-io commented Mar 22, 2017 • edited

Codecov Report

p-l- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjarry commented Mar 23, 2017

rjarry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpotter2 commented Mar 23, 2017

gpotter2 commented Mar 23, 2017

rjarry commented Mar 23, 2017

gpotter2 commented Mar 23, 2017 • edited

rjarry commented Mar 23, 2017

gpotter2 commented Mar 23, 2017 • edited

guedou commented Mar 24, 2017 via email

gpotter2 commented Mar 25, 2017 • edited

gpotter2 commented Mar 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-l- commented Mar 29, 2017

guedou commented Mar 29, 2017

gpotter2 commented Apr 17, 2017 • edited

rjarry commented Apr 17, 2017 • edited

guedou commented Apr 17, 2017 via email

gpotter2 commented Apr 23, 2017 • edited

guedou commented Apr 23, 2017 via email

gpotter2 commented Apr 24, 2017 • edited

b-x commented Apr 24, 2017

gpotter2 commented Apr 24, 2017

guedou commented Apr 25, 2017

gpotter2 commented Apr 25, 2017 • edited

guedou commented Jun 16, 2017

gpotter2 commented Mar 22, 2017 •

edited

codecov-io commented Mar 22, 2017 •

edited

gpotter2 commented Mar 23, 2017 •

edited

gpotter2 commented Mar 23, 2017 •

edited

gpotter2 commented Mar 25, 2017 •

edited

gpotter2 commented Mar 25, 2017 •

edited

gpotter2 commented Apr 17, 2017 •

edited

rjarry commented Apr 17, 2017 •

edited

gpotter2 commented Apr 23, 2017 •

edited

gpotter2 commented Apr 24, 2017 •

edited

gpotter2 commented Apr 25, 2017 •

edited