search

tuist / XcodeProj

📝 Read, update and write your Xcode projects
https://xcodeproj.tuist.io
MIT License
2.03k stars 309 forks source link

Add support for xcuserdata #739

Closed teameh closed 1 year ago

teameh commented 1 year ago

👋 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. 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.

209574381-98840d4a-ea19-4602-a838-9de86fbaac8d

Implementation 👩‍💻👨‍💻

I'll add some review remarks and questions to the diff.

Questions

image

Remarks?

I had good fun working on this today! I hope you see the added value.

teameh commented 1 year ago

(Rebased and signed commits with gpg key)

kwridan commented 1 year ago

@all-contributors add @teameh for code

allcontributors[bot] commented 1 year ago

@kwridan

I've put up a pull request to add @teameh! :tada:

teameh commented 1 year ago

Again thanks for the review. I've updated the PR.

Regarding your comments:

Only creating the directories if needed

Good point. I've also added this behaviour to XCSharedData

I propose we mark some of those path methods as deprecated but maintain the current behaviour and instead create XcodeProjPaths which would have more explicit methods

I wonder if all the path methods should be part of a separate object at all. Shouldn't they be more coupled with their Type? Eg. XCUserData 'knows' what it's own folder structure is like so wouldn't it make more sense to place the path methods in that file as well? 

extension XCUserData {
    /// Returns user data path relative to the given path.
    ///
    /// - Parameter path: `.xcodeproj` file path
    /// - Returns: user data path relative to the given path.
    public static func path(_ path: Path) -> Path {
        path + "xcuserdata"
    }

    /// Returns user data path for a specific user relative to the given path.
    ///
    /// - Parameter path: `.xcodeproj` file path
    /// - Returns: user data path relative to the given path.
    public static func path(_ path: Path, userName: String) -> Path {
        XCUserData.path(path) + "\(userName).xcuserdatad"
    }
}

After some variants I went with this approach and changed the files, this time keeping backwards compatibility in mind. See the bottom of the various classes.

I'm not sure if you would indeed need a separate XcodeProjPaths object to expose all the paths directly, but I've added it nevertheless. See https://github.com/tuist/XcodeProj/pull/739/files#diff-fa66e1c428ec70441e060c5e8cfcc78146a7485eaa5a57ab586063fa2dfeb6acR248. Do you think Tuist still needs it? The paths can also be derived. I'll move it to its own file if this is what we want.

teameh commented 1 year ago

No problem. Iterations are good.

Ok, I've removed the enum and added some more assertions to see why the builds are failing..

My machine:

Executed 314 tests, with 0 failures (0 unexpected) in 5.406 (5.463) seconds

teameh commented 1 year ago

@kwridan could you maybe approve the workflow? https://github.com/tuist/XcodeProj/actions/runs/3854685253

teameh commented 1 year ago
teameh commented 1 year ago

Thanks! I've removed the superfluous comments.

What do you think about https://github.com/tuist/XcodeProj/pull/739#discussion_r1063408365?

teameh commented 1 year ago

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.

kwridan commented 1 year ago

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.

teameh commented 1 year ago

Awesome! Thanks for merging this 😃

teameh commented 1 year ago

Hey there, any plans for creating a new release for XcodeProj? Would be cool if we could use this for https://github.com/yonaskolb/XcodeGen/pull/1142!

kwridan commented 1 year ago

It's already out as part of https://github.com/tuist/XcodeProj/releases/tag/8.9.0 🙂

MustardseedX commented 1 year ago

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:

https://github.com/tuist/XcodeProj/pull/739 Commit Summary

File Changes

(18 files https://github.com/tuist/XcodeProj/pull/739/files)

Patch Links:

— Reply to this email directly, view it on GitHub https://github.com/tuist/XcodeProj/pull/739, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3UTM2MPCY4HZODT54LODM3WPH2BZANCNFSM6AAAAAATJ3O6EQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

teameh commented 1 year ago

haha wow thanks, I just missed it. Great!