Repository navigation
Conversation
jwinarske
left a comment
There was a problem hiding this comment.
Does what #238 asked and the reasoning is sound. Verified on the branch: analyze clean, format clean, 1059 tests pass. I also checked the mechanics by hand rather than trusting the tests:
- A batched
chmod 0640 f0 f1 f2really applies to every path (modes came back640,640,640). - Measured here: 2000 individual
chmodspawns ≈ 613 ms of pure fork/exec, one batched call over 2000 paths ≈ 5 ms. Through Dart'sProcess.runthe per-spawn cost is ~3x a shell's, which is where #238's ~6.5 s figure came from — so the win is real at both measurements. - The repeated-destination carve-out behaves: an extra file whose dest collides with the binary install path still gets
0755then0600in that order, and the staged content is the extra file's. Same end state as main. - Byte+count bounding is conservative in the right direction (Linux allows ~128 KB per single argument and ~2 MB total), and the
args.length > 1guard means a single over-long path still goes out in its own call rather than being dropped.
Two suggestions inline, neither blocking. Nothing here regresses behavior, so I'm approving.
jwinarske
left a comment
There was a problem hiding this comment.
CI green — 39 pass, 1 skipping. Re-approving for 8df4e6a, which does both things I suggested and folds in the two maintenance notes.
Verified by running it rather than reading the diff:
argv for an option-shaped mode: [chmod, --, --reference=/etc/shadow, <staged>]
real chmod with -- : exit 1, "chmod: invalid mode: '--reference=/etc/shadow'"
chmod failure reported : chmod failed (exit 1): chmod: bad mode
So the -- separator plus the new exit-code check together turn #235's stolen-mode vector into a hard build failure instead of a silent chmod against a stolen reference — better than the "removes the amplification" I asked for. chmod -- 0644 f, chmod -- -w f and chmod -- a-w f all still work, so octal and symbolic modes are unaffected.
Also in: the duplicated p.join(root.path, X.substring(1)) is now one stagedPath helper, which removes the drift risk I flagged, and the dead Platform.isWindows branch is gone.
Gates locally on this head: analyze clean, format clean, 1060 tests.
No rebase needed — 28 commits behind main but no file touched on both sides.
Merging this closes #238.
|
Thanks @shin4141 ! |
What this fixes
Fixes #238.
PackageStager.stagePayloadgroups staged extra-file destinations by mode and runschmodonce per bounded argv chunk, instead of once per file. Repeated destinations keep their original copy/chmodorder.Why it matters
expandPackageFilesassigns a mode to each swept file, including files without an explicit manifest mode. The existing staging loop therefore starts achmodprocess for every such file. This change makes the process-invocation count scale with the number of modes and argv chunks rather than the number of files. It does not change file expansion or claim a measured wall-clock speedup.Review scope
lib/src/cross/package_stager.dart: batch unique destinations by mode; cap each invocation at 1,024 paths and 96 KiB of UTF-8 arguments; pass--before the mode, fail on a nonzerochmodexit, and reuse one staged-path helper.test/src/cross/package_stager_test.dart: cover many files and modes, bounded argv, a 2,000-file single-mode case, copied content, repeated-destination order, and failedchmod.test/src/cross/deb_packager_test.dart: update thechmodargv expectation.Evidence
main(a1d4bc5489a171b6013bc2c741348c0d5c4200ed): the regression observed 960 extrachmodcalls for 960 mode-bearing files and 2,000 calls for 2,000 files.chmodcalls.dart analyzereports no issues for the three changed files;dart formatreports no changes.