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

Better support for projects with imports when users do not want/have them locally #652

Open
aescolar opened this issue Apr 24, 2023 · 7 comments
Labels
enhancement New feature or request Partial imports Incomplete or changing imports are much more complicated than you think

Comments

@aescolar
Copy link
Member

aescolar commented Apr 24, 2023

When a manifest has a project with an import, west requires that project to be present locally so it can resolve the whole manifest before it can do quite a few of its functions.
If that is not the case, some west commands/API operations will fail in a bit mysterious ways.
Some users are either confused by the way west fails, and or / do not like this requirement, and/or consider it unnecessary.

I create this issue to try to list possible solutions and/or improvements, so maybe be arrive at a nice option.

  1. west supports projects with import that are also part of a group Enhancement: import filtering #543 via import-group-filters:. This is not possible today, but would solve the problem when the project is not actually necessary (wrt to users being required to always have a project with an import)
  2. west does not fail when a project with an import is missing, but instead prints a warning to stderr and continues like if the project did not have the import set: This would alleviate the problem for users who knowingly do not want that project. Additionally west could offer the option to wave that warning by configuration.
  3. west provides a clearer error for west extension commands when this type of failure occurs (this would at least reduce the confusion and possible following frustration from users)
  4. west provides clearer errors for all commands or API calls when it tries to collate the manifest but it fails due to a missing import.
  5. west update having a new option to pull "whatever is the minimum needed" for west to work further. That is, all projects with imports, apart from the projects which may be explicitly listed. For ex. west update --and_required project_x project_y. (This would mitigate the problem for users and CI)
  6. (Variant of the previous). west init having a new option to pull "whatever is the minimum needed" for west to work further...
  7. west provide a new feature in which users can, thru local west workspace configuration, filter projects out of a manifest Provide local project filter in west config #653 . The manifest could set (as a project property) that that project is by default to be filtered out.
  8. west supports projects with import that are also part of a group by tracking relevant groups that were enabled at the time we did such an import, and erroring out if an attempt is made in a group-filter processed later that disables any of those relevant group
@aescolar
Copy link
Member Author

CC @mbolivar @carlescufi

@aescolar aescolar changed the title Better support for projects with imports when users do not have them locally Better support for projects with imports when users do not want/have them locally Apr 24, 2023
@carlescufi carlescufi added enhancement New feature or request Partial imports Incomplete or changing imports are much more complicated than you think labels Apr 24, 2023
@mbolivar-nordic
Copy link
Contributor

mbolivar-nordic commented Apr 25, 2023

4. west provides clearer errors for all commands or API calls when it tries to collate the manifest but it fails due to a missing import.

For commands:

For API calls: unless the exceptions generated by the API are confusing, I think that's a problem for the user of the west API, not west itself.

@mbolivar-nordic
Copy link
Contributor

  1. west supports projects with import that are also part of a group Enhancement: import filtering Enhancement: import filtering #543. This is not possible today, but would solve the problem when the project is not actually necessary (wrt to users being required to always have a project with an import)

To be clear, 'groups' and 'import' cannot be combined in a single project. This is not a question of priority, but semantics.

Import filtering is a separate enhancement that has been discussed to try to do filtering of imports in a way that doesn't use 'groups'.

  1. west does not fail when a project with an import is missing, but instead prints a warning to stderr and continues like if the project did not have the import set: This would alleviate the problem for users who knowingly do not want that project. Additionally west could offer the option to wave that warning by configuration.

As a design principle I have tried to avoid situations that lead to nondeterministic 'west update' behavior (unless the user explicitly asked for it with branches as project revisions) without a good reason. I don't see a great reason here, but I haven't thought about it hard.

  1. west provides a clearer error for west extension commands when this type of failure occurs (this would at least reduce the confusion and possible following frustration from users)

#654

  1. [...]

See previous comment.

  1. west update having a new option to pull "whatever is the minimum needed" for west to work further. That is, all projects with imports, apart from the projects which may be explicitly listed. For ex. west update --and_required project_x project_y. (This would mitigate the problem for users and CI)

I don't think this is the way.

Consider a situation where 'import:' is used recursively and you aren't sure where in the tree a particular extension command is defined (extension commands have a first-project-which-defines-the-extension "wins" semantics).

In practice, and in general, you basically have to resolve the entire manifest before you can make a decision about what extensions are defined where.

  1. (Variant of the previous). west init having a new option to pull "whatever is the minimum needed" for west to work further...

Extending 'west init' was discussed and rejected a while ago (#52), but feel free to pursue this if you think the discussion in there should be revisited.

  1. west provide a new feature in which users can, thru local west workspace configuration, filter projects out of a manifest Provide local project filter in west config Provide local project filter in west config #653 (This would mitigate the issue for users and CI, as they could add the local filter after a west init)

I deliberately decoupled active and inactive projects from enabled and disabled project groups for this reason: I wanted to leave open the option of adding new ways to make a project definition inactive.

So this is a possibility, but IMO is likely overkill if we can get by with #654.

@aescolar
Copy link
Member Author

aescolar commented Apr 25, 2023

Thanks @mbolivar-nordic

west update having a new option to pull "whatever is the minimum needed" for west to work further. That is, all projects with imports, apart from the projects which may be explicitly listed. For ex. west update --and_required project_x project_y. (This would mitigate the problem for users and CI)

I don't think this is the way.
Consider a situation where 'import:' is used recursively and you aren't sure where in the tree a particular extension command is defined (extension commands have a first-project-which-defines-the-extension "wins" semantics).
In practice, and in general, you basically have to resolve the entire manifest before you can make a decision about what extensions are defined where.

Just one note: This would be an option for savvy users who would otherwise do even more adhoc things, maybe for CI. If the user wants an extension command but does not know where it is, he better doesn't pull single projects, or the like.

@aescolar
Copy link
Member Author

aescolar commented Apr 26, 2023

Hi @mbolivar-nordic ,
Talking with a user concerned with this, his concern is about CI jobs or automated jobs, where there is no real need to do a west update beyond west requiring it.
He sees adding a west update <whatever_project_w_import> as fragile, as if another repo is added with an import, it will also break (i.e. ongoing maintenance).
For that case I'd think 1., 2., 5., 6 (or maybe 7 if we can have a positive list) would be solutions that remove the concern.

@marc-hb
Copy link
Collaborator

marc-hb commented Apr 27, 2023

west update having a new option to pull "whatever is the minimum needed" for west to work further.

+1, isn't that just #644?

It should really be possible to somehow get to a working state without downloading everything first. I mean a working state for "core" west commands of course, ignoring project-specific extensions.

This is especially true considering zephyr/west.yml seems to be growing out of control with little concern for modularity.

mbolivar-nordic added a commit to mbolivar-nordic/west that referenced this issue May 16, 2023
This involves a rework of the group filter mechanism, which has the
side effect of fixing its behavior to match its documentation. We can
therefore drop the xfail on a related test.

This is a top level manifest key that takes a boolean. The default
value is 'true' and is west's current behavior. The behavior when
this value is false can be shown by this example:

  manifest:
    group-filter: [-foo]
    import-group-filters: false
    projects:
      - name: proj
        import: true
    self:
      import: submanifest.yml

Here, the final Manifest.group_filter attribute for the manifest
whose data are shown above will always be [-foo], regardless of what
whe imported manifest data from 'proj' or 'submanifest.yml' are.

The purpose of this extension is to allow the combination of 'groups'
and 'import':

- In previous versions of west, we could not combine the two, because
  in pathological edge cases, we could end up resolving an import for a
  group which was later disabled by an import that happened later on.

- With this new backwards-compatible enhancement to the manifest file
  format, we can combine 'import:' with 'groups:' in a single project
  whenever 'import-group-filters:' is 'false'. This is because we will
  know the final 'group_filter' attribute for the entire Manifest at
  the time we perform the import, and it cannot be changed later by
  any imports we might perform.

Since this new behavior would break backwards compatibility if made the
default, we make it explicitly opt-in by requiring
'import-group-filters: false'.

Fixes: zephyrproject-rtos#663
Fixes: zephyrproject-rtos#652

Signed-off-by: Martí Bolívar <[email protected]>
@marc-hb
Copy link
Collaborator

marc-hb commented Sep 8, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Partial imports Incomplete or changing imports are much more complicated than you think
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants