project: Restricting projects from being located in west folder #132
Conversation
| path = mp.get('path') | ||
| if path is not None: | ||
| path = os.path.normpath(path) | ||
| for restricted_path in WEST_RESTRICTED_FOLDERS: |
There was a problem hiding this comment.
Would this be cleaner with a if X in Y construction?
There was a problem hiding this comment.
because we can't do a simple X in Y here.
west is restricted, but it could be that manifest states west/subfolder and that is also forbidden, and wouldn't be matched in a X in Y.
Thus we loop over WEST_RESTRICTED_FOILDERS to see if there is a match on commonpath between restricted and the path specified for the folder.
There was a problem hiding this comment.
commonpath now changed to commonprefix to support 3.4, as commonpath is new in python 3.5
Fixes: zephyrproject-rtos#102 This commit introduces a list of restricted folders that are compared against project defined paths to ensure a manifest does not contain paths ending in the west folder, e.g.: - west - west/foo - foo/../west while still allowing folders, such as: - west-ext as project folder. Signed-off-by: Torsten Rasmussen <torsten.rasmussen@nordicsemi.no>
tejlmand
force-pushed
the
issue_102_west_directory
branch
from
5c79619 to
b57a372
Compare
| @@ -127,6 +127,29 @@ def test_path(): | |||
| assert manifest.projects[0].path == 'sub/directory' | |||
| assert manifest.projects[0].abspath == os.path.realpath('/west_top/sub/directory') | |||
|
|
|||
| def test_restricted_path(): | |||
| # Projects are restricted from west/ folder, but folders containing west as | |||
| # part of path name, e.g. west-ext arte still allowed. | |||
| @@ -216,6 +219,18 @@ def _load(self, data, sections): | |||
| format(name) + | |||
| 'be used to name a manifest project') | |||
|
|
|||
| path = mp.get('path') | |||
| if path is not None: | |||
There was a problem hiding this comment.
If not given, the implied path is the name of the project. What if I have a project named west/subproject or so? Do we still fail?
If so, would you mind adding some test coverage (another invalid_xxx.yaml should do it) to show that is true?
| # Appending os.path.sep to ensure that eg. restricted path 'west/' and | ||
| # 'west-ext/' common prefix does not match on the restricted path 'west/' | ||
| restricted_path += os.path.sep | ||
| if os.path.commonprefix([restricted_path, path + os.path.sep]) is restricted_path: |
There was a problem hiding this comment.
Strings need to be compared with ==, not is.
|
Does this really apply now @tejlmand ? |
Good question. Will push an update, if we still consider this relevant. |
|
Sure, we could restrict people from using |
|
Should we close this for now until we're ready to revisit? We still have the issue open, so we won't forget. |
@tejlmand ping? |
|
I'm going to close this for now so it doesn't keep showing up in the slack channel's pull reminders, as it's the only one that is currently doing that. Please reopen if you'd like. |
Fixes: #102
This commit introduces a list of restricted folders that are compared
against project defined paths to ensure a manifest does not contain
paths ending in the west folder, e.g.:
while still allowing folders, such as:
as project folder.
Signed-off-by: Torsten Rasmussen torsten.rasmussen@nordicsemi.no