Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'path-prefix' key in west import map #367

Closed
mbolivar opened this issue Feb 3, 2020 · 13 comments · Fixed by #423
Closed

Add 'path-prefix' key in west import map #367

mbolivar opened this issue Feb 3, 2020 · 13 comments · Fixed by #423
Assignees

Comments

@mbolivar
Copy link
Contributor

mbolivar commented Feb 3, 2020

Example syntax:

manifest:
  projects:
  - name: zephyr
    url: https://github.com/zephyrproject-rtos/zephyr
    import:
      directory: zephyr_projects

Semantics:

  • the zephyr project, and all imported projects, have zephyr_projects as an additional path component. e.g. zephyr_projects/zephyr instead of zephyr as the path for zephyr project
  • the directory value cannot escape the workspace, e.g. directory: ../.. is an error
  • the directory value is cumulative, e.g. if the imported manifest in turn does an import with directory: foo, then the combined directory is zephyr_projects/foo

Alternate suggested spellings for directory:

  • path
  • path-prefix
@mbolivar
Copy link
Contributor Author

mbolivar commented Feb 3, 2020

cc @henrikbrixandersen @tejlmand

@mbolivar mbolivar self-assigned this Feb 3, 2020
@mbolivar mbolivar added this to the 0.8 milestone Feb 3, 2020
@henrikbrixandersen
Copy link
Member

LGTM. Personally I prefer directory.

@DenvE
Copy link

DenvE commented Feb 10, 2020

Directory is fine fo me too

@marc-cpdesign
Copy link

I'd love to see this implemented also !

I'll take whatever keyword I get for 'directory', but also consider some alternatives:

  • folder
  • destination
  • destination-prefix
  • base-prefix
  • base-destination

(Having 'prefix' or 'base' in there appeals to me because the actual destination is the path+name)

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 19, 2020

(Having 'prefix' or 'base' in there appeals to me because the actual destination is the path+name)

Agreed, "directory" or "path" are super generic terms, way too vague when left alone.

Inspired by "destination" and trying to describe the action rather than the elusive nature, how about into:?

manifest:
  projects:
  - name: zephyr
    url: https://github.com/zephyrproject-rtos/zephyr
    import:
      into: all_imports/zephyr_clones/

By the way will this new prefix support more than one multiple directory level?

@mbolivar
Copy link
Contributor Author

By the way will this new prefix support more than one multiple directory level?

It should IMO.

@marc-cpdesign
Copy link

+1 for "into"
+1 for recursive/ multi-level import support

Let me know if I can help somehow.

@euripedesrocha
Copy link

I would like to have something like this. But I would prefer a syntax that adds a prefix to the defaults section of the manifest:

defaults:
    path_prefix: prefix_path

My goal at this moment is to have only the projects I´m using by setting import as false.

@tejlmand
Copy link
Collaborator

Just a thought.
Today I can have the following in a manifest:

manifest:
  projects:
  - name: zephyr-project
    path: zephyr-upstream
    repo-path: zephyr
    remote: upstream

and now you suddenly add directory as a keyword, so a manifest could end up as:

manifest:
  projects:
  - name: zephyr-project
    path: zephyr-upstream
    repo-path: zephyr
    remote: upstream
    import:
      directory: zephyr_projects

mixing path and directory in our yaml is inconsistent, so simply for that reason I dislike directory.

In this case, import-path or path-prefix would fit better in current structure, as well as little more self-explaining.

And we would end up with:

manifest:
  projects:
  - name: zephyr-project
    path: zephyr-upstream
    repo-path: zephyr
    remote: upstream
    import:
      import-path: zephyr_projects
      path-prefix: zephyr_projects

@marc-hb
Copy link
Collaborator

marc-hb commented May 4, 2020

mixing path and directory in our yaml is inconsistent,

While I find path and directory both too generic, I think the difference between them is well defined: a path is (long story short) a list of nested directories. This definition is surprisingly consistent across languages:
https://docs.python.org/3/library/pathlib.html#accessing-individual-parts
https://perldoc.perl.org/File/Path.html
https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.management/split-path
https://docs.oracle.com/javase/8/docs/api/java/nio/file/Path.html
https://en.cppreference.com/w/cpp/filesystem/path

Hence my earlier question about supporting multiple levels.

@tejlmand
Copy link
Collaborator

tejlmand commented May 4, 2020

mixing path and directory in our yaml is inconsistent,

While I find path and directory both too generic, I think the difference between them is well defined: a path is (long story short) a list of nested directories. This definition is surprisingly consistent across languages:

I was more referring to the situation in the original proposal where the yaml could look as:

manifest:
  projects:
  - name: zephyr-project
    path: zephyr-upstream
    import:
      directory: zephyr_projects

in this case I personally fail to see why path is correct under projects, and directory would be correct under the import.
As I assume users can write:

    import:
      directory: imported_projects/zephyr_projects

then it seems to my the path-based keyword is still most correct, and what is already in use.

@egernst
Copy link

egernst commented Jun 2, 2020

This seems like a gap with west now -- looking forward to this issue being resolved by a PR.

@mbolivar-nordic mbolivar-nordic changed the title Add 'directory' key in west import map Add 'path-prefix' key in west import map Aug 14, 2020
mbolivar-nordic added a commit to mbolivar-nordic/west that referenced this issue Aug 14, 2020
Several users have requested the ability to place a project and its
imports in a designated subdirectory to keep third party code
separated from user-written code. Add this feature using a new key in
an import: map.

Example syntax:

  manifest:
    projects:
    - name: zephyr
      url: https://github.com/zephyrproject-rtos/zephyr
      import:
        path-prefix: zephyr_projects

Semantics:

 - the zephyr project, and all imported projects, have zephyr_projects
   as an additional path component. e.g. zephyr_projects/zephyr instead
   of zephyr as the path for zephyr project

 - the path-prefix value cannot escape the workspace, e.g.
   path-prefix: ../.. would be an error in the above example.

 - the 'path-prefix' value is "cumulative" across imports, e.g.
   if the imported manifest in turn does an import with "path-prefix: foo",
   the combined directory is "zephyr_projects/foo"

We've already laid most of the groundwork for plumbing this through
the import handling code using the _import_ctx and _import_map
namedtuples. When recursively loading Manifest instances during import
resolution, _import_ctx is a container that allows us to pass
information obtained from "higher" manifests down to "lower" manifests
in the tree. The _import_map namedtuple is just a container for the
import: contents when its value is a dict.

What remains to be done is then to add the current "cumulative" path
prefix to _import_ctx and pass it around to a couple of additional
places where it wasn't previously needed but now is. This requires
generalizing _new_ctx() a bit so we can combine an existing context
with the entire _import_map at a particular "import:" instead of just
using its filter function like we do now.

The one edge case is that since path-prefix affects the "imported"
project itself (zephyr in the above example), we need to make sure to
load it when creating every Project instance. This means we
effectively do it twice, which is a bit wasteful but keeps the code
simple, so it's worthwhile waste.

Fixes: zephyrproject-rtos#367
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
@mbolivar-nordic
Copy link
Contributor

@marc-hb and @tejlmand have convinced me that the keyword should contain 'path'. I think I'll use 'path-prefix'.

I added #423 for this. Testing and feedback very much desired.

mbolivar-nordic added a commit to mbolivar-nordic/west that referenced this issue Aug 14, 2020
Several users have requested the ability to place a project and its
imports in a designated subdirectory to keep third party code
separated from user-written code. Add this feature using a new key in
an import: map.

Example syntax:

  manifest:
    projects:
    - name: zephyr
      url: https://github.com/zephyrproject-rtos/zephyr
      import:
        path-prefix: zephyr_projects

Semantics:

 - the zephyr project, and all imported projects, have zephyr_projects
   as an additional path component. e.g. zephyr_projects/zephyr instead
   of zephyr as the path for zephyr project

 - the path-prefix value cannot escape the workspace, e.g.
   path-prefix: ../.. would be an error in the above example.

 - the 'path-prefix' value is "cumulative" across imports, e.g.
   if the imported manifest in turn does an import with "path-prefix: foo",
   the combined directory is "zephyr_projects/foo"

We've already laid most of the groundwork for plumbing this through
the import handling code using the _import_ctx and _import_map
namedtuples. When recursively loading Manifest instances during import
resolution, _import_ctx is a container that allows us to pass
information obtained from "higher" manifests down to "lower" manifests
in the tree. The _import_map namedtuple is just a container for the
import: contents when its value is a dict.

What remains to be done is then to add the current "cumulative" path
prefix to _import_ctx and pass it around to a couple of additional
places where it wasn't previously needed but now is. This requires
generalizing _new_ctx() a bit so we can combine an existing context
with the entire _import_map at a particular "import:" instead of just
using its filter function like we do now.

The one edge case is that since path-prefix affects the "imported"
project itself (zephyr in the above example), we need to make sure to
load it when creating every Project instance. This means we
effectively do it twice, which is a bit wasteful but keeps the code
simple, so it's worthwhile waste.

Fixes: zephyrproject-rtos#367
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
mbolivar-nordic added a commit that referenced this issue Aug 14, 2020
Several users have requested the ability to place a project and its
imports in a designated subdirectory to keep third party code
separated from user-written code. Add this feature using a new key in
an import: map.

Example syntax:

  manifest:
    projects:
    - name: zephyr
      url: https://github.com/zephyrproject-rtos/zephyr
      import:
        path-prefix: zephyr_projects

Semantics:

 - the zephyr project, and all imported projects, have zephyr_projects
   as an additional path component. e.g. zephyr_projects/zephyr instead
   of zephyr as the path for zephyr project

 - the path-prefix value cannot escape the workspace, e.g.
   path-prefix: ../.. would be an error in the above example.

 - the 'path-prefix' value is "cumulative" across imports, e.g.
   if the imported manifest in turn does an import with "path-prefix: foo",
   the combined directory is "zephyr_projects/foo"

We've already laid most of the groundwork for plumbing this through
the import handling code using the _import_ctx and _import_map
namedtuples. When recursively loading Manifest instances during import
resolution, _import_ctx is a container that allows us to pass
information obtained from "higher" manifests down to "lower" manifests
in the tree. The _import_map namedtuple is just a container for the
import: contents when its value is a dict.

What remains to be done is then to add the current "cumulative" path
prefix to _import_ctx and pass it around to a couple of additional
places where it wasn't previously needed but now is. This requires
generalizing _new_ctx() a bit so we can combine an existing context
with the entire _import_map at a particular "import:" instead of just
using its filter function like we do now.

The one edge case is that since path-prefix affects the "imported"
project itself (zephyr in the above example), we need to make sure to
load it when creating every Project instance. This means we
effectively do it twice, which is a bit wasteful but keeps the code
simple, so it's worthwhile waste.

Fixes: #367
Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants