Add support for xcuserdata #739
Conversation
| @@ -45,9 +45,9 @@ | |||
| buildConfiguration = "Debug" | |||
There was a problem hiding this comment.
When adding tests that would read and then write schemes, breakpoints and a xcschememanagement.plist back to disk the tests failed because these changes popped up in the git diff. So I've added them to make sure that when these files are read in and written back to disk there is no diff. I think this is the correct behaviour.
|
|
||
| /// Returns schemes folder path relative to the given path. | ||
| /// | ||
| /// - Parameter path: parent folder of schemes folder (xcshareddata or xcuserdata) | ||
| /// - Returns: schemes folder path relative to the given path. | ||
| public static func schemesPath(_ path: Path) -> Path { |
There was a problem hiding this comment.
These methods now need the xcshareddata or xcuserdata path instead of the .xcodeproj path because they need to it to produce a relative path to that folder instead of a path always relative to the xcshareddata folder.
public static func schemesPath(path:)
public static func schemePath(path:schemeName:)
public static func debuggerPath(path:)
public static func breakPointsPath(path:)
public static func schemeManagementPath(path:) <-- New
The same goes for the writing methods, they need a path to the xcshareddata or xcuserdata folder instead of the .xcodeproj path.
public static func writeSchemes(schemes:path:override:)
public static func writeBreakPoints(breakpoints:path:override:)
| initModel: { try? XCUserData(path: $0) }, | ||
| modify: { $0 }, | ||
| assertion: { | ||
| assert(userData: $1, userName: "copy") |
There was a problem hiding this comment.
testWrite copies the username1.xcuserdatad folder over to <tmpDir>copy.xcuserdatad thus changing the userName of the XCUserData to copy...
teameh
force-pushed
the
xcuserdata-support
branch
from
b22dc60
to
0f1861e
Compare
There was a problem hiding this comment.
Thank you for contributing this @teameh , the additional notes and tests were much appreciated to help review this 🙏
I think many of the pieces here are valuable additions, we'll need to figure out how to integrate it without introducing functional or source breaking changes.
| /// - Throws: An error if the write fails. | ||
| public func write(path: Path) throws { | ||
| public func write(path: Path, override: Bool) throws { |
There was a problem hiding this comment.
note: this introduces a breaking change, we'll need to offer a default value for override
e.g.
| try writeSchemes(path: path, override: override) | ||
| try writeBreakPoints(path: path, override: override) | ||
| try writeSharedData(path: path, override: override) | ||
| try writeUserData(path: path, override: override) |
There was a problem hiding this comment.
This sadly introduces a few functional breaking changes ...
- Any user specified local schemes will get wiped whenever the project is written to vs before they would be retained between project writes. The previous behaviour makes sense given custom user schemes wouldn't have been checked in anyways, and when moving to a project generation workflow the same semantics would ideally remain.
- Any user created breakpoints will get wiped out on every re-generation! (This is most likely undesired)
I suspect xcuserdata was not written by default on purpose and was left up to the tools that use the XcodeProj library to decide how to deal with it, for example worth taking a look at Tuist's XcodeProjWriter and XcodeProjWriterTests which only append user schemes rather than wipe the entire directory.
There was a problem hiding this comment.
Any user specified local schemes will get wiped
Fixed in 0ef27bc
Any user created breakpoints will get wiped
See https://github.com/tuist/XcodeProj/pull/739/files#r1059789045
for example worth taking a look at Tuist's XcodeProjWriter and XcodeProjWriterTests
Nice. Yes that approach makes sense. Nice that Tuist could also improve after this PR:
// XcodeProj can manage writing of shared schemes, we have to manually manage the user schemes
|
|
||
| // MARK: - Writable | ||
|
|
||
| public func write(path: Path, override: Bool) throws { |
There was a problem hiding this comment.
for XCUserData perhaps the override should only apply to the individual files rather than the wiping the entire directory ...
I could see this being extended to XCWorkspace later on as it has the same set of files, but in the XCWorkspace, there's a few additional files Xcode places there
App.xcodeproj/project.xcworkspace/xcuserdata/kas.xcuserdatad/UserInterfaceState.xcuserstate
The UserInterfaceState.xcuserstate file holds some Xcode UI state like which source files were being viewed, tabs etc... we in fact had to address this in Tuist (by preserving xcuserdata rather than completely wipe it) for our project generation workflow as it was quite disruptive to developers having Xcode lose its state every time a re-generation would take place.
There was a problem hiding this comment.
Ah yeah, that makes sense. That sounds like a good approach.
I've pushed 0ef27bc that changes the behaviour to only override individual files and individual XCUserData directories when data is written.
There was a problem hiding this comment.
Not sure about breakpoints though, see https://github.com/tuist/XcodeProj/pull/739/files#r1059789045
teameh
force-pushed
the
xcuserdata-support
branch
2 times, most recently
from
08ed35a
to
0ef27bc
Compare
| /// If false will throw error if breakpoints file exists at the given path. | ||
| func writeBreakpoints(path: Path, override: Bool) throws { | ||
| try XcodeProj.debuggerPath(path).mkpath() | ||
| try breakpoints?.write(path: XcodeProj.breakpointsPath(path), override: override) |
There was a problem hiding this comment.
Not so sure what to do about the breakpoints though.. since the breakpoints are all stored in a single file, I guess we'll need to:
- Read in the current file (if it exists).
- Append the new breakpoints
- Filter out duplicates?
Would that be a good approach?
There was a problem hiding this comment.
if it's explicitly specified then overwriting should be ok (reading / merging is unnecessary complexity), the concern was if there were no breakpoints specified (nil) and that wipes any local ones. The approach you have followed here is good 👍
| let schemesPath = XcodeProj.schemesPath(path) | ||
| if override, schemesPath.exists { | ||
| try schemesPath.delete() | ||
| } |
There was a problem hiding this comment.
This is still happening, only now as part of XCSharedData.writeSchemes
teameh
force-pushed
the
xcuserdata-support
branch
from
0ef27bc
to
c917787
Compare
|
(Rebased and signed commits with gpg key) |
| /// If false will throw error if breakpoints file exists at the given path. | ||
| func writeBreakpoints(path: Path, override: Bool) throws { | ||
| try XcodeProj.debuggerPath(path).mkpath() | ||
| try breakpoints?.write(path: XcodeProj.breakpointsPath(path), override: override) |
There was a problem hiding this comment.
if it's explicitly specified then overwriting should be ok (reading / merging is unnecessary complexity), the concern was if there were no breakpoints specified (nil) and that wipes any local ones. The approach you have followed here is good 👍
|
@all-contributors add @teameh for code |
|
I've put up a pull request to add @teameh! 🎉 |
teameh
force-pushed
the
xcuserdata-support
branch
from
6e55236
to
4f88e27
Compare
teameh
force-pushed
the
xcuserdata-support
branch
2 times, most recently
from
c73599f
to
9556a7b
Compare
| @@ -17,6 +17,8 @@ | |||
| </ActionContent> | |||
| </BreakpointActionProxy> | |||
| </Actions> | |||
| <Locations> | |||
| </Locations> | |||
There was a problem hiding this comment.
We can prevent this addition if we add this change to XCBreakpointList.swift:
--- a/Sources/XcodeProj/Project/XCBreakpointList.swift
+++ b/Sources/XcodeProj/Project/XCBreakpointList.swift
@@ -300,7 +300,9 @@ public final class XCBreakpointList: Equatable, Writable {
let locations = AEXMLElement(name: "Locations", value: nil, attributes: [:])
self.locations.map { $0.xmlElement() }.forEach { locations.addChild($0) }
- element.addChild(locations)
+ if !locations.children.isEmpty {
+ element.addChild(locations)
+ }```
|
|
Thanks! I've removed the superfluous comments. What do you think about #739 (comment)? |
kwridan
requested review from
a team,
adellibovi,
brentleyjones,
danieleformichelli and
fortmarek
and removed request for
a team and
adellibovi
|
Hey @kwridan, @adellibovi, @brentleyjones, @danyf90 & @fortmarek. If there's anything I can do to move this PR forward, please let me know. Also happy to hop on a quick call to elaborate the changes if you think that is helpful for one of you. |
Apologies for the delays - trying to source a second reviewer is proving more challenging these days 😅, I have posted a message in the Tuist slack for visibility. |
| .compactMap { try? XCScheme(path: $0) } | ||
| schemeManagement = try? XCSchemeManagement(path: XCSchemeManagement.path(schemesPath)) | ||
|
|
||
| breakpoints = try? XCBreakpointList(path: XCBreakpointList.path(XCDebugger.path(path))) |
There was a problem hiding this comment.
Why do we want to ignore the thrown errors here?
There was a problem hiding this comment.
I would say we shouldn't ignore the error's here, at least print / log them. But we're not doing that anywhere in similar files, see for example the same line in XCSharedData: https://github.com/tuist/XcodeProj/blob/master/Sources/XcodeProj/Project/XCSharedData.swift#L41
There was a problem hiding this comment.
@kwridan do you have any context why we'd want to ignore these erros? But if this is what we're doing elsewhere, I don't mind shipping as-is
There was a problem hiding this comment.
Keeping the behaviour consistent is preferable 👍
Looking closely at the code, we may not be able to safely change this without changing the APIs / behaviour. Some of those nested elements are optional, for example you can have an user data folder without breakpoints or schemes - and the nested elements currently throw if the path doesn't exist (which isn't ideal :/) - resulting in making the API unusable unless you have a project with all the sub elements present.
Co-authored-by: Marek Fořt <[email protected]>
Co-authored-by: Marek Fořt <[email protected]>
| .compactMap { try? XCScheme(path: $0) } | ||
| schemeManagement = try? XCSchemeManagement(path: XCSchemeManagement.path(schemesPath)) | ||
|
|
||
| breakpoints = try? XCBreakpointList(path: XCBreakpointList.path(XCDebugger.path(path))) |
There was a problem hiding this comment.
Keeping the behaviour consistent is preferable 👍
Looking closely at the code, we may not be able to safely change this without changing the APIs / behaviour. Some of those nested elements are optional, for example you can have an user data folder without breakpoints or schemes - and the nested elements currently throw if the path doesn't exist (which isn't ideal :/) - resulting in making the API unusable unless you have a project with all the sub elements present.
|
Awesome! Thanks for merging this 😃 |
|
Hey there, any plans for creating a new release for XcodeProj? Would be cool if we could use this for yonaskolb/XcodeGen#1142! |
|
It's already out as part of https://github.com/tuist/XcodeProj/releases/tag/8.9.0 🙂 |
|
Thank You
Semper Fi
XIÒK Mustardseed XIÒK
…On Mon, Dec 26, 2022, 11:57 Teameh ***@***.***> wrote:
👋 Hi I'm new here. Thanks for this project, it's great!
Short description 📝
XcodeProj doesn't support xcuserdata yet, this PR adds XCUserData
Background
XCSchemeManagement is already implemented, but it's not included in
Sources/XcodeProj/Project/XcodeProj.swift
<https://github.com/tuist/XcodeProj/blob/main/Sources/XcodeProj/Project/XcodeProj.swift>.
XcodeProj also currently does not feature the necessary XCUserData where
XCSchemeManagement should be part of.
In XcodeGen we are trying to add support for scheme management via
XCSchemeManagement that should result in three options for schemes:
visibility, order hints and wether or not a scheme is shared.
When a scheme is not shared it should be written to xcuserdata instead of
xcshareddata but since xcuserdata is not part of XcodeProj yet this is
not possible at the moment and the shared: Bool property of
XCSchemeManagement is not very useful.
[image: 209574381-98840d4a-ea19-4602-a838-9de86fbaac8d]
<https://user-images.githubusercontent.com/1330668/209579166-8e0095ef-9edf-40e5-843e-ad8392dba64e.png>
Implementation 👩💻👨💻
- Created XCUserData & XCUserDataTests
- Changed XcodeProj.swift so schemes and breakpoints could be written
to both shared and user data
- Conformed XCSharedData to Writable because XcodeProj.swift was
getting a bit bloated.
- Conformed XCSchemeManagement to Writable
- Added XcodeProjTests to verify changes did not break anything
- Extracted func testReadWriteProducesNoDiff(file:line:path:initModel:)
to testWrite.swift to dry up the tests a bit.
I'll add some review remarks and questions to the diff.
Questions
- Coverage for the files inSources/XcodeProj/Project is a bit low. I
found no tests for the code in XcodeProj.swift that is responsible for
writing files to disk. XCSharedData is also lacking tests. I thought
it would be too much out of scope for this PR, but maybe I can add those in
another PR?
[image: image]
<https://user-images.githubusercontent.com/1330668/209579322-245ffcce-a970-4ea5-9e66-e8fd8f71bb54.png>
Remarks?
I had good fun working on this today! I hope you see the added value.
------------------------------
You can view, comment on, or merge this pull request online at:
#739
Commit Summary
- b22dc60
<b22dc60>
Added support for xcuserdata
File Changes
(18 files <https://github.com/tuist/XcodeProj/pull/739/files>)
- *M* Fixtures/Schemes/xcschememanagement.plist
<https://github.com/tuist/XcodeProj/pull/739/files#diff-c01a5816d7ab008c99dbd691a3ebaaf8b5ae99956db35f7c250daca40ef6782e>
(4)
- *M*
Fixtures/iOS/Project.xcodeproj/xcshareddata/xcdebugger/Breakpoints_v2.xcbkptlist
<https://github.com/tuist/XcodeProj/pull/739/files#diff-eabfd621a0c61935d1ac7763363a7770ab15588c910a6d492312413897289288>
(8)
- *M*
Fixtures/iOS/Project.xcodeproj/xcshareddata/xcschemes/iOS.xcscheme
<https://github.com/tuist/XcodeProj/pull/739/files#diff-260186bccc6b14e7206dc20f6cef4bbf164d0a31fba4cddf52574db3598891b3>
(55)
- *A*
Fixtures/iOS/Project.xcodeproj/xcuserdata/username1.xcuserdatad/xcdebugger/Breakpoints_v2.xcbkptlist
<https://github.com/tuist/XcodeProj/pull/739/files#diff-693f84790fb71552b98a3b234af82bfc876838558a82ab5f326f4e82c2855447>
(41)
- *A*
Fixtures/iOS/Project.xcodeproj/xcuserdata/username1.xcuserdatad/xcschemes/iOS-debug.xcscheme
<https://github.com/tuist/XcodeProj/pull/739/files#diff-d2cfd6e5141718960162b8caccd1bcf7b3cd3cddcbbd8c0bfd60de036432af43>
(58)
- *A*
Fixtures/iOS/Project.xcodeproj/xcuserdata/username1.xcuserdatad/xcschemes/iOS-release.xcscheme
<https://github.com/tuist/XcodeProj/pull/739/files#diff-4e01990d7b181f6398a4507b811fc919e2831bb532f20d5e74ea12727e8c48e6>
(58)
- *A*
Fixtures/iOS/Project.xcodeproj/xcuserdata/username1.xcuserdatad/xcschemes/xcschememanagement.plist
<https://github.com/tuist/XcodeProj/pull/739/files#diff-481af6485d9ceda1908df541a5550de524f5bd264276900e3b17e60a94ada9dc>
(45)
- *A*
Fixtures/iOS/Project.xcodeproj/xcuserdata/username2.xcuserdatad/xcschemes/iOSTests.xcscheme
<https://github.com/tuist/XcodeProj/pull/739/files#diff-a2f551f6f6ff6588c55029e139f2e70540ca5698a627fb99641c10e9cb4dcaa3>
(55)
- *M* Sources/XcodeProj/Errors/Errors.swift
<https://github.com/tuist/XcodeProj/pull/739/files#diff-2e91802a1d655319ebb90763a4591239c5f769360edfecab872701c3d598773e>
(16)
- *M* Sources/XcodeProj/Project/XCSharedData.swift
<https://github.com/tuist/XcodeProj/pull/739/files#diff-d9813515fcdca420ec369da92701017d37747feec7abd4519c9cc008e06a3796>
(11)
- *A* Sources/XcodeProj/Project/XCUserData.swift
<https://github.com/tuist/XcodeProj/pull/739/files#diff-91c96251686399e9eecf84e72d4c486d45ebb411071049cbdb9d78c4b7febae4>
(84)
- *M* Sources/XcodeProj/Project/XcodeProj.swift
<https://github.com/tuist/XcodeProj/pull/739/files#diff-fa66e1c428ec70441e060c5e8cfcc78146a7485eaa5a57ab586063fa2dfeb6ac>
(131)
- *M* Sources/XcodeProj/Scheme/XCSchemeManagement.swift
<https://github.com/tuist/XcodeProj/pull/739/files#diff-d92c5d3d02b4fd525536f4113a16fcad669b08e2936e4d8aa954ea2cc647fa5d>
(15)
- *A* Tests/XcodeProjTests/Project/XCUserDataTests.swift
<https://github.com/tuist/XcodeProj/pull/739/files#diff-1de710cd732e868527208ac96d7a3b15075e50db03dc1826afc901c254bba669>
(38)
- *A* Tests/XcodeProjTests/Project/XcodeProjTests.swift
<https://github.com/tuist/XcodeProj/pull/739/files#diff-8075c174d2fe93cee0c43340e3efe00353c7904246d73304dc86deeae4d9d689>
(56)
- *M* Tests/XcodeProjTests/Scheme/XCSchemeManagementTests.swift
<https://github.com/tuist/XcodeProj/pull/739/files#diff-55065e34288606fdfc846acb7583090de95cd05838908667a842847f9cd33693>
(17)
- *M* Tests/XcodeProjTests/Scheme/XCSchemeTests.swift
<https://github.com/tuist/XcodeProj/pull/739/files#diff-69b872ede661673d9bbfbf7bbe775d096ee66a9a86892971b14747e53213b16f>
(5)
- *M* Tests/XcodeProjTests/Tests/testWrite.swift
<https://github.com/tuist/XcodeProj/pull/739/files#diff-7ee17cff9a8fc67e599c12dde9f4fdda159e1c086df68d2bb3e81da4c80d7ffe>
(30)
Patch Links:
- https://github.com/tuist/XcodeProj/pull/739.patch
- https://github.com/tuist/XcodeProj/pull/739.diff
—
Reply to this email directly, view it on GitHub
<#739>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A3UTM2MPCY4HZODT54LODM3WPH2BZANCNFSM6AAAAAATJ3O6EQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
haha wow thanks, I just missed it. Great! |
👋 Hi I'm new here. Thanks for this project, it's great!
Short description 📝
XcodeProj doesn't support
xcuserdatayet, this PR addsXCUserDataBackground
XCSchemeManagementis already implemented, but it's not included inSources/XcodeProj/Project/XcodeProj.swift.XcodeProjalso currently does not feature the necessaryXCUserDatawhereXCSchemeManagementshould be part of.In XcodeGen we are trying to add support for scheme management via
XCSchemeManagementthat should result in three options for schemes: visibility, order hints and wether or not a scheme is shared.When a scheme is not shared it should be written to
xcuserdatainstead ofxcshareddatabut sincexcuserdatais not part of XcodeProj yet this is not possible at the moment and theshared: Boolproperty ofXCSchemeManagementis not very useful.Implementation 👩💻👨💻
XCUserData&XCUserDataTestsXcodeProj.swiftso schemes and breakpoints could be written to both shared and user dataXCSharedDatatoWritablebecauseXcodeProj.swiftwas getting a bit bloated.XCSchemeManagementtoWritableXcodeProjTeststo verify changes did not break anythingfunc testReadWriteProducesNoDiff(file:line:path:initModel:)totestWrite.swiftto dry up the tests a bit.I'll add some review remarks and questions to the diff.
Questions
Sources/XcodeProj/Projectis a bit low. I found no tests for the code inXcodeProj.swiftthat is responsible for writing files to disk.XCSharedDatais also lacking tests. I thought it would be too much out of scope for this PR, but maybe I can add those in another PR?Remarks?
I had good fun working on this today! I hope you see the added value.