Re: [dts] [PATCH v2] python: standard project structure

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

> -----Original Message-----
> From: Tu, Lijuan <lijuan.tu@intel.com>
> Sent: Monday, September 6, 2021 5:01 AM
> To: Juraj Linkeš <juraj.linkes@pantheon.tech>; ohilyard@iol.unh.edu
> Cc: dts@dpdk.org
> Subject: RE: [PATCH v2] python: standard project structure
> 
> > -----Original Message-----
> > From: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Sent: 2021年8月27日 21:04
> > To: Tu, Lijuan <lijuan.tu@intel.com>; ohilyard@iol.unh.edu
> > Cc: dts@dpdk.org; Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Subject: [PATCH v2] python: standard project structure
> >
> > DTS does not use the standard project structure and has therefore add
> > paths to python interpreter search paths. Move to a standard structure:
> > * Move main.py to the root directory.
> > * Add __init__.py to directories from which python modules are imported.
> > * Adjust import paths to account for this new structure.
> >
> > Moving to a standard structure has a host of positives, such as:
> > * No need to study any non-standard approaches. This removes any
> > bewilderment the developers may feel when encountering something
> > non-standard without proper justification (as is the case with DTS).
> > * Better integration with IDEs which rely on the standard structure.
> > * More accurate results from automated tools.
> >
> > In addition to this, not only adjust the import paths but make then
> > explicit for modules imported from the same level, e.g. instead of
> > using from foo import, use from .foo import (and import bar.foo as
> > foo, where foo if on the same level as the importing module). This
> > allows developers to separate DTS modules from third party modules at a
> glance.
> >
> > Also sort the import using the isort tool. Add config using the "black"
> > profile for isort to facilitate interoperability with Black.
> >
> > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > ---
> > v2:
> > * Reintroduced the dts shell cript.
> > * Tested the imports by trying to import everything and fixed what I
> > * found.
> > * Sorted the imports using isort with the black profile.
> > ---
> 
> * the import of Dot1BR  is broken.

I don't understand how this works exactly. Are there two distinct tools that are trying to use dep/scapy_modules/Dot1BR.py?

> 	Dot1BR  is aimed to import in scapy application in Tester.

Is this something that runs outside of DTS? Doesn't DTS import Scapy libs and send packets using those libs? What exactly is the scapy application and how it's supposed to used dep/scapy_modules/Dot1BR.py?

> 	The patch just import it in the script, but not in the scapy.

What script are you talking about here? Maybe diagram would be helpful - what runs where and what is supposed to import what.

> 	So that when sending packet by scapy, it can't recognize Dot1BR
> packet.
> 
> * Suite directory is still imported, which should by remove.
>    - File: framework/dts.py  line: 534
> 	# add python module search path
>     	sys.path.append(suite_dir)
> 

I'll remove this, but that will make the  "--suite-dir" argument obsolete. We should think about how to deprecate it - if we decide to do that, it should be in another patch though. I'd guess it would a very low priority.

> * Missed some dynamic import in the function:
>    - File: framework/dts.py
> 	line: 250
> 		project_module = __import__(PROJECT_MODULE_PREFIX +
> project_name)
> 	line: 454
> 		suite_module = __import__('TestSuite_' + suite_name)
>    - File: framework/test_case.py		line: 380
> 	debugger.AliveModule = __import__('TestSuite_' + self.suite_name)
>    - File:	framework/tester.py	line: 740, 846, 855
> 	module = __import__("packet")
>    - File: framework/flow/flow_pattern_item.py 	line: 92
> 	from flow import Flow
> 

I also found some other instances of __import__ being called (in tester.py and dump_case.py), I'll fix all of them.

We may want to replace __import__() with importlib.import_module(), as the docs say that direct use of __import__() is discouraged (in favor of importlib.import_module()). This would also be a low priority item I think.