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

Shallow and concurrent git clones #10254

Merged
merged 2 commits into from
Nov 17, 2024

Conversation

alt-romes
Copy link
Collaborator

@alt-romes alt-romes commented Aug 12, 2024

Please read Github PR Conventions and then fill in one of these two templates.


Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Manual QA notes have been included.
  • Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

Manual QA: Create or use a package with multiple source-repository-package dependencies, cabal clean, then cabal build. You should see two repositories start being cloned at the same time, and then see other repositories being cloned concurrently as jobs are finished. The reason why only two repositories get cloned in parallel is that the limit of asynchronous downloads is == 2, to mimic asyncFetchPackages.


Questions: The maximum two-job cap on cloning concurrency seems too low, I think it would be best if it could be configured (as a follow up MR?)

@alt-romes alt-romes force-pushed the wip/romes/cabal-shallow-mr branch 3 times, most recently from 844e1bb to fd373ad Compare August 12, 2024 11:07
@alt-romes alt-romes changed the title Wip/romes/cabal shallow mr Shallow and concurrent git clones Aug 12, 2024
@mpickering
Copy link
Collaborator

This change also affects cabal get -s, that needs documenting if it's intentional.

@alt-romes alt-romes force-pushed the wip/romes/cabal-shallow-mr branch 7 times, most recently from 0416a52 to 85b2a5c Compare November 8, 2024 10:35
@alt-romes
Copy link
Collaborator Author

This is ready

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

cabal-install/src/Distribution/Client/ProjectConfig.hs Outdated Show resolved Hide resolved
cabal-install/src/Distribution/Client/VCS.hs Outdated Show resolved Hide resolved
cabal-install/src/Distribution/Client/ProjectConfig.hs Outdated Show resolved Hide resolved
@alt-romes
Copy link
Collaborator Author

I've addressed your review @ulysses4ever. Merging this now!

@alt-romes alt-romes self-assigned this Nov 11, 2024
@alt-romes alt-romes added the merge me Tell Mergify Bot to merge label Nov 11, 2024
@mergify mergify bot added the ready and waiting Mergify is waiting out the cooldown period label Nov 11, 2024
@ulysses4ever ulysses4ever linked an issue Nov 11, 2024 that may be closed by this pull request
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Nov 13, 2024
@ulysses4ever
Copy link
Collaborator

@alt-romes can you resolve the conflicts?

Cloning the entire repository for the purpose of compiling packages
specified in source-repository-packages is wasted effort. To read and
compile the package, we need only the HEAD of the repository, thus a
shallow clone is sufficient.

Note that this doesn't change the behaviour of `cabal get -s` which
still does a full clone (--depth=1 is only used in vcsSyncRepo, not in
vcsCloneRepo)

Fixes haskell#7264
Cloning/synchronising VCS repos can be unnecessarily slow if done
serially. By synchronizing the repos concurrently we make much better
use of time.

Introduces rerunConcurrentlyIfChanged, a Rebuild monad function that
runs, from multiple actions, the actions that need rebuilding concurrently.
@alt-romes alt-romes added merge me Tell Mergify Bot to merge and removed merge me Tell Mergify Bot to merge labels Nov 16, 2024
@mergify mergify bot merged commit c3a9dd7 into haskell:master Nov 17, 2024
52 checks passed
@9999years
Copy link
Collaborator

Hi, I'm working on the VCS test suite and (while running it in verbose mode) I noticed this:

Running: git -c 'protocol.file.allow=always' clone '--depth=1' --no-checkout /private/var/folders/z5/fclwwdms3r1gq4k4p3pkvvc00000gn/T/vcstest-89726/src /private/var/folders/z5/fclwwdms3r1gq4k4p3pkvvc00000gn/T/vcstest-89726/dest1
Cloning into '/private/var/folders/z5/fclwwdms3r1gq4k4p3pkvvc00000gn/T/vcstest-89726/dest1'...
warning: --depth is ignored in local clones; use file:// instead.

As a result, I'm not sure this feature is actually being tested.

When I patched the test suite to use the file:// URLs as suggested by the warning, I found a failure:

Running: git --version
Running: git -c 'protocol.file.allow=always' init
Initialized empty Git repository in /private/var/folders/z5/fclwwdms3r1gq4k4p3pkvvc00000gn/T/vcstest-60316/src/.git/
Running: git -c 'protocol.file.allow=always' add file/C
Running: git -c 'protocol.file.allow=always' -c 'user.name=A' -c '[email protected]' commit --all '--message=a patch' '--author=A <[email protected]>'
[master (root-commit) 569c51c] a patch
 1 file changed, 1 insertion(+)
 create mode 100644 file/C
Running: git -c 'protocol.file.allow=always' log '--format=%H' -1
Running: git -c 'protocol.file.allow=always' tag --force --no-sign tag_XFUS

--------------------------------------------------------------------------------

Running: git -c 'protocol.file.allow=always' clone '--depth=1' --no-checkout 'file:///private/var/folders/z5/fclwwdms3r1gq4k4p3pkvvc00000gn/T/vcstest-60316/src' /private/var/folders/z5/fclwwdms3r1gq4k4p3pkvvc00000gn/T/vcstest-60316/dest1
Cloning into '/private/var/folders/z5/fclwwdms3r1gq4k4p3pkvvc00000gn/T/vcstest-60316/dest1'...
remote: Enumerating objects: 4, done.
remote: Counting objects: 100% (4/4), done.
remote: Total 4 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
Receiving objects: 100% (4/4), done.
Running: git -c 'protocol.file.allow=always' submodule deinit --force --all
Running: git -c 'protocol.file.allow=always' fetch origin tag_XFUS
From file:///private/var/folders/z5/fclwwdms3r1gq4k4p3pkvvc00000gn/T/vcstest-60316/src
 * tag               tag_XFUS   -> FETCH_HEAD
Running: /Users/wiggles/.nix-profile/bin/git -c 'protocol.file.allow=always' reset --hard FETCH_HEAD --
HEAD is now at 569c51c a patch
Running: git -c 'protocol.file.allow=always' clean -ffxdq
Running: git -c 'protocol.file.allow=always' clone '--depth=1' --no-checkout 'file:///private/var/folders/z5/fclwwdms3r1gq4k4p3pkvvc00000gn/T/vcstest-60316/src' /private/var/folders/z5/fclwwdms3r1gq4k4p3pkvvc00000gn/T/vcstest-60316/dest2 --reference /private/var/folders/z5/fclwwdms3r1gq4k4p3pkvvc00000gn/T/vcstest-60316/dest1
Cloning into '/private/var/folders/z5/fclwwdms3r1gq4k4p3pkvvc00000gn/T/vcstest-60316/dest2'...
fatal: reference repository '/private/var/folders/z5/fclwwdms3r1gq4k4p3pkvvc00000gn/T/vcstest-60316/dest1' is shallow

@9999years 9999years mentioned this pull request Nov 22, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

source-repository-package generates fat clones
5 participants