speed up init by performing a shallow clone #47430
Conversation
|
|
||
| // check if we want to perform a shallow clone with --no-history | ||
| // check if ref is advertised by github (ls-remote) | ||
| // If it isn't we need to perform a full clone or download an archive |
There was a problem hiding this comment.
nit: lowercase if to be consistent with casing in comments above
|
This repo is deprecated, we shouldn’t be adding any new code here |
| @@ -0,0 +1,180 @@ | |||
| { | |||
There was a problem hiding this comment.
Why would this be committed to the repo?
There was a problem hiding this comment.
I think this is probably unintentionally added?
| // check if we want to perform a shallow clone with --no-history | ||
| // check if ref is advertised by github (ls-remote) | ||
| // If it isn't we need to perform a full clone or download an archive | ||
| const shouldShallowCloneBraveCore = ( |
There was a problem hiding this comment.
We don’t really need to use brave-browser at all anymore and at some point we’d still like to move everything into a single repo so I don’t think we want to add anything here. Does brave-core checkout actually take a long time? I think you’re basically free to checkout brave core any way you want so no need for anything in a dedicated script. This is mostly still here for historical reasons.
There was a problem hiding this comment.
I agree, we should have only one repo, just added this for completeness.
Does brave-core checkout actually take a long time? I think you’re basically free to checkout brave core any way you want so no need for anything in a dedicated script. This is mostly still here for historical reasons.
A full clone is 3.57Gb. But yes the brave-core can be aquired in any way as long there is a git repo with a previous commit.
At this point we just need to update docs and remove a few dependendies on the folder structure eg.: brave-core's npm run init needs to be tweaked to work in a standalone brave-core checkout as it currently runs: cd ../../ && npm run --prefix src/brave sync -- --init.
| noHistory | ||
| && util | ||
| .runGit('.', ['ls-remote', braveCoreUrl], false, {maxBuffer: 1024 * 1024 * 5}) | ||
| .includes(braveCoreRef) |
There was a problem hiding this comment.
Could you add more context on why --depth=2 is needed? Specifically, which script is failing and what is the error? This would be very helpful for future developers who might need to debug this part of the code.
| // check if ref is advertised by github (ls-remote) | ||
| // If it isn't we need to perform a full clone or download an archive | ||
| const shouldShallowCloneBraveCore = ( | ||
| noHistory |
There was a problem hiding this comment.
Is the maxBuffer of 5MB an arbitrary number or is it based on the expected size of the ls-remote output? A comment explaining the reasoning behind this value would be useful.
There was a problem hiding this comment.
Semi random pick. Big enough for now and for the future. I think it is suggestive that might want to reduce the amount of branches we have in brave-core...
| // If it isn't we need to perform a full clone or download an archive | ||
| const shouldShallowCloneBraveCore = ( | ||
| noHistory | ||
| && util |
There was a problem hiding this comment.
The code doesn't seem to handle the case where ls-remote fails. If runGit returns null (which it does on failure), the subsequent call to .includes() will throw a TypeError. It would be safer to check the result of runGit for null before attempting to call .includes() on it.
|
|
||
| if (!fs.existsSync(path.join(braveCoreDir, '.git'))) { | ||
| Log.status(`Cloning brave-core [${braveCoreRef}] into ${braveCoreDir}...`) | ||
| fs.mkdirSync(braveCoreDir) | ||
| util.runGit(braveCoreDir, ['clone', util.getNPMConfig(['projects', 'brave-core', 'repository', 'url']), '.']) | ||
|
|
||
| // check if we want to perform a shallow clone with --no-history |
There was a problem hiding this comment.
Instead of using ls-remote to check if the ref is advertised, have you considered trying a shallow clone first and then falling back to a full clone if it fails? This would avoid the overhead of ls-remote, which can be slow, and would only incur the cost of a failed git clone in the rare case where a shallow clone is not possible for the given ref.
There was a problem hiding this comment.
I think this is a good idea. Will try
Resolves: #6444
related brave-core pr: brave/brave-core#29938