Fix hardcoded config folders

Message ID 20220111155937.36524-1-ohilyard@iol.unh.edu (mailing list archive)
State Superseded
Headers
Series Fix hardcoded config folders |

Checks

Context Check Description
ci/Intel-dts-suite-test fail Testing issues

Commit Message

Owen Hilyard Jan. 11, 2022, 3:59 p.m. UTC
  From: Owen Hilyard <ohilyard@iol.unh.edu>

All hardcoded instances of "DTS_ROOT/conf" or "./conf" have been replaced with
CONFIG_ROOT_PATH, which is either the environment variable
DTS_CFG_FOLDER if it is set, or is DTS_ROOT/conf/ by default.

Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
---
 framework/checkCase.py                              | 10 ++++------
 framework/project_dpdk.py                           |  4 ++--
 tests/TestSuite_iavf_package_driver_error_handle.py |  3 ++-
 tests/TestSuite_ipsec_gw_cryptodev_func.py          |  4 +++-
 tests/TestSuite_vhost_user_live_migration.py        |  5 +++--
 tests/vhost_peer_conf.py                            |  4 +++-
 tools/setup.py                                      | 10 ++++++----
 7 files changed, 23 insertions(+), 17 deletions(-)
  

Comments

Juraj Linkeš Jan. 17, 2022, 12:11 p.m. UTC | #1
> -----Original Message-----
> From: ohilyard@iol.unh.edu <ohilyard@iol.unh.edu>
> Sent: Tuesday, January 11, 2022 5:00 PM
> To: dts@dpdk.org
> Cc: lijuan.tu@intel.com; Owen Hilyard <ohilyard@iol.unh.edu>
> Subject: [PATCH] Fix hardcoded config folders
> 
> From: Owen Hilyard <ohilyard@iol.unh.edu>
> 
> All hardcoded instances of "DTS_ROOT/conf" or "./conf" have been replaced
> with CONFIG_ROOT_PATH, which is either the environment variable
> DTS_CFG_FOLDER if it is set, or is DTS_ROOT/conf/ by default.
> 
> Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
> ---
>  framework/checkCase.py                              | 10 ++++------
>  framework/project_dpdk.py                           |  4 ++--
>  tests/TestSuite_iavf_package_driver_error_handle.py |  3 ++-
>  tests/TestSuite_ipsec_gw_cryptodev_func.py          |  4 +++-
>  tests/TestSuite_vhost_user_live_migration.py        |  5 +++--
>  tests/vhost_peer_conf.py                            |  4 +++-
>  tools/setup.py                                      | 10 ++++++----
>  7 files changed, 23 insertions(+), 17 deletions(-)
> 

I had a quick glance at this and it seems to me that we should change the FOLDERS settings variable as well. Maybe in another patch, since it contains more than just the conf path?

Should DTS_CONFIG_PATHS in ci/get_tests_for_patchset.py also use CONFIG_ROOT_PATH?

Do we want to address comments as well? E.g. the docstring of load_global_config in dts/virt_base.py.

The same question goes for docs, there are a few instances of conf/<cfgfile>.cfg.

> diff --git a/framework/checkCase.py b/framework/checkCase.py index
> 3182dc77..013b9551 100644
> --- a/framework/checkCase.py
> +++ b/framework/checkCase.py
> @@ -1,14 +1,12 @@
>  import collections
>  import json
> +import os
> 
> -import xlrd
> -
> -from .settings import HOST_DRIVER_SETTING, get_nic_name,
> load_global_setting
> +from .settings import HOST_DRIVER_SETTING, get_nic_name,
> +load_global_setting, CONFIG_ROOT_PATH
>  from .utils import RED
> 
> -filter_json_file = './conf/test_case_checklist.json'
> -support_json_file = './conf/test_case_supportlist.json'
> -
> +filter_json_file = os.path.join(CONFIG_ROOT_PATH,
> +'test_case_checklist.json') support_json_file =
> +os.path.join(CONFIG_ROOT_PATH, 'test_case_supportlist.json')
> 
>  class CheckCase(object):
>      """
> diff --git a/framework/project_dpdk.py b/framework/project_dpdk.py index
> 9927bcc1..05462b6d 100644
> --- a/framework/project_dpdk.py
> +++ b/framework/project_dpdk.py
> @@ -46,7 +46,7 @@ from .settings import (
>      NICS,
>      accepted_nic,
>      load_global_setting,
> -    save_global_setting,
> +    save_global_setting, CONFIG_ROOT_PATH,
>  )
>  from .ssh_connection import SSHConnection  from .tester import Tester @@ -
> 571,7 +571,7 @@ class DPDKdut(Dut):
>          folder_info = folder.split('/')
>          name = folder_info[-1]
>          if name != 'examples' and name not in self.apps_name:
> -            raise Exception('Please config %s file path on conf/app_name.cfg' %
> name)
> +            raise Exception(f'Please config {name} file path on
> + {os.path.join(CONFIG_ROOT_PATH, "app_name.cfg")}')
> 
>          if name == 'examples':
>              example = 'all'
> diff --git a/tests/TestSuite_iavf_package_driver_error_handle.py
> b/tests/TestSuite_iavf_package_driver_error_handle.py
> index 0cdba385..c4277fc7 100644
> --- a/tests/TestSuite_iavf_package_driver_error_handle.py
> +++ b/tests/TestSuite_iavf_package_driver_error_handle.py
> @@ -35,6 +35,7 @@ import time
> 
>  from framework.config import UserConf
>  from framework.pmd_output import PmdOutput
> +from framework.settings import CONFIG_ROOT_PATH
>  from framework.test_case import TestCase
> 
> 
> @@ -47,7 +48,7 @@ class Testiavf_package_and_driver_check(TestCase):
>          self.verify(len(self.dut_ports) >= 1, "Insufficient ports")
>          self.PF_QUEUE = 16
> 
> -        conf_file = 'conf/iavf_driver_package.cfg'
> +        conf_file = os.path.join(CONFIG_ROOT_PATH,
> + 'iavf_driver_package.cfg')
>          conf_peer = UserConf(conf_file)
>          conf_session = conf_peer.conf._sections['suite']
>          self.driverPath_latest = conf_session['ice_driver_file_location_latest']
> diff --git a/tests/TestSuite_ipsec_gw_cryptodev_func.py
> b/tests/TestSuite_ipsec_gw_cryptodev_func.py
> index a5d6e775..57ca79d9 100644
> --- a/tests/TestSuite_ipsec_gw_cryptodev_func.py
> +++ b/tests/TestSuite_ipsec_gw_cryptodev_func.py
> @@ -30,11 +30,13 @@
>  # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> 
>  import binascii
> +import os.path
>  import time
> 
>  import framework.packet as packet
>  import framework.utils as utils
>  import tests.cryptodev_common as cc
> +from framework.settings import CONFIG_ROOT_PATH
>  from framework.test_case import TestCase
> 
> 
> @@ -87,7 +89,7 @@ class TestIPsecGW(TestCase):
>              "u": "0x1"
>          }
> 
> -        conf_file = r'conf/ipsec_ep0.cfg'
> +        conf_file = os.path.join(CONFIG_ROOT_PATH, 'ipsec_ep0.cfg')
>          self.dut.session.copy_file_to(conf_file, '/tmp')
> 
>      def set_up(self):
> diff --git a/tests/TestSuite_vhost_user_live_migration.py
> b/tests/TestSuite_vhost_user_live_migration.py
> index e50800d5..8796f16a 100644
> --- a/tests/TestSuite_vhost_user_live_migration.py
> +++ b/tests/TestSuite_vhost_user_live_migration.py
> @@ -28,13 +28,14 @@
>  # THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
> USE  # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> DAMAGE.
> -
> +import os.path
>  import re
>  import time
> 
>  import framework.utils as utils
>  from framework.config import UserConf
>  from framework.exception import VirtDutInitException
> +from framework.settings import CONFIG_ROOT_PATH
>  from framework.test_case import TestCase  from framework.virt_common
> import VM
> 
> @@ -54,7 +55,7 @@ class TestVhostUserLiveMigration(TestCase):
>                          "Insufficient ports for testing")
> 
>          # get mount info from cfg file
> -        conf_info = UserConf('conf/%s.cfg' % self.suite_name)
> +        conf_info = UserConf(os.path.join(CONFIG_ROOT_PATH,
> + f'{self.suite_name}.cfg'))
>          conf_session = conf_info.conf._sections['mount_info']
>          self.mount_path = conf_session['backup_mount_path']
>          self.share_path = conf_session['host_share_dir'] diff --git
> a/tests/vhost_peer_conf.py b/tests/vhost_peer_conf.py index
> c8802277..4dc47341 100644
> --- a/tests/vhost_peer_conf.py
> +++ b/tests/vhost_peer_conf.py
> @@ -28,10 +28,12 @@
>  # THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
> USE  # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> DAMAGE.
> +import os
> 
>  from framework.config import UserConf
> +from framework.settings import CONFIG_ROOT_PATH
> 
> -conf_file = 'conf/vhost_peer_conf.cfg'
> +conf_file = os.path.join(CONFIG_ROOT_PATH, 'vhost_peer_conf.cfg')
>  conf_peer = UserConf(conf_file)
>  conf_session = conf_peer.conf._sections['peerconf']
> 
> diff --git a/tools/setup.py b/tools/setup.py index c70730c7..f4cc5d74 100755
> --- a/tools/setup.py
> +++ b/tools/setup.py
> @@ -20,10 +20,12 @@ DTS_TOOLS = DTS_PATH + '/tools'
>  DTS_SUITES = DTS_PATH + '/tests'
>  DTS_EXECS = DTS_PATH + '/executions'
> 
> -DTS_EXEC_CFG = DTS_PATH + '/execution.cfg'
> -DTS_CRBS_CFG = DTS_PATH + '/conf/crbs.cfg'
> -DTS_PORTS_CFG = DTS_PATH + '/conf/ports.cfg'
> -DTS_IXIA_CFG = DTS_PATH + '/conf/ixia.cfg'
> +CONFIG_ROOT_PATH = os.environ.get("DTS_CFG_FOLDER") or
> +os.path.join(DTS_PATH, "conf/")
> +
> +DTS_EXEC_CFG = os.path.join(DTS_PATH, 'execution.cfg') DTS_CRBS_CFG =
> +os.path.join(CONFIG_ROOT_PATH, 'crbs.cfg') DTS_PORTS_CFG =
> +os.path.join(CONFIG_ROOT_PATH, 'ports.cfg') DTS_IXIA_CFG =
> +os.path.join(CONFIG_ROOT_PATH, 'ixia.cfg')
> 
>  sys.path.append(DTS_FRAMEWORK)
>  sys.path.append(DTS_TOOLS)
> --
> 2.30.2
>
  
Owen Hilyard Jan. 18, 2022, 3:21 p.m. UTC | #2
From what I can tell, FOLDERS is where the defaults are derived from in a
lot of places, so I want to leave that alone.

I can't actually find get_tests_for_patchset.py anywhere in the DTS repos,
which leads me to believe that it was never actually applied to DTS.

As for comments/documentation, I'll submit a V2 fixing as much of that as I
can find.
  

Patch

diff --git a/framework/checkCase.py b/framework/checkCase.py
index 3182dc77..013b9551 100644
--- a/framework/checkCase.py
+++ b/framework/checkCase.py
@@ -1,14 +1,12 @@ 
 import collections
 import json
+import os
 
-import xlrd
-
-from .settings import HOST_DRIVER_SETTING, get_nic_name, load_global_setting
+from .settings import HOST_DRIVER_SETTING, get_nic_name, load_global_setting, CONFIG_ROOT_PATH
 from .utils import RED
 
-filter_json_file = './conf/test_case_checklist.json'
-support_json_file = './conf/test_case_supportlist.json'
-
+filter_json_file = os.path.join(CONFIG_ROOT_PATH, 'test_case_checklist.json')
+support_json_file = os.path.join(CONFIG_ROOT_PATH, 'test_case_supportlist.json')
 
 class CheckCase(object):
     """
diff --git a/framework/project_dpdk.py b/framework/project_dpdk.py
index 9927bcc1..05462b6d 100644
--- a/framework/project_dpdk.py
+++ b/framework/project_dpdk.py
@@ -46,7 +46,7 @@  from .settings import (
     NICS,
     accepted_nic,
     load_global_setting,
-    save_global_setting,
+    save_global_setting, CONFIG_ROOT_PATH,
 )
 from .ssh_connection import SSHConnection
 from .tester import Tester
@@ -571,7 +571,7 @@  class DPDKdut(Dut):
         folder_info = folder.split('/')
         name = folder_info[-1]
         if name != 'examples' and name not in self.apps_name:
-            raise Exception('Please config %s file path on conf/app_name.cfg' % name)
+            raise Exception(f'Please config {name} file path on {os.path.join(CONFIG_ROOT_PATH, "app_name.cfg")}')
 
         if name == 'examples':
             example = 'all'
diff --git a/tests/TestSuite_iavf_package_driver_error_handle.py b/tests/TestSuite_iavf_package_driver_error_handle.py
index 0cdba385..c4277fc7 100644
--- a/tests/TestSuite_iavf_package_driver_error_handle.py
+++ b/tests/TestSuite_iavf_package_driver_error_handle.py
@@ -35,6 +35,7 @@  import time
 
 from framework.config import UserConf
 from framework.pmd_output import PmdOutput
+from framework.settings import CONFIG_ROOT_PATH
 from framework.test_case import TestCase
 
 
@@ -47,7 +48,7 @@  class Testiavf_package_and_driver_check(TestCase):
         self.verify(len(self.dut_ports) >= 1, "Insufficient ports")
         self.PF_QUEUE = 16
 
-        conf_file = 'conf/iavf_driver_package.cfg'
+        conf_file = os.path.join(CONFIG_ROOT_PATH, 'iavf_driver_package.cfg')
         conf_peer = UserConf(conf_file)
         conf_session = conf_peer.conf._sections['suite']
         self.driverPath_latest = conf_session['ice_driver_file_location_latest']
diff --git a/tests/TestSuite_ipsec_gw_cryptodev_func.py b/tests/TestSuite_ipsec_gw_cryptodev_func.py
index a5d6e775..57ca79d9 100644
--- a/tests/TestSuite_ipsec_gw_cryptodev_func.py
+++ b/tests/TestSuite_ipsec_gw_cryptodev_func.py
@@ -30,11 +30,13 @@ 
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 import binascii
+import os.path
 import time
 
 import framework.packet as packet
 import framework.utils as utils
 import tests.cryptodev_common as cc
+from framework.settings import CONFIG_ROOT_PATH
 from framework.test_case import TestCase
 
 
@@ -87,7 +89,7 @@  class TestIPsecGW(TestCase):
             "u": "0x1"
         }
 
-        conf_file = r'conf/ipsec_ep0.cfg'
+        conf_file = os.path.join(CONFIG_ROOT_PATH, 'ipsec_ep0.cfg')
         self.dut.session.copy_file_to(conf_file, '/tmp')
 
     def set_up(self):
diff --git a/tests/TestSuite_vhost_user_live_migration.py b/tests/TestSuite_vhost_user_live_migration.py
index e50800d5..8796f16a 100644
--- a/tests/TestSuite_vhost_user_live_migration.py
+++ b/tests/TestSuite_vhost_user_live_migration.py
@@ -28,13 +28,14 @@ 
 # THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-
+import os.path
 import re
 import time
 
 import framework.utils as utils
 from framework.config import UserConf
 from framework.exception import VirtDutInitException
+from framework.settings import CONFIG_ROOT_PATH
 from framework.test_case import TestCase
 from framework.virt_common import VM
 
@@ -54,7 +55,7 @@  class TestVhostUserLiveMigration(TestCase):
                         "Insufficient ports for testing")
 
         # get mount info from cfg file
-        conf_info = UserConf('conf/%s.cfg' % self.suite_name)
+        conf_info = UserConf(os.path.join(CONFIG_ROOT_PATH, f'{self.suite_name}.cfg'))
         conf_session = conf_info.conf._sections['mount_info']
         self.mount_path = conf_session['backup_mount_path']
         self.share_path = conf_session['host_share_dir']
diff --git a/tests/vhost_peer_conf.py b/tests/vhost_peer_conf.py
index c8802277..4dc47341 100644
--- a/tests/vhost_peer_conf.py
+++ b/tests/vhost_peer_conf.py
@@ -28,10 +28,12 @@ 
 # THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+import os
 
 from framework.config import UserConf
+from framework.settings import CONFIG_ROOT_PATH
 
-conf_file = 'conf/vhost_peer_conf.cfg'
+conf_file = os.path.join(CONFIG_ROOT_PATH, 'vhost_peer_conf.cfg')
 conf_peer = UserConf(conf_file)
 conf_session = conf_peer.conf._sections['peerconf']
 
diff --git a/tools/setup.py b/tools/setup.py
index c70730c7..f4cc5d74 100755
--- a/tools/setup.py
+++ b/tools/setup.py
@@ -20,10 +20,12 @@  DTS_TOOLS = DTS_PATH + '/tools'
 DTS_SUITES = DTS_PATH + '/tests'
 DTS_EXECS = DTS_PATH + '/executions'
 
-DTS_EXEC_CFG = DTS_PATH + '/execution.cfg'
-DTS_CRBS_CFG = DTS_PATH + '/conf/crbs.cfg'
-DTS_PORTS_CFG = DTS_PATH + '/conf/ports.cfg'
-DTS_IXIA_CFG = DTS_PATH + '/conf/ixia.cfg'
+CONFIG_ROOT_PATH = os.environ.get("DTS_CFG_FOLDER") or os.path.join(DTS_PATH, "conf/")
+
+DTS_EXEC_CFG = os.path.join(DTS_PATH, 'execution.cfg')
+DTS_CRBS_CFG = os.path.join(CONFIG_ROOT_PATH, 'crbs.cfg')
+DTS_PORTS_CFG = os.path.join(CONFIG_ROOT_PATH, 'ports.cfg')
+DTS_IXIA_CFG = os.path.join(CONFIG_ROOT_PATH, 'ixia.cfg')
 
 sys.path.append(DTS_FRAMEWORK)
 sys.path.append(DTS_TOOLS)