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

Consider adding config option for treeless clones (--fetch-opt=--filter=...) #638

Closed
rgilton opened this issue Mar 1, 2023 · 16 comments
Closed
Labels
performance How long things take

Comments

@rgilton
Copy link

rgilton commented Mar 1, 2023

When running west in CI, it's useful to be able to minimise the amount of data that a job has to download. The clone-depth setting in manifests allows git clones to be "shallow", but this doesn't work with all git hosts -- it's not always possible to shallow clone an arbitrary commit hash. What makes this even worse is that what could be shallow-cloned yesterday cannot necessarily be shallow-cloned today -- some git hosts permit shallow cloning on the last N commits -- this makes a build that was working one day fail the next, even if the local project has not been changed.

A solution to this may be to add a config option to permit the use of "treeless" clones (more info here). Or perhaps even just allowing a git filter to be specified for maximum flexibility.

cc:

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 1, 2023

Or perhaps even just allowing a git filter to be specified for maximum flexibility.

Did you try west update --fetch-opt ... ?

Also take a look at https://github.com/zephyrproject-rtos/west/issues?q=shallow+

EDIT: and now the better https://github.com/zephyrproject-rtos/west/issues?q=label%3Aperformance

@rgilton
Copy link
Author

rgilton commented Mar 2, 2023

Ah nice, hadn't seen the --fetch-opt argument! 👍

Also take a look at https://github.com/zephyrproject-rtos/west/issues?q=shallow+

Yea, I'd looked at the shallow clone options, but shallow cloning isn't viable for git hosts that don't allow cloning arbitrary commit hashes (github).

@rgilton rgilton closed this as completed Mar 2, 2023
@mbolivar-nordic
Copy link
Contributor

git hosts that don't allow cloning arbitrary commit hashes (github).

Github does allow this now

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 2, 2023

https://github.com/zephyrproject-rtos/west/issues?q=shallow+

Yea, I'd looked at the shallow clone options,

These discussions were not just about shallow options. Some of these discussions may be obsolete now with these new (to me!) --filter= options. I finally took the time to read the https://github.blog/2020-12-21-get-up-to-speed-with-partial-clone-and-shallow-clone/ page you shared and --filter= looks impressive! Will have to try it soon, thanks for sharing.

Ah nice, hadn't seen the --fetch-opt argument!

So are you confirming that --fetch-opt=--filter= ... works with the initial cloning too? I'm ashamed to admit I keep forgetting how west performs the initial cloning... I know it changed in e283d99 but that was only for the manifest which you can of course easily clone any way you want and bootstrap west before invoking west. So manifest optimizations are a non-issue, off-topic here.

To be honest I'm not a fan of west update doing BOTH git clone and git fetch; if these had been separate then #519 would never have been a problem and maybe others in https://github.com/zephyrproject-rtos/west/issues?q=is%3Aissue++label%3A%22Partial+imports%22+ would have not been either. west update is too much "abstraction" IMHO.

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 2, 2023

but shallow cloning isn't viable for git hosts that don't allow cloning arbitrary commit hashes (github).

I recently noticed that west update --narrow is incompatible with direct SHA fetch from github, maybe that's why it didn't work for you? I'm sure there's a very good reason why but again I keep getting lost in how west update works, too much combinatorial explosion :-)

At the risk of stating the obvious: don't forget to fetch the FULL, 40-digit long SHA.

This is a bit confusing BTW:

west update -h

  -n, --narrow          fetch just the project revision if fetching is necessary;
                        do not pass --tags to git fetch (may not work for SHA
                        revisions depending on the Git host)

It's confusing because Github does support allowAnySHA1InWant, yet --narrow breaks it for some reason.

@mbolivar-nordic
Copy link
Contributor

mbolivar-nordic commented Mar 2, 2023 via email

@mbolivar-nordic
Copy link
Contributor

mbolivar-nordic commented Mar 2, 2023 via email

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 2, 2023

Wait, maybe it's the other way round. Need to test this again. It's either one or the other for sure:

We added --narrow precisely because github supported direct sha fetch.

That wasn't my understanding, I saw --narrow as a way to avoid fetching many unused branches and tags

Some projects generate branches and tags automatically and have hundreds of them:
https://opensource.googleblog.com/2018/05/introducing-git-protocol-version-2.html

@mbolivar-nordic
Copy link
Contributor

mbolivar-nordic commented Mar 2, 2023 via email

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 3, 2023

Why try to use my brain and remember things correctly when I can just dump my thoughts into github projects instead?

Other jobs typically use 'west update --narrow' which is faster but
also able to fetch "wild" SHA1s from any random place! --narrow
is useful for testing unmerged Zephyr commits but risks
accepting "invalid" ones

Just confirmed in https://github.com/thesofproject/sof/actions/runs/4319928351/jobs/7539643427

@mbolivar-nordic
Copy link
Contributor

@marc-hb I believe your above comment confirms that you can fetch SHAs directly from GitHub.

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 6, 2023

Yes but ONLY with --narrow. My memories were "flipped".

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 10, 2023

Wait, west update --narrow SHA fails only in Github Actions; I cannot reproduce locally! It's also an incredibly cryptic error:

https://github.com/thesofproject/sof/actions/runs/4374649912/jobs/7654345972

From https://github.com/juimonen/zephyr
 * branch              5eade984fff40f11c9711d96[37](https://github.com/thesofproject/sof/actions/runs/4374649912/jobs/7654345972#step:3:38)1654182f68061f -> FETCH_HEAD
HEAD is now at 5eade984 soc: intel_adsp: cavs: start using zephyr power management
HEAD is now at 5eade984 soc: intel_adsp: cavs: start using zephyr power management
...
fatal: expected 'acknowledgments'

None of that makes sense. But I've seen this error consistently in the same conditions.

(yes I should file a new bug. I might)

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 22, 2023

So are you confirming that --fetch-opt=--filter= ... works with the initial cloning too?

So apparently "no news is good news" - in this case excellent news even. I tried this myself and I was very pleased and surprised to find that it works. I guess that's why this issue was closed :-)

I'm ashamed to admit I keep forgetting how west performs the initial cloning...

So unlike the manifest after commit e283d99 , subprojects are still cloned in two steps: init + fetch. While --filter= is barely mentioned in git help fetch (it's documented in git help clone), it does work with and affect git fetch. tl;dr: it works with west update -o.

I'd be curious to know if there's any low-hanging fruit there if you
find the time.

Not just "it works" but the results are very impressive. "Impressive" as in: stop what you're doing now and try it in your CI right away.

From my system inside the company network (behind proxies and what not)

west init -m https://github.com/thesofproject/sof

40- seconds   west update    zephyr
35+ seconds   west update --narrow   zephyr
15  seconds   west update --fetch-opt=--filter=tree:0    zephyr
15  seconds   west update --fetch-opt=--filter=tree:0  --narrow zephyr
# evil and shallow clone
33 seconds    west update --fetch-opt=--depth=1  zephyr

Evil and... fails!

west.manifest: "git fetch -f --tags --depth=1 -- https://github.com/zephyrproject-rtos/zephyr 'refs/heads/*:refs/west/*'" exit code: 0 stdout: None stderr: None
west.manifest: running 'git rev-parse 'e3ae110a05d3427647a03fcbeff4478c195c5a48^{commit}'' in /var/home/mherber2/SOFproj/zephyr
west.manifest: "git rev-parse 'e3ae110a05d3427647a03fcbeff4478c195c5a48^{commit}'" exit code: 128 stdout: b'e3ae110a05d3427647a03fcbeff4478c195c5a48^{commit}\n' stderr: b"fatal: ambiguous argument 'e3ae110a05d3427647a03fcbeff4478c195c5a48^{commit}': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n"
ERROR: update failed for project zephyr

Does not seem compatible with https://github.com/actions/checkout though :-(

marc-hb added a commit to marc-hb/sof that referenced this issue Mar 22, 2023
Test new, fancy and impressive looking git --filter optimization on
non-critical "manifest check". Extend it later to other checks if it
hasn't caused any issue.

https://github.blog/2020-12-21-get-up-to-speed-with-partial-clone-and-shallow-clone/
zephyrproject-rtos/west#638 (comment)

Signed-off-by: Marc Herbert <[email protected]>
kv2019i pushed a commit to thesofproject/sof that referenced this issue Mar 23, 2023
Test new, fancy and impressive looking git --filter optimization on
non-critical "manifest check". Extend it later to other checks if it
hasn't caused any issue.

https://github.blog/2020-12-21-get-up-to-speed-with-partial-clone-and-shallow-clone/
zephyrproject-rtos/west#638 (comment)

Signed-off-by: Marc Herbert <[email protected]>
@marc-hb
Copy link
Collaborator

marc-hb commented Mar 23, 2023

Does not seem compatible with https://github.com/actions/checkout though :-(

Please upvote:

@mbolivar-nordic
Copy link
Contributor

mbolivar-nordic commented Mar 27, 2023 via email

marc-hb added a commit to marc-hb/sof that referenced this issue Apr 24, 2023
Twice faster than shallow clones without any of the git describe or
other hassle. More info in
zephyrproject-rtos/west#638

Signed-off-by: Marc Herbert <[email protected]>
kv2019i pushed a commit to thesofproject/sof that referenced this issue Apr 25, 2023
Twice faster than shallow clones without any of the git describe or
other hassle. More info in
zephyrproject-rtos/west#638

Signed-off-by: Marc Herbert <[email protected]>
@marc-hb marc-hb added the performance How long things take label May 5, 2023
@marc-hb marc-hb changed the title Consider adding config option for treeless clones Consider adding config option for treeless clones (--fetch-opt=--filter=...) May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance How long things take
Projects
None yet
Development

No branches or pull requests

3 participants