Add scripts for sparse checkout of some contribs #48183

tavplubix · 2023-03-29T16:48:09Z

Changelog category (leave one):

Not for changelog (changelog entry is not required)

See https://git-scm.com/docs/git-sparse-checkout

Before:

du -hs contrib
4.6G	contrib
find contrib -type f | wc -l
357344

After:

du -hs contrib
1.6G	contrib
find contrib -type f | wc -l
91082

It does not reduce network usage when cloning and does not reduce the sizes of .git/modules/contrib/* dirs. But it's still useful and can make some IDEs work faster. We should exclude more files from contrib.

Felixoid · 2023-04-04T09:33:09Z

I've tested it as the following:

$ git clone git@github.com:ClickHouse/ClickHouse.git --depth=1 --single-branch --branch=contrib_sparse_checkout ClickHouse-sparse
$ cd ClickHouse-sparse
# the new command
$ time ./contrib/update-submodules.sh
.....
./contrib/update-submodules.sh  61,69s user 12,43s system 21% cpu 5:50,28 total
$ du -hscx .git/modules contrib
1,1G	.git/modules
1,6G	contrib
2,7G	total
# remove the repo and apply the command from https://github.com/ClickHouse/checkout/
$ git clone git@github.com:ClickHouse/ClickHouse.git --depth=1 --single-branch --branch=contrib_sparse_checkout ClickHouse-sparse
$ time (
  git -C ClickHouse-sparse submodule sync && \
  git -C ClickHouse-sparse submodule init && \
  git config --file ClickHouse-sparse/.gitmodules --null --get-regexp path | \
    sed -z 's|.*\n||' | \
    xargs --max-procs=100 --null --no-run-if-empty --max-args=1 \
      git -C ClickHouse-sparse submodule update --depth=1 --single-branch
)
.....
( LC_ALL=en_US.UTF-8 git -C ClickHouse-sparse submodule sync && LC_ALL= git -)  71,25s user 16,78s system 92% cpu 1:35,53 total
$ du -hscx .git/modules contrib
1,1G	.git/modules
4,5G	contrib
5,6G	total

As far as I get, the only thing the submodule.<name>.update does is ignore the paths from checking out. But we still have them in the .git/modules AKA still download them. For the CI the changes are unnecessary since the ClickHouse/checkout is already optimized from the checkout time perspective.

From an end-user usability perspective, I couldn't really say. Yes, 3GB is a quite significant difference.

Felixoid · 2023-04-04T09:35:29Z

.github/workflows/pull_request.yml

@@ -550,6 +550,13 @@ jobs:
        with:
          clear-repository: true
          submodules: true
+      - name: Apply sparse checkout for contrib # in order to check that it doesn't break build


Please, don't forget to roll it back. Unfortunately, it doesn't increase the checkout speed, it only decreases the allocated disk space.

I had no intention of rolling it back, I'm going to keep it. See the comment below

It is necessary for at least one build. Because we must check that it works. Imagine that some contributor cloned the repo with "sparse checkout" following the documentation. And another contributor has full contribs. The second contributor adds some code that uses a new boost module, for example. It compiles on their machine. It compiles in the CI.

I see. In this case, the job is done very well. We have all submodules cloned in the checkout step before, and the following test is done faster.

Maybe, if we do it in BuilderDebDebug, it makes sense to adjust every workflow having it?

.github/workflows/backport_branches.yml

.github/workflows/master.yml

.github/workflows/release_branches.yml

Maybe, if we do it in BuilderDebDebug, it makes sense to adjust every workflow having it?

I think that having it in pull_request.yml only is enough to avoid breaking the build with "sparse checkout". But I don't have a strong opinion on that, we can add it to all workflows for consistency.

Also it makes sense to add an option to use update-submodules.sh in https://github.com/ClickHouse/checkout (and avoid the ugly trick with rm -rf), but it will make the action less universal

@Felixoid, I added the same change to other workflows

P.S. It would be great to refactor these yamls somehow and reduce the amount of copy-paste

docs/ru/development/developer-instruction.md

docs/en/development/developer-instruction.md

tavplubix · 2023-04-04T15:19:34Z

As far as I get, the only thing the submodule..update does is ignore the paths from checking out.

Yep.

But we still have them in the .git/modules AKA still download them.

Yep, that's written in the PR description:

It does not reduce network usage when cloning and does not reduce the sizes of .git/modules/contrib/* dirs. But it's still useful and can make some IDEs work faster. We should exclude more files from contrib.

But it's worth mentioning that git/modules/contrib/* contain compressed files. For example, .git/modules/contrib/llvm-project is only 265MB (with --depth=1) while contrib/llvm-project is 1.5GB.

For the CI the changes are unnecessary

It is necessary for at least one build. Because we must check that it works. Imagine that some contributor cloned the repo with "sparse checkout" following the documentation. And another contributor has full contribs. The second contributor adds some code that uses a new boost module, for example. It compiles on their machine. It compiles in the CI.

But then the PR from the second contributor is merged and the first contributor cannot build ClickHouse anymore because the scripts in contrib/sparse-checkout/ are stale.

Having "sparse checkout" in the CI avoids issues like this.

From an end-user usability perspective, I couldn't really say. Yes, 3GB is a quite significant difference.

My build directory is almost 50GB. I don't really care about 3GB of disk space, it's not a big difference. The problem is that an IDE may index all code in contrib/ and it may slow down the IDE significantly. Also, having less garbage in contrib/ is good for searching in code (it will search faster and will find fewer irrelevant results).

Also it's useful for understanding actual dependencies:

ClickHouse/contrib/sparse-checkout/update-grpc.sh

Lines 13 to 14 in 686b284

    
           # FIXME why do we need csharp? 
        
           #echo '!/src/csharp/*' >> $FILES_TO_CHECKOUT

(probably we use it by mistake and it has to be fixed)

tavplubix · 2023-04-11T13:15:54Z

Stateless tests (debug) [2/5] - #48574

Felixoid · 2023-05-22T12:59:09Z

.github/workflows/backport_branches.yml

@@ -349,6 +349,13 @@ jobs:
        with:
          clear-repository: true
          submodules: true
+      - name: Apply sparse checkout for contrib # in order to check that it doesn't break build


I suggest reverting this part in the CI.

Besides the fact, that it only does the additional work after the modules are already checked out, this part has broken the debug build https://github.com/ClickHouse/ClickHouse/actions/runs/5033995222/jobs/9028616459#step:7:3663

cc @nickitat

upd: it's only a suggestion though. Fixing in the same PR works too. But it's better to backport it too

@Felixoid, that's exactly why I added this to the CI. Changes in #50037 break sparse checkout, so it must be fixed in #50037 before merging.

it makes sense to check this script in CI, but maybe as a separate step with self-explanatory name. otherwise people will just waste time over and over again because obviously most of them have no clue about this logic. and they shouldn't.

Agree with Nikina. It's unclear now what is broken there, and why it differs from other steps

Besides the fact that it's good to have a good explanation of failure, we should use one of the fastest builds instead of a long one

What do you think of BuilderBinRelease as the job to test the sparse checkout? We can rename it explicitly to BuilderBinReleaseSparseSubmodules

Yes, it was unobvious that we use sparse checkout for debug build only, sorry for that. But I don't like introducing another build just to check that sparse checkout works, because it will actually duplicate an ordinary build for no good reason. I would prefer to add some note about this to the report.

we should use one of the fastest builds instead of a long one

It does not matter, sparse checkout does not make a build slower, it just takes a few seconds to re-checkout submodules, and after that build continues as usual.

What do you think of BuilderBinRelease as the job to test the sparse checkout?

Release build is slower than debug build.

Release deb yes, but I am talking regarding the release binary build. See the report, 0:16:20 for the BuilderBinRelease vs 0:20:05 for BuilderDebDebug

I am speaking not about adding a new check, but renaming the existing one to reflect what we actually check there.

tavplubix added 2 commits March 29, 2023 17:03

add scripts for sparse checkout of some contribs

499b58d

update docs

905975f

robot-clickhouse added pr-not-for-changelog This PR should not be mentioned in the changelog submodule changed At least one submodule changed in this PR. labels Mar 29, 2023

tavplubix added 7 commits March 31, 2023 16:10

Merge branch 'master' into contrib_sparse_checkout

2ab198f

check sparse checkout in CI

ee3da38

exclude more subdirs

8169f01

Merge branch 'master' into contrib_sparse_checkout

c4c3602

fix

409c8e3

Update pull_request.yml

043729e

fix

686b284

antonio2368 self-assigned this Apr 3, 2023

Felixoid reviewed Apr 4, 2023

View reviewed changes

tavplubix added 2 commits April 7, 2023 23:44

Merge branch 'master' into contrib_sparse_checkout

3c50921

fix

3633f0b

antonio2368 approved these changes Apr 11, 2023

View reviewed changes

antonio2368 assigned Felixoid Apr 11, 2023

tavplubix merged commit 327bfbc into master Apr 11, 2023
148 checks passed

tavplubix deleted the contrib_sparse_checkout branch April 11, 2023 13:18

Felixoid reviewed May 22, 2023

View reviewed changes

tavplubix mentioned this pull request May 24, 2023

Add comments to build reports #50200

Merged

Felixoid added a commit that referenced this pull request Oct 9, 2023

Revert sparse-checkout, the #48183 is not backported

a23369c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scripts for sparse checkout of some contribs #48183

Add scripts for sparse checkout of some contribs #48183

tavplubix commented Mar 29, 2023 •

edited

Felixoid commented Apr 4, 2023

Felixoid Apr 4, 2023 •

edited

tavplubix Apr 4, 2023

Felixoid Apr 4, 2023

tavplubix Apr 4, 2023

tavplubix Apr 7, 2023

tavplubix commented Apr 4, 2023 •

edited

tavplubix commented Apr 11, 2023

Felixoid May 22, 2023 •

edited

tavplubix May 22, 2023

nickitat May 22, 2023

Felixoid May 22, 2023

Felixoid May 22, 2023 •

edited

tavplubix May 22, 2023

Felixoid May 22, 2023 •

edited

tavplubix May 24, 2023

Add scripts for sparse checkout of some contribs #48183

Add scripts for sparse checkout of some contribs #48183

Conversation

tavplubix commented Mar 29, 2023 • edited

Changelog category (leave one):

Felixoid commented Apr 4, 2023

Felixoid Apr 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tavplubix commented Apr 4, 2023 • edited

tavplubix commented Apr 11, 2023

Felixoid May 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Felixoid May 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Felixoid May 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tavplubix commented Mar 29, 2023 •

edited

Felixoid Apr 4, 2023 •

edited

tavplubix commented Apr 4, 2023 •

edited

Felixoid May 22, 2023 •

edited

Felixoid May 22, 2023 •

edited

Felixoid May 22, 2023 •

edited