Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Support to Local Swift Packages from SPM, Xcode Project & Workspaces #10

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

rogerluan
Copy link
Collaborator

@rogerluan rogerluan commented Sep 5, 2021

Description

This PR resolves #8

NOTE: This PR branches off of #7 , please review that one first. Since I can only work off of GitHub Forks, I can't point this PR to that other branch 😞

Discussion

Feel free to ask as many questions as needed @maxchuquimia 🙇 There're a looot of changes haha

Please don't mind about code style issues (issues that would be caught by a linter) right now. We can fix those as we move forward with the linter PR 👍

Resources

These docs have helped me a lot: https://www.rubydoc.info/gems/xcodeproj/

Demo

xcgrapher --workspace="SomeApp.xcworkspace" --scheme="SomeAppScheme" --spm && open /tmp/xcgrapher

image

…ages

# Conflicts:
#	Sources/XCGrapherLib/XCGrapher.swift
Copy link
Owner

@maxchuquimia maxchuquimia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a huuuge amount of work and it looks good, well done! I've read through the code and left a few comments but haven't had time to play with it in Xcode yet - which I'd like to do after it's been rebased on top of our more recent merges, so will find some time for it later.

But, looking good so far!

Sources/XCGrapherLib/Helpers/ShellTask.swift Show resolved Hide resolved
Sources/XCGrapherLib/ShellTasks/SwiftPackage.swift Outdated Show resolved Hide resolved
Sources/XCGrapherLib/ShellTasks/Xcodeproj.swift Outdated Show resolved Hide resolved
@maxchuquimia
Copy link
Owner

Okay, I had a play with it in Xcode and looked at the diffs - seems it's almost there!

A couple of things still to do:

  • SomeApp's Podfile needs to be updated to reflect the target name change
  • sample-package-description.json has two absolute paths to a directory on your computer, causing some test failures hehe
  • Even after adding a temporary fix for the above, many tests are failing for me... I spent quite some time stepping through testSomeAppSPM and it seems defaultXCGrapherPluginLocation() no longer produces a path to a file - in fact, XCGrapherModuleImportPlugin.framework suddenly doesn't seem to exist at all! 🤔

On this last point, I compared what happens after manually deleting XCGrapher's derived data while the project is open in Xcode and then running File > Swift Packages > Resolve Package Versions. For master it seems that doing this correctly (?) places a XCGrapherModuleImportPlugin.framework inside $DERIVED_DATA/Build/Products/PackageFrameworks but for this branch it does not... the only thing I can think that's changed is the addition of resource processing... have you got any ideas why this is now happening?

@rogerluan
Copy link
Collaborator Author

Podfile

Nice catch!

sample-package-description.json

😳 what a newb mistake! haha but after thinking it through, there's no way to really mock a real path, right? What do you recommend changing it to?

the addition of resource processing

Hum, that's a good hunch. That may be the case, yeah. Idk how to reproduce that issue you just mentioned, though, but we can move the .json sample file to a simple string in the codebase - so no resources. What do you think @maxchuquimia ? Could you test if that change makes it work for you?

@maxchuquimia
Copy link
Owner

Oh man I went down a massive rabbit hole of going through commits to see which one introduced the issue -turns out the .framework wasn't there just because I had the xcgrapher scheme selected instead of the xcgrapher-Package scheme when hitting Command+U 😂 🤦 . I'm guessing this came around as part of 2557e1a - can we add a point to the "Development Notes" section of the ReadMe about needing this scheme selected?

Regarding the json - maybe put a token like $TEST_PATH in it instead of the URL and perform substitution of TEST_PATH to what you expect on the second line of PackageDescriptionTests.testInitialization()?

Now that I can actually run tests, there seems to be a common failure: in XCGrapherWorkspaceTests, testSomeAppSPM, testSomeAppPodsAndSPM, testSomeAppAppleAndSPM and testSomeAppAppleAndSPMAndPods are all failing with the same error:
image

Is there some additional setup I am maybe missing before it should work?

Also, I have noticed another issue: it seems --help no longer prints help... I think it might have something to do with the validate() arguments function having an else? Maybe we need to have die throw instead of exit or something...

@rogerluan
Copy link
Collaborator Author

Hey @maxchuquimia sorry for the radio silence, I've been busy lately (but also on vacation 😂 ) so I've been MIA and will still be for some time. Feel free to address necessary changes if/when you can, and I'll review/come back to this when I can 🙏

@maxchuquimia
Copy link
Owner

No problem @rogerluan, millions of things going on here too so I haven't done any coding over the weekends at all, think it's gonna be a while until I get a good chunk of time to do anything anyway hahah

@mthole
Copy link

mthole commented May 17, 2022

@rogerluan @maxchuquimia Any chance of wrapping this up sometime soon? Looks very useful!

@maxchuquimia
Copy link
Owner

Well, I was able to make a bit of progress in resolving the outstanding issues... see progress.patch.txt (resolves test paths and help not being printed)

I can't seem to get the same result as the sample png in this PR's description - mine simply ignores the local dependency.. so not sure that this is ready to be merged yet

1 similar comment
@maxchuquimia

This comment was marked as duplicate.

@rogerluan
Copy link
Collaborator Author

I don't remember what state I left this PR in @maxchuquimia 😅 I'd have to revisit it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Local Swift Packages
3 participants