-
Notifications
You must be signed in to change notification settings - Fork 435
Enable carthage #282
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
Enable carthage #282
Conversation
|
@MrMage: I would like to have your opinion about that (just because I have no clue about what files generated by This said, I want to emphasize that the Carthage build works. So a last resort would be to provide the required frameworks ( In my understanding (according to the Carthage documentation), it works like this:
This way, without versioning the How does that sound to you? |
|
Releasing binary Swift frameworks doesn’t make sense until module stability is reached by Seift, which comes way after ABI stability. So that’s not an option anytime soon. In terms of stuff generated by Haven’t looked into the PR code yet, will do that later. |
MrMage
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, this looks very good to me already. Nice work!
Only suggestion would be to add a test similar to the one that diffs the generated code for the echo service (can be found in the makefile) and add that to the Travis config.
Makefile
Outdated
|
|
||
| project-carthage: | ||
| swift package generate-xcodeproj --output SwiftGRPC-Carthage.xcodeproj | ||
| ruby fix-indentation-settings.rb SwiftGRPC-Carthage.xcodeproj || echo "You may need to install xcodeproj ('sudo gem install xcodeproj')!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use @- for those as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, except for remove-unwanted-targets-for-carthage.rb that should not be ignored in case of failure, I believe.
|
(All of this would still require sign-off from @timburks, of course.) |
d32cbc4 to
d53636e
Compare
|
You were right, I was missing some source files, that are coming from Package.swift:21, I believe. So I had to version What do you think?
I'm not sure what exactly you want to test. Would this test check that running What I can test, though, is that |
3fc11f9 to
d7f37cc
Compare
|
Sorry, I had forgotten that the build is not self-contained because it also requires Swift-Protobuf to be downloaded from GitHub. Versioning SwiftProtobuf in the repo is definitely a no-go. It appears that the
Sorry, I had been hoping that generating the project file was deterministic. |
That's what I was thinking.
Yes, we can add a
So I see that Again, I don't know much about build in Apple systems, but my guess would be that Maybe a solution would be to link SwiftProtobuf dynamically. That way the I am not sure if I am right, but if I am: would it be possible to link |
|
Well, there may actually be another solution. Running Question now: is there a way to add a |
|
I believe I may have reached an acceptable solution! I added a script ( I am actually quite happy with this solution, because it only adds |
| xcodebuild -configuration "Debug" -parallelizeTargets -target SwiftGRPC -target Echo -target Simple -target protoc-gen-swiftgrpc build | ||
| xcodebuild -project SwiftGRPC.xcodeproj -configuration "Debug" -parallelizeTargets -target SwiftGRPC -target Echo -target Simple -target protoc-gen-swiftgrpc build | ||
|
|
||
| build-carthage: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is not necessary. But by having the CI run Carthage, that checks that the Carthage build at least succeeds.
cc908b2 to
b21d895
Compare
b21d895 to
2c5a80e
Compare
|
NOTE: I need travis to |
An xcodeproj needs to be present in the repo for Carthage to work.
Only 4 frameworks are enabled for Carthage builds:
- BoringSSL
- CgRPC
- SwiftGRPC
- SwiftProtobuf
The other targets are removed from SwiftGRPC-Carthage.xcodeproj.
Moreover, a prebuild script is added to the build phases of the
cartage xcodeproj file in order to download the dependencies required
by Package.swift.
The Carthage-compatible xcodeproj can be generated with the following:
$ make project-carthage
2c5a80e to
e07895f
Compare
|
LGTM - thanks @JonasVautherin ! |
MrMage
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from my side too, very creative how you found that workaround! 👍
Sorry for the late reply, I had been OOO for most of Friday and planned to review this on Monday.
|
Yay, thanks a lot! I wasn't expecting it to get merged so quickly! :-) Two more things:
|
|
First, this is amazing that some work has been done to enable Carthage. This was the main blocker for us to use this repo instead of our fork. I've been trying to use this repository with Carthage but I'm still running into problem with Am I doing something wrong or that's expected? Thank you |
|
Hmm, my understanding is that the Also, would having to run |
|
It's not a huge issue per se, but definitely not ideal. |
|
What are the downsides to having to run To be honest, at this point I really don't see how we can avoid that, as the paths into which SwiftPM downloads dependencies seem to be non-deterministic and may change without notice (causing the paths to mismatch inside the pre-built project). |
Not sure I understand that. So you mean that people should That's not how Carthage works. Either you can add If we don't manage to do that, I believe that we cannot say that we support Carthage here. People will probably have to fork, and maintain their fork with the correct
This said, when does Which I am doing because there hasn't been a release since Carthage is supported. But ideally I would do something like: So maybe a tradeoff would be this: whenever you make a release, you have to run Does that make sense to you? Would need to be tested, but that's my current understanding :-). |
|
@SebastianThiebaud, would you mind trying with this in your Cartfile? |
|
No, my idea was for people to add the regular SwiftGRPC repo to their Cartfile and then, when they try to build, prompt them to run `make carthage-project` once in the repo that Carthage downloaded. That shouldn't cause any dependency issues.
In terms of non-deterministicity (?), it seems like every single commit in #281 fails on Travis CI in the `carthage build` step due to a path mismatch (I'm on the go right now, but you should be able to click any of the red Xes to view the CI logs yourself). So it seems like `swift resolve` *always* changes the paths in that branch, and I don't know why. It's fairly frustrating how Carthage seems so fundamentally incompatible with SwiftPM.
If you have any other idea what could be going on there (or how to improve the current situation), however, I'd love to hear it :-)
… On 12. Oct 2018, at 11:45, Jonas Vautherin ***@***.***> wrote:
What are the downsides to having to run make carthage-project once yourself?
Not sure I understand that. So you mean that people should git clone the repository, run make carthage-project, and point their Cartfile to that local project?
That's not how Carthage works. Either you can add grpc-swift in your Cartfile, or you can't. So as a user, in your Cartfile, you want to do something like:
github "grpc/grpc-swift" == 0.4.2
If we don't manage to do that, I believe that we cannot say that we support Carthage here. People will probably have to fork, and maintain their fork with the correct SwiftGRPC-Carthage.xcodeproj, because that's what Carthage uses. Then they may run into dependency issues:
Say project A depends on the fork grpcA
Say project B depends on the fork grpcB, and on project A.
Carthage will see grpcA and grpcB as two different dependencies, and the build will probably fail.
This said, when does SwiftPM change the paths? I believe it is non-deterministic, but it is fixed for a given commit. Right now my Cartfile works fine with:
github "grpc/grpc-swift" "23a0ebdee9613f615f2f2469ed3e700df5856417"
Which I am doing because there hasn't been a release since Carthage is supported. But ideally I would do something like:
github "grpc/grpc-swift" == 0.4.2
So maybe a tradeoff would be this: whenever you make a release, you have to run make carthage-project to update SwiftGRPC-Carthage.xcodeproj for that release tag. Following commits may not work with Carthage, but I believe we could live with having Carthage support only for the releases (and not for the intermediate commits).
Does that make sense to you? Would need to be tested, but that's my current understanding :-).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Won't that break the carthage build? You mean you On my end, I think I would rather fork grpc-swift than providing my users with a Cartfile that always fails the first time.
Right. Whenever you run But Carthage doesn't run I am trying to confirm that on my computer, but somehow I cannot run |
|
Why should this be any different if pushed to a tag? AFAICT the only time when we don't need to run `resolve+make project` is *when we ship the repo with all dependencies (and the corresponding project)* included, and we don't want to do that.
The current approach (added by you, IIRC) specifically calls `swift resolve` as the first step of building Swift Protobuf, so I am a bit surprised that it ever works. Also, we specifically didn't want to call`make carthage-project` on CI to ensure that the build fails if the Carthage project currently committed to the repo is out of date (so we notice that during CI runs).
As far as I see it, we somehow need to have the user download this project's SPM dependencies before building (which we currently do in the generated Xcode project as explained above), but that sometimes non-deterministically messes up the resolved paths inside the project. I still don't see a way to avoid that, correct me if I'm wrong.
… On 12. Oct 2018, at 16:36, Jonas Vautherin ***@***.***> wrote:
No, my idea was for people to add the regular SwiftGRPC repo to their Cartfile and then, when they try to build, prompt them to run make carthage-project once in the repo that Carthage downloaded. That shouldn't cause any dependency issues.
Won't that break the carthage build? You mean you carthage bootstrap, wait for it to fail, go in the the grpc-swift directory that failed, run make carthage-project and then run the carthage build again?
On my end, I think I would rather fork grpc-swift than providing my users with a Cartfile that always fails the first time.
So it seems like swift resolve always changes the paths in that branch
Right. Whenever you run swift resolve, you need to run make carthage-project. I believe the CI should do it (because the CI tries to carthage build, which is a local test, when users run carthage bootstrap or carthage update, which is different).
But Carthage doesn't run swift resolve, right? So if you do swift resolve and make carthage-project and push that to a tag, it should work with Carthage. Juste as it works on master right now:
github "grpc/grpc-swift" "23a0ebdee9613f615f2f2469ed3e700df5856417"
I am trying to confirm that on my computer, but somehow I cannot run swift resolve 🤔. But if you can build with the Cartfile above, it means that there is a way to have release tags compatible with Carthage, I believe.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
I guess what I'm saying is that for me, using I'll spend some time on it over the week-end. I spent quite some time trying to get this to work, so I'm willing to push a bit further now instead of dropping support for Carthage :-). |
|
Yeah, I haven't figured out why it sometimes fails and sometimes works, either. But it looks like my current open PR *always* fails *on CI*, so you could experiment with that one. (For faster iteration times, you can temporarily skip other CI steps.)
… On 12. Oct 2018, at 18:30, Jonas Vautherin ***@***.***> wrote:
I guess what I'm saying is that for me, using github "grpc/grpc-swift" "23a0ebdee9613f615f2f2469ed3e700df5856417" in a Cartfile always works. So I'm trying to understand why it fails in the CI and in @SebastianThiebaud's build.
I'll spend some time on it over the week-end. I spent quite some time trying to get this to work, so I'm willing to push a bit further now instead of dropping support for Carthage :-).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
So, I took your NIO branch and ran So I believe this is the situation:
It seems like we won't be able to always have the Carthage project up-to-date (because it would imply constantly running and commiting But I believe it is fine, because it is not really how we would expect people to depend on Carthage anyway, and at least depending on the releases should work flawlessly. Now back to the CI: it should run @MrMage does that make sense to you? So it seems like |
|
@JonasVautherin thank you for the elaboration! This definitely makes sense for me. Personally, I would simply always run Instead, before a release is cut, it would be the responsibility of whoever is cutting the release to run |
Makes perfect sense to me! :-) Note (for the record) that until the next release of grpc-swift, I'll keep using this commit that I know works in my Cartfile:
|
Opening the work for #13.
Note that there are very few changes in the build system: most of this PR is about archiving the project
SwiftGRPC-Carthage.xcodeprojthat is generated bymake project-carthage.The issue here is that it won't work unless
makeis run (and versioned).Trying the PR:
carthage build --no-skip-currentand observe that it fails because files are missingmakecarthage build --no-skip-currentagain, and see that it works this time.