sync: fix manifest sync-j handling
Since --jobs defaults to 0, not None, we never pull the value out
of the manifest. Treat values of 0 and None the same to fix.
Bug: http://b/239712300
Bug: http://b/260908907
Change-Id: I9b1026682072366616825fd72f90bd90c10a252f
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/354254
Tested-by: Mike Frysinger <vapier@google.com>
Reviewed-by: Josip Sokcevic <sokcevic@google.com>
Reviewed-by: Sam Saccone <samccone@google.com>
diff --git a/subcmds/sync.py b/subcmds/sync.py
index 0c0f0cf..e4e7a97 100644
--- a/subcmds/sync.py
+++ b/subcmds/sync.py
@@ -1190,6 +1190,45 @@
'release. Use `--auto-gc` instead.', file=sys.stderr)
opt.auto_gc = True
+ def _ValidateOptionsWithManifest(self, opt, mp):
+ """Like ValidateOptions, but after we've updated the manifest.
+
+ Needed to handle sync-xxx option defaults in the manifest.
+
+ Args:
+ opt: The options to process.
+ mp: The manifest project to pull defaults from.
+ """
+ if not opt.jobs:
+ # If the user hasn't made a choice, use the manifest value.
+ opt.jobs = mp.manifest.default.sync_j
+ if opt.jobs:
+ # If --jobs has a non-default value, propagate it as the default for
+ # --jobs-xxx flags too.
+ if not opt.jobs_network:
+ opt.jobs_network = opt.jobs
+ if not opt.jobs_checkout:
+ opt.jobs_checkout = opt.jobs
+ else:
+ # Neither user nor manifest have made a choice, so setup defaults.
+ if not opt.jobs_network:
+ opt.jobs_network = 1
+ if not opt.jobs_checkout:
+ opt.jobs_checkout = DEFAULT_LOCAL_JOBS
+ opt.jobs = os.cpu_count()
+
+ # Try to stay under user rlimit settings.
+ #
+ # Since each worker requires at 3 file descriptors to run `git fetch`, use
+ # that to scale down the number of jobs. Unfortunately there isn't an easy
+ # way to determine this reliably as systems change, but it was last measured
+ # by hand in 2011.
+ soft_limit, _ = _rlimit_nofile()
+ jobs_soft_limit = max(1, (soft_limit - 5) // 3)
+ opt.jobs = min(opt.jobs, jobs_soft_limit)
+ opt.jobs_network = min(opt.jobs_network, jobs_soft_limit)
+ opt.jobs_checkout = min(opt.jobs_checkout, jobs_soft_limit)
+
def Execute(self, opt, args):
manifest = self.outer_manifest
if not opt.outer_manifest:
@@ -1240,35 +1279,9 @@
else:
print('Skipping update of local manifest project.')
- # Now that the manifests are up-to-date, setup the jobs value.
- if opt.jobs is None:
- # User has not made a choice, so use the manifest settings.
- opt.jobs = mp.default.sync_j
- if opt.jobs is not None:
- # Neither user nor manifest have made a choice.
- if opt.jobs_network is None:
- opt.jobs_network = opt.jobs
- if opt.jobs_checkout is None:
- opt.jobs_checkout = opt.jobs
- # Setup defaults if jobs==0.
- if not opt.jobs:
- if not opt.jobs_network:
- opt.jobs_network = 1
- if not opt.jobs_checkout:
- opt.jobs_checkout = DEFAULT_LOCAL_JOBS
- opt.jobs = os.cpu_count()
-
- # Try to stay under user rlimit settings.
- #
- # Since each worker requires at 3 file descriptors to run `git fetch`, use
- # that to scale down the number of jobs. Unfortunately there isn't an easy
- # way to determine this reliably as systems change, but it was last measured
- # by hand in 2011.
- soft_limit, _ = _rlimit_nofile()
- jobs_soft_limit = max(1, (soft_limit - 5) // 3)
- opt.jobs = min(opt.jobs, jobs_soft_limit)
- opt.jobs_network = min(opt.jobs_network, jobs_soft_limit)
- opt.jobs_checkout = min(opt.jobs_checkout, jobs_soft_limit)
+ # Now that the manifests are up-to-date, setup options whose defaults might
+ # be in the manifest.
+ self._ValidateOptionsWithManifest(opt, mp)
superproject_logging_data = {}
self._UpdateProjectsRevisionId(opt, args, superproject_logging_data,
diff --git a/tests/test_subcmds_sync.py b/tests/test_subcmds_sync.py
index 13f3f87..236d54e 100644
--- a/tests/test_subcmds_sync.py
+++ b/tests/test_subcmds_sync.py
@@ -13,11 +13,13 @@
# limitations under the License.
"""Unittests for the subcmds/sync.py module."""
+import os
import unittest
from unittest import mock
import pytest
+import command
from subcmds import sync
@@ -43,6 +45,51 @@
assert cmd._GetCurrentBranchOnly(opts, cmd.manifest) == result
+# Used to patch os.cpu_count() for reliable results.
+OS_CPU_COUNT = 24
+
+@pytest.mark.parametrize('argv, jobs_manifest, jobs, jobs_net, jobs_check', [
+ # No user or manifest settings.
+ ([], None, OS_CPU_COUNT, 1, command.DEFAULT_LOCAL_JOBS),
+ # No user settings, so manifest settings control.
+ ([], 3, 3, 3, 3),
+ # User settings, but no manifest.
+ (['--jobs=4'], None, 4, 4, 4),
+ (['--jobs=4', '--jobs-network=5'], None, 4, 5, 4),
+ (['--jobs=4', '--jobs-checkout=6'], None, 4, 4, 6),
+ (['--jobs=4', '--jobs-network=5', '--jobs-checkout=6'], None, 4, 5, 6),
+ (['--jobs-network=5'], None, OS_CPU_COUNT, 5, command.DEFAULT_LOCAL_JOBS),
+ (['--jobs-checkout=6'], None, OS_CPU_COUNT, 1, 6),
+ (['--jobs-network=5', '--jobs-checkout=6'], None, OS_CPU_COUNT, 5, 6),
+ # User settings with manifest settings.
+ (['--jobs=4'], 3, 4, 4, 4),
+ (['--jobs=4', '--jobs-network=5'], 3, 4, 5, 4),
+ (['--jobs=4', '--jobs-checkout=6'], 3, 4, 4, 6),
+ (['--jobs=4', '--jobs-network=5', '--jobs-checkout=6'], 3, 4, 5, 6),
+ (['--jobs-network=5'], 3, 3, 5, 3),
+ (['--jobs-checkout=6'], 3, 3, 3, 6),
+ (['--jobs-network=5', '--jobs-checkout=6'], 3, 3, 5, 6),
+ # Settings that exceed rlimits get capped.
+ (['--jobs=1000000'], None, 83, 83, 83),
+ ([], 1000000, 83, 83, 83),
+])
+def test_cli_jobs(argv, jobs_manifest, jobs, jobs_net, jobs_check):
+ """Tests --jobs option behavior."""
+ mp = mock.MagicMock()
+ mp.manifest.default.sync_j = jobs_manifest
+
+ cmd = sync.Sync()
+ opts, args = cmd.OptionParser.parse_args(argv)
+ cmd.ValidateOptions(opts, args)
+
+ with mock.patch.object(sync, '_rlimit_nofile', return_value=(256, 256)):
+ with mock.patch.object(os, 'cpu_count', return_value=OS_CPU_COUNT):
+ cmd._ValidateOptionsWithManifest(opts, mp)
+ assert opts.jobs == jobs
+ assert opts.jobs_network == jobs_net
+ assert opts.jobs_checkout == jobs_check
+
+
class GetPreciousObjectsState(unittest.TestCase):
"""Tests for _GetPreciousObjectsState."""
