west init should recursively handle submodules for the manifest repo

[johnkjellberg]

To make the workflow west init ... west update fully working for manifest repositories with submodules, west init command should recursively get git submodules.

[marc-hb]

I'm not sure I understand, are you referring to submodules of the manifest repo specifically as opposed to submodules of projects listed in the manifest?

I understand that idea but it would break one of west's core principles and promises: never ever modify the manifest repo.

[mbolivar-nordic]

never ever modify the manifest repo.

... except during west init when we fetch it for the first time. So this request seems reasonable.

[marc-hb]

So it would affect west init only when used without -l, correct?

Seems fine as long as it's possible and easy to manually git clone the manifest repo non-recursively and take it from there.

[mbolivar-nordic]

So it would affect west init only when used without -l, correct?

Correct.

Seems fine as long as it's possible and easy to manually git clone the manifest repo non-recursively and take it from there.

This remains possible to do with -l.

[mbolivar-nordic]

I spent some time today trying to implement this within the constraints of how west init works now, and I think we need to change a few things to handle this properly.

Basically, west init uses git init + git fetch to get the manifest repository set up, instead of git clone. (Incidentally, west update does the same thing, but that's not what I'm talking about here.)

I think we should handle this issue (i.e. issue #498) by:

• changing west init to use git clone instead
• adding a --clone-opt option that users can use to add options to git clone

Then you'd be able to write something like this:

west init -m https://example.com/my-manifest --clone-opt=--recurse-submodules

This way, west gets out of your way and lets you decide how you want to set things up.

@johnkjellberg will that work for you?

@marc-hb any thoughts? I know you're about to ask me "why does west init do it this way, anyway". The answer is explained in
8679b43 (by @tejlmand). In hindsight, I think this use case is not that important to support, because initializing from a pull request can be done in a two-step process by initializing, then checking out the weirdo pull/1243/head ref before running west update. And anyway, we can always bend over backwards to fetch --manifest-rev and check out FETCH_HEAD if we really need to keep supporting it.

[marc-hb]

I know you're about to ask me "why does west init do it this way, anyway"

So you read me like a book during a pandemic.... I thought about: what if we were in the same office space and I got scared. Thanks for the link, very relevant.

This (--clone-opt) way, west gets out of your way and lets you decide how you want to set things up.

tl;dr: nice, LGTM.

Because west is a git wrapper for a very large part, there will always be a constant stream of requests for west to support more and more git features[*]. This happens with every wrapper and is especially common in the poster child for "too many layers of indirections": build systems. What I found works and scales well in this adjacent build system area is exactly this "passthrough" approach. What I found does not work and does not scale is trying to mimic/replicate each git option into a new west option (and now I'm trying not to think about J2EE frameworks).

One of the top issues with this "passthrough" approach is quoting whitespace on the command line. One layer of quoting is easy, two is hards and three and above are basically impossible. The -- double dash can sometimes help but maybe not here? Config files provide another common solution but they would too inconvenient in this case.

When that makes sense (not always), it's also good to preserve git defaults according to the https://en.wikipedia.org/wiki/Principle_of_least_astonishment

the weirdo pull/1243/head

Funny how git clone --branch seems to support only refs/heads and refs/tags/ whereas git fetch can fetch anything...

And anyway, we can always bend over backwards to fetch --manifest-rev and check out FETCH_HEAD if we really need to keep supporting it.

Do you mean git clone HEAD and then git fetch with a hopefully small performance overHEAD?

cc: @Damian-Nordic

[*] and issues with west vs git "impedance mismatches" like this very similar one: #496 (comment). BTW when is mercurial support planned? (just kidding)

[johnkjellberg]

Then you'd be able to write something like this:

west init -m https://example.com/my-manifest --clone-opt=--recurse-submodules

This way, west gets out of your way and lets you decide how you want to set things up.

@johnkjellberg will that work for you?

I think this is a matter of philosophy for the tool. For me, I imagined west init being something you can use with the least amount of prior knowledge about the project setup as possible. So you know that west init -m <repo>/west update will always work for having something ready to compile. Needing to know when to add --clone-opt will in a way defeat this, and would add just a little bit of extra convenience compared to doing a git pull --recurse-submodule manually after(which is nice, but not what I was aiming for).

I am a new user to west so I might have missed important things. What would be the drawback of having --recurse-submodules as the default behavior in the west init tool?

[marc-hb]

For me, I imagined west init being something you can use with the least amount of prior knowledge about the project setup as possible.

I think it's unfortunately difficult for a git wrapper not to assume any git knowledge and to "correct" some git decisions and defaults (e.g., recursive by default) because by doing so it breaks the https://en.wikipedia.org/wiki/Principle_of_least_astonishment for other users who do know git and got familiar with its defaults.

The idea that a west init just works without knowing about git submodules seems appealing but the initial ignorance/bliss is not going to last for long. I mean at some point the developer is going to have to actually use these submodules and then join the majority of users who had to learn something about how they work. The disappointment is going to be especially strong in a project that mixes BOTH submodules and west - really not the most user-friendly situation. I agree that west init --clone-opt --recurse-submodule looks annoying but that it seems petty to me compared to the typical and much bigger submodules pain.

What would be the drawback of having --recurse-submodules[=pathspec] as the default behavior in the west init tool?

Besides surprising people familiar with git I can think of another one: this makes changing the value more complex for west. Now west has to scan the value of --clone-opt and make decisions based on it instead of just passing it through.

[marc-hb]

@mbolivar-nordic how hard would it be to support --clone-opt in west config or in the manifest? I see west config already has update.fetch so --clone-opt would not seem out of place.

Slightly off-topic: is there a way for west init to initialize .west/config with default values coming from the manifest repo? I didn't find this in the documentation.

[mbolivar-nordic]

Do you mean git clone HEAD and then git fetch with a hopefully small performance overHEAD?

Yes, if we really really have to because somebody complains loudly enough.

But again I lean towards "this use case is not important enough to implement" for 'weird' refs like pull/N/head". In other words, "if west init --manifest-rev REF -m URL can't be turned directly into git clone -b REF URL, then it's the user's responsibility to init, then git fetch REF && git checkout FETCH_HEAD".

Bending over backwards to suit the way GitHub does things in edge cases like this feels a bit wrong.

[mbolivar-nordic]

@mbolivar-nordic how hard would it be to support --clone-opt in west config or in the manifest? I see west config already has update.fetch so --clone-opt would not seem out of place.

Config options are easy. I am skeptical of adding this to the manifest without a well thought out use case.

Slightly off-topic: is there a way for west init to initialize .west/config with default values coming from the manifest repo? I didn't find this in the documentation.

TL;DR no. Slightly longer answer, we discussed this during west's early days, but the only use case we came up with was to set zephyr.base, then we realized that we shouldn't be putting zephyr specifics into west anyway if we can avoid it and subsequently removed as many as we could, so we never revisited the idea.

[johnkjellberg]

The idea that a west init just works without knowing about git submodules seems appealing but the initial ignorance/bliss is not going to last for long. I mean at some point the developer is going to have to actually use these submodules and then join the majority of users who had to learn something about how they work.

I didn't mean that the person doesn't know about git submodules. I meant that the person don't know that the specific project includes submodules or not. Say a company have 10 manifest repos. Some with submodules and some without. Then the person needs to know which project needs that extra flag or not to get to a state so that it can be compiled.

I might not understand the purpose of west init fetching the manifest repo? I got the impression that it is a standard and easy way to get what you need. And to be consistent with that I feel that needing project specific flags would defeat that goal.

What would be the drawback of having --recurse-submodules[=pathspec] as the default behavior in the west init tool?

Besides surprising people familiar with git I can think of another one: this makes changing the value more complex for west. Now west has to scan the value of --clone-opt and make decisions based on it instead of just passing it through.

If --clone-opt is added that is. I meant compared to the current west. But perhaps there are other reasons to add --clone-opt?

[tejlmand]

In hindsight, I think this use case is not that important to support, because initializing from a pull request can be done in a two-step process by initializing, then checking out the weirdo pull/1243/head ref before running west update.

Personal experience, mostly I use current workspace when reviewing, but it does happen once in a while that I decide to create a new workspace for testing complex PRs and want to keep it away from my usual work, however I think that to majority of users the recursive submodule support might be more valuable.
The PR is the main use of special ref fetching I can think of.
And as @mbolivar-nordic points out, this special use-case can easily be handled we just a few extra git commands.

• changing west init to use git clone instead

I see no problem in doing this.

• adding a --clone-opt option that users can use to add options to git clone

Should we generalize this command:
--git-opt and then pass this raw to git ?
Then we could add this --git-opt to west diff, west update, and west status and it will be the callers responsibility to provide valid argument (or git itself will complain).

And of course allow --git-opt to be applied multiple times so users can pass more than one argument down to git.
That way we avoid having to add --clone-opt and later --diff-opt and later again yet another git argument.

[mbolivar-nordic]

I might not understand the purpose of west init fetching the manifest repo? I got the impression that it is a standard and easy way to get what you need. And to be consistent with that I feel that needing project specific flags would defeat that goal.

The problem with adding a specific flag or non-default git behavior to west init is that you can't please everyone, and if you try, you will regret it. My experience and opinion are that the best thing for a wrapper to do is in line with the zen of Python: in the face of ambiguity, refuse the temptation to guess.

For me, I imagined west init being something you can use with the least amount of prior knowledge about the project setup as possible. Needing to know when to add --clone-opt will in a way defeat this, and would add just a little bit of extra convenience compared to doing a git pull --recurse-submodule manually after(which is nice, but not what I was aiming for).

The "as possible" part is the important one here.

Maybe --recurse-submodules is what most people want, but some people will want --recurse-submodules=my-pathspec, others still will want --recurse-submodules --remote-submodules -j8, other people will find -j8 ridiculously underpowered and prefer -j16. Etc, etc.

There just isn't a one size fits all solution here, so I don't want to try to provide one. Especially given that there's no stability in the underlying tool itself: maybe git version 2.75 in the future will provide some wholly new default behavior for git clone that it wants everybody to use by default, and --recurse-submodules will defeat that default. Then west will have to decide to break compatibility, move to the new behavior, maybe do one or the other depending on the git version.... it gets messy. It's better to just expose the underlying tool and let the user decide.

[tejlmand]

Some with submodules and some without. Then the person needs to know which project needs that extra flag or not to get to a state so that it can be compiled.

How would you know if all submodules are indeed required.
There are many projects using submodules where a user might only want to fetch a few of those, namely those needed for the work he will be doing.

As example: connectedhomeip comes with a large list of submodules where you might only want a few of those.
https://github.com/project-chip/connectedhomeip/tree/master/third_party

So I think this is better to keep this as opt-in.
(connectedhomeip is not using west, but just use as an example)

[mbolivar-nordic]

--git-opt and then pass this raw to git ?

I think not because west calls git a surprisingly large number of times, so it would be ambiguous "which" git this refers to.

[tejlmand]

--git-opt and then pass this raw to git ?

I think not because west calls git a surprisingly large number of times, so it would be ambiguous "which" git this refers to.

Well, if for the west init if it is specified that it is for the clone part, it should be clear.
For west status and west diff things should also be clear.

west update is surely a bit more tricky.

[marc-hb]

I meant that the person don't know that the specific project includes submodules or not. Say a company have 10 manifest repos. Some with submodules and some without. Then the person needs to know which project needs that extra flag or not to get to a state so that it can be compiled.

It's the exact same problem with plain git, west only inherited it. There is no end of git usability issues that west could go and try to fix.

https://stevebennett.me/2012/02/24/10-things-i-hate-about-git/

From https://ideas.ted.com/the-wisdom-of-linus-torvalds/

There are people who care about the user interface (UI); I can’t do UI to save my life. I mean if I was stranded on an island and the only way to get off that island was to make a pretty UI, I’d die there.

[marc-hb]

It's the exact same problem with plain git, west only inherited it.

Just found this : https://stackoverflow.com/a/53622660

That is true; submodule.recurse is not affecting git clone.
This was a design decision once it was introduced, as the git clone might be too large.
Maybe we need to revisit that decision and just clone the submodules if submodule.recurse is set.

[marc-hb]

Slightly off-topic: is there a way for west init to initialize .west/config with default values coming from the manifest repo? I didn't find this in the documentation.

TL;DR no. Slightly longer answer, we discussed this during west's early days, but the only use case we came up with was to set zephyr.base, then we realized that we shouldn't be putting zephyr specifics into west anyway if we can avoid it and subsequently removed as many as we could, so we never revisited the idea.

I think --clone-opt would be good use case, except... it comes too late :-(

Similarly, the brand new manifest.group-filter looks like another good use case: it means different branches could trivially advertise different dependencies.

Also quoting https://docs.zephyrproject.org/latest/guides/west/config.html#west-config-index

Configuration options supported by Zephyr’s extension commands are documented in the pages for those commands.

---

But perhaps there are other reasons to add --clone-opt?

#496 may provide some example(s)

[johnkjellberg]

I meant that the person don't know that the specific project includes submodules or not. Say a company have 10 manifest repos. Some with submodules and some without. Then the person needs to know which project needs that extra flag or not to get to a state so that it can be compiled.

It's the exact same problem with plain git, west only inherited it. There is no end of git usability issues that west could go and try to fix.

Yes. I agree with the problem is in plain git. I guess this is mostly about expectations about west and what it tries to solve. That west init checks out the manifest repo at all made me have different expectations then what it is designed for I think.

I think I have a much better insight into west after all these comments on this issue. I don't think I have anything more to contribute regarding this issue. Thanks.

[marc-hb]

That west init checks out the manifest repo at all made me have different expectations then what it is designed for I think.

That's a very fair point and in fact in 4th comment above I wrote:

So it would affect west init only when used without -l, correct?
Seems fine as long as it's possible and easy to manually git clone the manifest repo non-recursively and take it from there.

However this all has to be balanced against implementation complexity, other preferences and priorities, "future-proofness" and last but not least limited manpower to deal with it all. For now it seems the person writing 98% of the west code makes the technical decisions (after attentive consultation) and for the coherence and success of the project I hope it stays that way :-)

[mbolivar-nordic]

I think not because west calls git a surprisingly large number of times, so it would be ambiguous "which" git this refers to.

For west status and west diff things should also be clear.

west update is surely a bit more tricky.

Even simple commands like west manifest can run git multiple times for different reasons. Try this from NCS:

$ west --verbose manifest --validate | grep 'running.*git'
west.manifest: running 'git rev-parse --show-cdup' in /home/mbolivar/ncs/zephyr
west.manifest: running 'git ls-tree refs/heads/manifest-rev west.yml' in /home/mbolivar/ncs/zephyr
west.manifest: running 'git show refs/heads/manifest-rev:west.yml' in /home/mbolivar/ncs/zephyr

I think it is better to avoid overgeneralizing right now and stick to specific options like --clone-opt that are clear and explicit about which git they refer to.

[tejlmand]

Even simple commands like west manifest can run git multiple times for different reasons.

Yes, but I was referring to those commands where a user might have an interest in passing additional git options, that is west commands that are closer to be a git wrapper.
I don't consider west manifest to be such type of a command, and thus the reason it was not part of my list.

I didn't say all west command should support this option, more the commands where users might have interest in specifying extra git options could benefit from a common argument name instead of an new arg name for each command.

those were the commands I listed above:

Then we could add this --git-opt to west diff, west update, and west status

and I see those commands as wrapping git behavior (not one to one, more like how git porcelain commands are related to git plumbing commands).

[mbolivar-nordic]

My point is that I don't like the name --git-opt because it is ambiguous. In the west init case, --clone-opt is very specific. I would say we could add --diff-opt to west diff and --status-opt to west status for similar reasons. These are not likely to be difficult to remember even though they are different, because the name of the option is right there in the command name.

For west update, a --git-opt is "just wrong" in my opinion. The west update command in particular runs git a lot of times for different purposes and a generic --git-opt option there is particularly confusing.

[marc-hb]

I think we should handle this issue (i.e. issue #498) by:

• changing west init to use git clone instead

This was done a long time ago in commit e283d99 (July 2021)

• adding a --clone-opt option that users can use to add options to git clone

This was merged today in #744, thank you @usmanimtiaz63

I just tested west init -o=--recursive ... and it works! Closing this.

[marc-hb]

I would say we could add --diff-opt to west diff and --status-opt to west status for similar reasons.

This was done recently, commit 73aee32 from @pdgendt

For west update, a --git-opt is "just wrong" in my opinion. The west update command in particular runs git a lot of times for different purposes and a generic --git-opt option there is particularly confusing.

west update has indeed no generic --git-opt but it got --fetch-opt in your commit bc66a3d