bsim module must be cloned for west to work

[jgl-meta]

Describe the bug
When running west a prompt that looks like the following appears.
FATAL ERROR: failed manifest import in bsim (tools/bsim) Failed importing "west.yml" from revision "908ffde6298a937c6549dbfa843a62caab26bfc5" Hint: for this to work: - bsim must be cloned - its manifest-rev ref must point to a commit with the import data To fix, run: west update
this seems to be a little backwards since it forces pulling in additional modules, outside of the minimum necessary to build.

To Reproduce
Steps to reproduce the behavior:

1. Follow the getting started guide but replace west update with west update only_what_I_need
2. Run a west status
   You will get an error message that looks like the one above.
3. Run west build -b qemu_x86 samples/hello_world/, you get: west: error: argument <command>: invalid choice: 'build'
4. git revert 01a12f6 makes everything work again instantly (no need to re-init, update or anything)

Expected behavior
We should still be able to build without the bsim module. Or at least we should provide a work around to build without it.

Environment (please complete the following information):

• OS: Seen on MacOS and Linux

[carlescufi]

I am adding the TSC label because there is no workaround for this, unless west starts supporting group filters and import statements in a single project.

[cfriedt]

I remember this was a topic at a TSC Meeting recently. It didn't occur to me at the time that it would create a mandatory module requirement. It's kind of against the ideas discussed in #54276 and #55056 .

Would be interesting to investigate in a bit of depth - I'm sure we can come up with a solution. Who is the best assignee for it?

[carlescufi]

Run a west command such as update or init

@jgl-meta west init will never fail this way. west update will not either. It is the rest of the commands that will indeed fail.

[cfriedt]

I updated the description - able to repro fairly easily with west status.

[aescolar]

CC @mbolivar
The immediate fix is to follow the expected workflow and do a west update after init, or after git checkout on the manifest repo (after a checkout which introduced a new project with an import).
If you want to avoid a full west update you can do just a west update bsim, and that will fetch you the bsim manifest repo (~400KB). After that things will work as before. You also only need to do it once even if you jump branches in the main manifest as west does not delete from the workspace projects that disappear from the manifest. (So the bsim manifest project will be back there when is needed again)

Note that this happens only if you are using the default zephyr manifest, but do not do a west update.
If you are using another manifest that imports the zephyr manifest, this problem does not exist (if you do not add the bsim project to the import allow-list, things work just fine also)

(To the best of my understanding) The issue is just a technicality around west. If the manifest has a project with an import, west needs that project locally to do anything beyond "init" and "update", as otherwise what west tries to do is collate the whole manifest (which requires the submanifests from the imports) and will fail when it does not have the submanifests from the imports.
The basic assumption in west in this regard seems to be that users will do a plain "west update" and get everything they need in that way before running any other west command.

Note that the "bsim" project is not the whole bsim simulator. It is just 5 files which include the submanifest that is being imported.

[aescolar]

Created an issue in west to track possible improvements zephyrproject-rtos/west#652

[nordicjm]

From my point of view as a non-TSC member, I'll leave my comments here: this is a complete hindrance to my every day usage of zephyr. Traditionally I could get a selective clone of zephyr ready for development (i.e. west init, edit west.yml with a script to get the modules I specifically need, west update, git checkout west.yml) and develop as needed, switch branches to the many open PRs I have. This is now an impossibility, if the bsim repo is in the manifest and I do not have it pulled down by west, I'm no longer able to build or develop anything, I can't even build the documentation because I have not got the bsim module, which is completely irrelevant for any and all development I do and I would argue for the vast majority of all development everyone does in zephyr. My vote would be on reverting the change that introduced this 01a12f6 and only adding it back once west is updated to support optionally loading this module if it exists or ignoring it if it has not been fetched without needing to have a separate manifest with an allow/block list.

Now it might be said that bsim is a small repo with only a minimum number of files, but if that module has been allowed to be added like this, that means any module can be added like this. What if it's now e.g. 6 different modules with imports that have a size in the gigabyte range which similar to bsim are used by the minority of users - I'm not against bsim particularly, I am against this whole concept in general. Once one module is allowed, that's it for allowing all modules to work like this. e.g. let's force all hals to be required to build any project for any platform - want to build hello_world for native_posix, well let's prevent that unless you have every vendor's hal fetched and downloaded.

[nordicjm]

@cfriedt I think you accidentally edited my comment, so I'm reverting it and putting your comment here:

Traditionally I could get a selective clone of zephyr ready for development (i.e. west init, edit west.yml with a script to get the modules I specifically need, west update,

@nordicjm - you can also run west update [module names...] to get only the modules you need without editing west.yml.

Same here though - this is a showstopper for our workflow. I haven't looked into the relevant west code myself, so I can't really speculate at a solution. There were some possibilities mentioned in a linked issue though.

[cfriedt]

@cfriedt I think you accidentally edited my comment, so I'm reverting it and putting your comment here:

@nordicjm - sorry about that - fat fingers and not enough ☕ . Thanks for catching it 👍

[nordicjm]

@nashif pinging you as I vaguely remember you were the one I saw comment that non-zephyr repos weren't allowed in west.yml, can we not move these files in to zephyr or do something with it? The rule doesn't really make any sense since the bsim-manifest repo is now required to be present to do anything with zephyr and it imports the west.yml from that repo, that repo is hosted on zephyr and adds third party repos as per https://github.com/zephyrproject-rtos/babblesim-manifest/blob/main/west.yml so I don't really see the point in what moving it to a separate repo but making that repo required did

[aescolar]

@nordicjm Please note that people is already using this bsim_manifest repo and the bsim project in the main manifest to fetch the simulator. We can't just remove it, or move things around without careful thought.
So, "let's just revert that commit" is hardly a good solution. That will just cause another minor breakage for whoever is doing adhoc things (instead of a full west update), but adapted to needing bsim (by maybe doing a west update bsim in their scripting); but more importantly it will cause a worse break for those who started getting and using the simulator in this way. This was added because quite many users requested it. I can only imagined some of those are using it already.

This issue of "being forced to at the very least fetch the bsim manifest repo, if you don't want to do a plain west update" to be able to work with west with the zephyr main manifest, seems to me a relatively minor inconvenience we can live with while we do something nicer (maybe in the west side).
So far I haven't heard of any other issue about this, apart from some users who were not following the default workflow having been surprised, and maybe annoyed, that their workflow broke, and they did not know why. But once you know why, fixing it is adding 1 extra command (and using 400KB of disk space and 1 second for an extra git fetch).

[nordicjm]

@nordicjm Please note that people is already using this bsim_manifest repo and the bsim project in the main manifest to fetch the simulator. We can't just remove it, or move things around without careful thought.

I don't remember being asked for thoughts for having this change added in the first place, hence why I'm bringing it up here. This change has not been put into any releases so the true number of users it will cause problems for is not known, it has actually completely broken a private CI task I have for synchronising zephyr to a different git server because I can no longer run e.g. west list to just get a list of repos. And seemingly at least 2 members of the TSC that voted on it were not aware of the changes it entailed.

[aescolar]

Sorry, but why is this a release blocker? The only background I can see in the TSC mom that @cfriedt mentioned it during the meeting even that it was not in the TSC meeting agenda.
I would request that we don't overblow minor issues..
If this is a serious problem, it needs to be described clearly why.

[cfriedt]

@aescolar - the release blocker tag is mainly to get flagged at the next release meeting.

I honestly don't think the ideal solution will be a lot of work, so I don't want you to be worried.

Given that the bsim module is so small, is there a possibility it can be added to the main repo? I did see that you presented a number of possible solutions as well. Thanks for doing that.

You can unassign yourself if it helps for now.

[aescolar]

@cfriedt removing the bsim repo would cause trouble for its users (as already stated above ).
Would you be so kind as to consider possible solutions in your workflow?

[aescolar]

@aescolar - the release blocker tag is mainly to get flagged at the next release meeting.

Instead of discussing this at the next release meeting (Please note those meetings are at a quite inconvenient time for people working in Europe), could you please check if the solution @mbolivar proposed for your particular problem is ok?
Otherwise it seems we are not spending our time wisely.

For those interested, mbolivar proposal: https://discord.com/channels/720317445772017664/733037524498382881/1100819756904366151
and variant in zephyrproject-rtos/west#653 (comment)

[aescolar]

Removing the "release blocker", as this seems to require meta to understand their problem, and evaluate a proposed solution.

[marc-hb]

I'm not against bsim particularly, I am against this whole concept in general. Once one module is allowed, that's it for allowing all modules to work like this.

Agreed 200%. This is simply not scalable at all.

User-friendliness is critical, especially around version control which too many developers don't want to hear about. I understand the #55696 desire to make life easier for babblesim users but what about users of other modules? Can they all stuff their default locations in zephyr/west.yml too? This is obviously not scalable.

Ironically, all other users now have to know about the secret west update bsim trick. How user friendly!

Downloading everything by default with a plain west update is slow and not user friendly either - and obviously not scalable. It should always be possible to west update only_what_I_need out of the box.

The official zephyr/west.yml should be kept simple and point only at official zephyrproject-rtos projects, directly or indirectly. It probably does not need any import.

This is the flawed, "manifest stuffing" opt-in logic in a nutshell:

• Stuff default manifests with many locations by default for user friendliness
• group-disable them by default because most users don't want them (group-filter: [-babblesim])
• Ask users who want opt-ins to "disable the disable"

The usual, scalable opt-in approach is to simply download and drop opt-ins in a well known submanifests/ location. Almost as user friendly, a familiar approach, infinitely scalable and no "double negation". Compose by adding, not by stuffing+remove+re-add. I already pointed at this flawed opt-in approach in 2021 in the major group filter change zephyrproject-rtos/west#481 (west v0.10) but I was "overruled" :-)

[marc-hb]

as this seems to require meta to understand their problem and evaluate a proposed solution.

This sounds quite dismissive. Commit 01a12f6 broke west update only what I need in a very confusing way and there's nothing specific to Meta about that. Meta just happened to hit this first, this commit was merged less than one week ago. Reverting 01a12f6 makes everything instantly work again.

west update only what I need has been working with zephyr/west.yml since forever so I think this is major regression that deserves a priority higher than "low". This type of manifest stuffing of the default zephyr/west.yml is also crossing a major line IMHO.

Given that the bsim module is so small, is there a possibility it can be added to the main repo?

+1, if it's small and very popular then just add it like all other repos. People who don't want it can west update only what I need as they always have.

PS @jgl-meta: I took the liberty to add west update only what I need to your description above, please review.

[marc-hb]

@nordicjm Please note that people is already using this bsim_manifest repo and the bsim project in the main manifest to fetch the simulator. We can't just remove it, or move things around without careful thought.
So, "let's just revert that commit" is hardly a good solution. That will just cause another minor breakage for whoever is doing adhoc things (instead of a full west update), but adapted to needing bsim (by maybe doing a west update bsim in their scripting); but more importantly it will cause a worse break for those who started getting and using the simulator in this way. This was added because quite many users requested it. I can only imagined some of those are using it already.

I don't think this matters. This is a development branch so breakages are expected regularly (e.g. #55290, #57127, #57298,...). What matters is how things will work in the future. This commit has been merged only one week ago whereas west update only what I need has been working since the start of the project; so if we cared about the past (we don't) then this commit should absolutely be reverted.

[cfriedt]

@aescolar - please leave the Release Blocker label.

As I've mentioned, it's mainly to indicate to the release team that we need to address this at the next Release Meeting.

I've also marked it as priority: medium again because it's impacted 3 or 4 companies that I know of at this point, multiple boards, many CI workflows, etc. It isn't a showstopper upstream in CI though, so for that reason it's not priority: high.

Let's avoid being combative in the comments please and try to work together to find a solution that works for everyone.

Thanks for your patience.

[cfriedt]

Removing the "release blocker", as this seems to require meta to understand their problem, and evaluate a proposed solution.

So again (I've already mentioned this) - please do not single out Meta. We've already heard from at least 3 or 4 companies that this is a problem. Those companies include (in chronological order FWIU):

1. Intel
2. Meta
3. STM
4. Nordic

Anas also mentioned it shortly before going on vacation on Discord.

[jgl-meta]

In the release meeting we have discussed this issue and it is agreed that this is a release blocker.

[nashif]

The part I did not know about is the fact that the group filter works on the imported repos and not the entry in the west manifest itself, so:

  group-filter: [-babblesim]

  #
  # Please add items below based on alphabetical order
  projects:
    - name: bsim
      repo-path: babblesim-manifest
      revision: 908ffde6298a937c6549dbfa843a62caab26bfc5
      import:
        path-prefix: tools

means that the filtering does not apply to the bsim module and it can't be filtered out, group-filter: [-babblesim] works on the repos listed in the babbesim-manifest repo, so this bsim module becomes mandatory, i.e. you will always need it.

@mbolivar-nordic @aescolar What is the rationale or constraints for not allowing filtering when import is used? why can't we filter out the parent repo?

[aescolar]

@mbolivar-nordic @aescolar What is the rationale or constraints for not allowing filtering when import is used?

@nashif It is described here: zephyrproject-rtos/west#543

[yperess]

I'm curious what's the status of this issue? For me running west update bsim isn't working. I have to revert 01a12f6 locally

[mbolivar-nordic]

We are working on a new version of west to solve this problem

[aescolar]

After a discussion between @mbolivar-nordic @carlescufi @tejlmand, @57300 and me , it was agreed @mbolivar-nordic will continue with option 7 of zephyrproject-rtos/west#652 : zephyrproject-rtos/west#653 as a fix for this

[nordicjm]

After a discussion between @mbolivar-nordic @carlescufi @tejlmand, @57300 and me , it was agreed @mbolivar-nordic will continue with option 7 of zephyrproject-rtos/west#652 : zephyrproject-rtos/west#653 as a fix for this

So long as this does not need someone to change the configuration locally using a local west configuration file and it can be done through west manifests, that's fine, otherwise it's a non-starter.

[nashif]

After a discussion between @mbolivar-nordic @carlescufi @tejlmand, @57300 and me , it was agreed @mbolivar-nordic will continue with option 7 of zephyrproject-rtos/west#652 : zephyrproject-rtos/west#653 as a fix for this

So long as this does not need someone to change the configuration locally using a local west configuration file and it can be done through west manifests, that's fine, otherwise it's a non-starter.

agree with that if I understood option 7 correctly: If I have bsim in the manifest, but not in my workspace, I will not be able to use west correctly and wst status gives me an error, option 7 will let me exclude bsim or any entry in the manifest and things will start working again? Hows does this solve the problem of west not working correctly due to missing bsim? Most likely one would end up reverting the bsim manifest commit like @yperess did in one of his PRs before looking into setting options in west config

But maybe I am wrong about what options 7 is supposed to do.

[yperess]

agree with that if I understood option 7 correctly: If I have bsim in the manifest, but not in my workspace, I will not be able to use west correctly and wst status gives me an error, option 7 will let me exclude bsim or any entry in the manifest and things will start working again? Hows does this solve the problem of west not working correctly due to missing bsim? Most likely one would end up reverting the bsim manifest commit like @yperess did in one of his PRs before looking into setting options in west config

I would 100% just revert it locally. I don't think it would cross my mind to check the west config.

[marc-hb]

option 7 will let me exclude bsim or any entry in the manifest and things will start working again? Hows does this solve the problem of west not working correctly due to missing bsim?

Because it would be disabled by default.

Copying part of zephyrproject-rtos/west#660 (comment)

--- a/west.yml
+++ b/west.yml
@@ -15,6 +15,8 @@
 # information.
 
 manifest:
+  version: "1.1"
+
   defaults:
     remote: upstream
 
@@ -23,6 +25,7 @@ manifest:
       url-base: https://github.com/zephyrproject-rtos
 
   group-filter: [-babblesim]
+  import-group-filters: false
 
   #
   # Please add items below based on alphabetical order
@@ -30,6 +33,8 @@ manifest:
     - name: bsim
       repo-path: babblesim-manifest
       revision: 908ffde6298a937c6549dbfa843a62caab26bfc5
+      groups:
+        - babblesim
       import:
         path-prefix: tools
     - name: canopennode

[marc-hb]

As far back as 2021 I found stuffing manifests with URLs disabled by default not intuitive when you can just drop random .yml in a well known submanifests.d/ subdirectory instead (or delete some)

Two years later this approach might finally work?

[mbolivar-nordic]

New approach: zephyrproject-rtos/west#664

[carlescufi]

@nashif @yperess @nordicjm with the approach in zephyrproject-rtos/west#664 all you need to do is to type this one single time on your command-line:

west config --global manifest.project-filter -- -bsim

[carlescufi]

After a discussion between @mbolivar-nordic @carlescufi @tejlmand, @57300 and me , it was agreed @mbolivar-nordic will continue with option 7 of zephyrproject-rtos/west#652 : zephyrproject-rtos/west#653 as a fix for this

So long as this does not need someone to change the configuration locally using a local west configuration file and it can be done through west manifests, that's fine, otherwise it's a non-starter.

@nordicjm this only requires you to alter the local config once, since this setting will be then stored in your ~/.westconfig once and forever.

[nordicjm]

After a discussion between @mbolivar-nordic @carlescufi @tejlmand, @57300 and me , it was agreed @mbolivar-nordic will continue with option 7 of zephyrproject-rtos/west#652 : zephyrproject-rtos/west#653 as a fix for this

So long as this does not need someone to change the configuration locally using a local west configuration file and it can be done through west manifests, that's fine, otherwise it's a non-starter.

@nordicjm this only requires you to alter the local config once, since this setting will be then stored in your ~/.westconfig once and forever.

But can't be changed by a manifest itself which e.g. does an import as the original solution discussed was able to do

@nashif @yperess @nordicjm with the approach in zephyrproject-rtos/west#664 all you need to do is to type this one single time on your command-line:

west config --global manifest.project-filter -- -bsim

By this I assume you mean + bsim to enable it not, not - bsim to disable it since users should not have to disable this, it should not be present by default?

[carlescufi]

After a discussion between @mbolivar-nordic @carlescufi @tejlmand, @57300 and me , it was agreed @mbolivar-nordic will continue with option 7 of zephyrproject-rtos/west#652 : zephyrproject-rtos/west#653 as a fix for this

So long as this does not need someone to change the configuration locally using a local west configuration file and it can be done through west manifests, that's fine, otherwise it's a non-starter.

@nordicjm this only requires you to alter the local config once, since this setting will be then stored in your ~/.westconfig once and forever.

But can't be changed by a manifest itself which e.g. does an import as the original solution discussed was able to do

Unfortunately that's correct. We have tried in every way possible to make that work but we have not been able to.

@nashif @yperess @nordicjm with the approach in zephyrproject-rtos/west#664 all you need to do is to type this one single time on your command-line:
west config --global manifest.project-filter -- -bsim

By this I assume you mean + bsim to enable it not, not - bsim to disable it since users should not have to disable this, it should not be present by default?

No, users will have it by default, at least for the time being. But you will be able to disable it permanently locally so that it doesn't affect you at all.

[nordicjm]

No, users will have it by default, at least for the time being. But you will be able to disable it permanently locally so that it doesn't affect you at all.

This is not a solution anyone with this problem has agreed to, so doesn't resolve anything for anyone with the above issues. @cfriedt