[riot-notifications] [RIOT-OS/RIOT] tools/compile_and_test_for_board.py: allow use of wildcards for applications selection (#10838)

Gaëtan Harter notifications at github.com
Wed Jan 23 14:22:28 CET 2019


cladmi requested changes on this pull request.

I do like allowing wildcards in `--applications` or `--applications-exclude` but this completely changes the behavior. It should for me just result in replacing `list_of_dirs` to `glob of the items in the list` with base directory handing and all the things.

Having the implementation here, should not prevent from providing the implementation in the RIOT build system directly in https://github.com/RIOT-OS/RIOT/blob/7e77ac7ace75ca9643250e0c747a5e65074d3fef/makefiles/app_dirs.inc.mk#L8

A improvement for the future would be to even move the `applications`/`applications-exclude` handling in makefiles/app_dirs.inc.mk.

>  
     # Remove applications to skip
     apps_dirs = set(apps_dirs) - set(apps_dirs_skip)
 
     return sorted(list(apps_dirs))
 
 
+def _matching_apps_directories(apps_dirs, riotdir, exclude=False):

I would not put all this in a sub-function. Just convert `apps_dirs` to `apps_dirs with wildcard resolution`
This function is used with two modes which increases complexity

>  
     # Remove applications to skip
     apps_dirs = set(apps_dirs) - set(apps_dirs_skip)
 
     return sorted(list(apps_dirs))
 
 
+def _matching_apps_directories(apps_dirs, riotdir, exclude=False):
+    """Return a list of applications matching the given pattern."""
+    ret = []
+    # No specific applications directories specified, return the default
+    # list:
+    #  - empty list if this is related to excluded applications
+    #  - 'tracked' application if this is related to included applications
+    if apps_dirs is None:
+        return ret if exclude else _riot_tracked_applications_dirs(riotdir)
+    # All applications requested
+    if '*' in apps_dirs:

I do not like this pattern here.

It does not make sense as an augmentation mechanism, because if you put `['a', 'b']` you get `['a', 'b']` and if you put `['a', 'b', '*']` you get the riot directories.

I want more `applications` to be a "test this that and here" only. If you want all RIOT + something, I would prefer addressing it in the RIOT side.

>  
     # Remove applications to skip
     apps_dirs = set(apps_dirs) - set(apps_dirs_skip)
 
     return sorted(list(apps_dirs))
 
 
+def _matching_apps_directories(apps_dirs, riotdir, exclude=False):

It completely changes the behavior of the previous implementation.

nothing == RIOT directories
applications == use these ones, not considering whit RIOT is giving you so can force non tracked apps.
Exclude: exclusion that are applied after to blacklist only one test.

>  
     # Remove applications to skip
     apps_dirs = set(apps_dirs) - set(apps_dirs_skip)
 
     return sorted(list(apps_dirs))
 
 
+def _matching_apps_directories(apps_dirs, riotdir, exclude=False):
+    """Return a list of applications matching the given pattern."""
+    ret = []
+    # No specific applications directories specified, return the default
+    # list:
+    #  - empty list if this is related to excluded applications
+    #  - 'tracked' application if this is related to included applications
+    if apps_dirs is None:
+        return ret if exclude else _riot_tracked_applications_dirs(riotdir)
+    # All applications requested
+    if '*' in apps_dirs:
+        return _riot_tracked_applications_dirs(riotdir)
+
+    # Now search for matching applications in available RIOT applications
+    _riot_applications = _riot_applications_dirs(riotdir)

This one should not be used for `app_dirs` as it is the goal that it can be a non standard application directory.

> @@ -52,9 +52,11 @@
   --applications APPLICATIONS
                         List of applications to test, overwrites default
                         configuration of testing all applications
+                        Applications can contain wildcards.

It should also say that the wildcard will be evaluated as relative to the RIOT directory somehow.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/10838#pullrequestreview-195508944
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190123/3a85546b/attachment-0001.html>


More information about the notifications mailing list