groue / GRDBCombine

GRDB ❤️ Combine
MIT License
223 stars 16 forks source link

Fix for Issue #32 - package dependency graph error unresolveable #33

Closed haikusw closed 4 years ago

haikusw commented 4 years ago

This fixes "The package dependency graph can not be resolved" error by changing the dependency for GRDB to something more modern and one that is not incompatible with GRDBCombine.

Tested with Xcode 11.3.0 and Xcode 11.4beta2

haikusw commented 4 years ago

Honestly, I would probably remove the CombineExpectations and GRDB package dependencies from the GRDBCombineDemo project settings entirely. They are brought in via the project dependency on GRDBCombine automatically. But the change in this PR is the smallest change necessary to fix the issue and that seemed best without discussion. Cheers.

groue commented 4 years ago

Hello @haikusw, thanks for the investigation and your contribution! I'll look at #32 and your PR shortly.

groue commented 4 years ago

Honestly, I would probably remove the CombineExpectations and GRDB package dependencies from the GRDBCombineDemo project settings entirely. They are brought in via the project dependency on GRDBCombine automatically

That's a good idea. But I can't make it work:

The GRDBCombineDemo project has a GRDBCombineTests target which runs GRDBCombine tests (the tests for the library, not tests for the demo app). This target is useful as it helps testing the library against various iOS SDK versions. Combine is not quite stable yet, and has been applied a few changes and bug fixes that are quite impactful on GRDBCombine. Precisely speaking, the presence of the internal ReceiveValuesOn and OnDemandFuture publishers aims at getting an identical behavior across various Combine versions.

When I remove the CombineExpectations dependency from the GRDBCombineDemo project, this GRDBCombineTests target won't compile, with Xcode complaining:

error: no such module 'CombineExpectations'
import CombineExpectations
       ^

As an attempt at fixing this error, I tried to add CombineExpectations to the libraries linked to the GRDBCombineTests target. But Xcode won't expose it. I have no workaround... but adding an explicit dependency to CombineExpectations.

Given all this, I think your PR is on the right track. Please give me a little more time.

groue commented 4 years ago

OK @haikusw, I've pushed another commit in your branch, so that both the GRDBCombine package and the GRDBCombineDemo Xcode project share the same requirement on CombineExpectations. This achieves our goal just as well as your changes. But we also have a rule, now, in order to avoid this issue from coming back. The rule is: just keep the requirements synchronized.

If this change is OK for you, I'll happily merge this pull request!

groue commented 4 years ago

so didn't want to extend the allowed versions any more than necessary

I understand. I chose not to lock the CombineExpectations version so that users who want to use it along with GRDBCombine are not stuck. I don't expect much breaking changes to come to CombineExpectations, so this is a reasonable bet.

The question had to be asked, though, I totally agree.

Oddly though, for some reason this results in ending up with two GRDB package references in the Xcode project (see red circle in image below), at least under Xcode 11.4b2

This one worries me much less. It's good to prepare for the next Xcode release. I often do. But in a low-priority queue. No tagged version of any library I ship guarantees any support for beta software. This guarantee would represent a huge burden, and it would turn bugs of Xcode beta into bugs of GRDB, or GRDBCombine, or CombineExpectations, when they are not.

In this pull request, the only Xcode versions I'm focused on are Xcode 11.0+, excluding betas, as documented in the README of this library.

Problems with Xcode 11.4b2 should be discussed in another issue.

Looking at this, I still have this feeling that something isn't quite right with the whole package dependencies and connections.

There again, I'm not sure a holistic approach will give the best result. Focused issues opened in the correct repo will do better.

groue commented 4 years ago

Unless you find something wrong with the current state of this PR, I'll merge it shortly, and tag the next version of the library :-)

haikusw commented 4 years ago

Excellent points. I have confirmed that it does seem to be working as expected in Xcode 11.3 and doesn't have the odd double GDRB reference in Swift Package Dependencies section.

haikusw commented 4 years ago

I will explore the other issues raised when time permits and open new tickets for them as appropriate. Thanks.

groue commented 4 years ago

Shipped in v0.8.1 :-) Thank you @haikusw!

The way we worked together towards a solution to #32 makes me happy. Would you like to be granted a push access to the repository?

haikusw commented 4 years ago

Wow. I'm honored by your suggestion/offer. Thank you. I appreciate your thoughtful approach as well.

I certainly wouldn't feel confident pushing anything except through a PR that you reviewed. There is a LOT I still don't know about GRDB.

The thing I'd like to work on next is https://github.com/groue/GRDB.swift/issues/642

I was thinking that, given the number of project files involved in the GRDB repo, it would be important to be able to run CI on any fix attempts. My plan was to either get Travis working on my account (I've used CircleCi before but not Travis) or to ask you if you'd mind if I used WIP PRs against the GRDB repo which would not be intended to merged but would allow CI to run on possible fixes.

groue commented 4 years ago

Welcome to the GRDBCombine contributors, then 🤝 Of course we'll keep on discussing eventual changes. Yet it's important, for the library and its users, that some contributor dare pushing a change if I'm not there, eventually. Don't worry, this is not a trap: you didn't sign anything with your blood, and you can step out any time - this is the 101 of happy open source developers :-)

My plan was to either get Travis working on my account (I've used CircleCi before but not Travis) or to ask you if you'd mind if I used WIP PRs against the GRDB repo which would not be intended to merged but would allow CI to run on possible fixes.

WIP PRs are just fine :-)

haikusw commented 4 years ago

Welcome to the GRDBCombine contributors, then 🤝 Thank you.

Don't worry, this is not a trap: you didn't sign anything with your blood, and you can step out any time

😂

WIP PRs are just fine :-) 👍