SPM: restore a promoted scalar build setting on deinit - #57744
Open
chrfalch wants to merge 2 commits into
Open
Conversation
`spm add` merges its array build settings (`HEADER_SEARCH_PATHS`, `OTHER_LDFLAGS`, `FRAMEWORK_SEARCH_PATHS`, `LD_RUNPATH_SEARCH_PATHS`) via `addArrayStringValues`, which has three add paths: create the field, append to an existing array, or promote an existing SCALAR into a `( … )` array. Only the first two were reversible. A promotion was recorded as `appendedArrayValues`, whose reversal only strips the injected members — so `spm deinit` left the promoted array and its `"$(inherited)"` seed behind, and the original scalar was never recorded anywhere to restore from. A scalar is the ordinary shape in a real project (`HEADER_SEARCH_PATHS = "$(inherited)";` in the Debug config), so this broke the byte-identical restore `deinit` promises on a common input. Record the pre-injection value in a new `promotedArrayScalars` field on the marker's `BuildSettingChange` and restore it in place. Notes on the details: - The recorded value is kept RAW. `findField`'s token for a bare scalar runs to the `;`, so it carries any whitespace before it, and deinit has to write those bytes back. The value emitted as an array MEMBER is still trimmed — a member with trailing whitespace would be malformed. The two differ deliberately. - The record is gated on the merge having actually changed the text. `addArrayStringValues` no-ops when its value list is empty (as `FRAMEWORK_SEARCH_PATHS` is with no flavored frameworks) or when every value is already present, and a recorded-but-untouched field would have deinit clobber whatever the user has there by then. - Restoration is skipped when the field is absent at deinit time: it was deleted after injection, and re-adding it would resurrect it at the top of the dictionary, matching neither the original nor the user's intent. - Promotion no longer re-emits a prior value that is itself `"$(inherited)"` (the seed) or empty (a bare `,` is not a valid plist element). Reversing a promotion rewrites the whole field, because the injected members and the seed are indistinguishable from the user's own once folded together. Members hand-added to a promoted array afterwards are therefore lost; the removal-helper banner in spm-pbxproj.js now names that exception. Co-Authored-By: Claude Opus 5 <[email protected]>
…tradeoff Review feedback on the promoted-scalar restore: - The seed guard compared the prior scalar against the quoted `"$(inherited)"` only, so an unquoted `$(inherited)` — equally valid, and present in the suite's own untrimmed-scalar fixture — was re-emitted alongside the seed. Deinit still restored it byte-identically (the record is raw), so this was duplication rather than breakage, but it defeated the guard. Compare unquoted, and parametrize the guard's unit test over both spellings so the injected shape is asserted, not just the post-deinit bytes. - The reversal tradeoff is not deinit-only: every re-sync reverts from the recorded baseline before re-injecting, so an `spm update` discards hand-added members just the same. Say so in the banner. Co-Authored-By: Claude Opus 5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
TL;DR —
spm deinitcould not undo one of its own edits, leaving the user'sproject.pbxprojpermanently modified.spm addmerges four array build settings into the app target:HEADER_SEARCH_PATHS,OTHER_LDFLAGS,FRAMEWORK_SEARCH_PATHS,LD_RUNPATH_SEARCH_PATHS.addArrayStringValueshas three add paths — create the field, append to an existing array, or promote an existing scalar into a( … )array. Only the first two were reversible. A promotion was recorded asappendedArrayValues, whose reversal only strips the injected members, sodeinitleft the array shell and its injected"$(inherited)"seed behind — and the original scalar was never recorded anywhere to restore from.Stock projects hit this: Xcode's app template sets
LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks";as a target-level scalar.Fix: record the pre-injection value in a new
promotedArrayScalarsfield on the.spm-injected.jsonmarker, and restore it in place.Four details worth a reviewer's eye:
findField's token for a bare scalar runs to the;, so it carries the whitespace before it anddeinithas to write those bytes back. The value emitted as an array member is still trimmed; a member with trailing whitespace would be malformed. The two differ deliberately.addArrayStringValuesno-ops on an empty value list (FRAMEWORK_SEARCH_PATHSwith no flavored frameworks) or when every value is already present — and a recorded-but-untouched field would havedeinitclobber whatever the user has there by then.deinittime: it was deleted after injection, and re-adding it would resurrect it at the top of the dictionary."$(inherited)"seed — quoted or bare, since the seed test unquotes first — or empty (a bare,is not a valid plist element).Known tradeoff: reversing a promotion rewrites the whole field, because the injected members and the seed are indistinguishable from the user's own once folded together. Members hand-added to a promoted array afterwards are therefore lost. This is not
deinit-only: every re-sync reverts from the recorded baseline before re-injecting, so anspm updatediscards them just the same. The removal-helper banner inspm-pbxproj.jsnames both.The alternative — strip the injected members, then collapse back to a scalar only if the remainder is just the seed — preserves those edits, but has to decide what "just the seed" means across both quotings and any user whitespace. Accepting the loss and documenting it seemed the better trade; happy to revisit.
Changelog:
[Internal] [Fixed] - SwiftPM:
spm deinitnow restores an array build setting that injection promoted from a scalarTest Plan:
yarn jest packages/react-native/scripts/spm/__tests__→ 18 suites, 477 tests (was 462).15 new tests, each written red first:
FRAMEWORK_SEARCH_PATHSas a scalar with no flavored frameworks: untouched by injection, no marker entry, and survivesdeinitafter being edited betweenaddanddeinitaddis not resurrected bydeinitdeinitbytes: the seed guard over both spellings ("$(inherited)"and bare$(inherited)), and the empty valueMarker round-trip was verified across a real process boundary — inject in one
nodeprocess, read.spm-injected.jsonfrom disk,deinitin a second — sincedeinitis always a separate invocation.