geo

geo copied to clipboard

Remove geo-types patches in Cargo.toml files, so that geo-types is now completelly separated from the rest of the code.

This should be reverted once geo-types 0.8+ is released.

This PR makes it easier to review unlinking part, in preparation for #797 and #772

• [x] I agree to follow the project's code of conduct. ~- [ ] I added an entry to CHANGES.md if knowledge of this change could be valuable to users.~

---

What's the goal of removing patching?

It's a pretty commonly used approach. And if someone is building within the georust/geo workspace (as in they've checked out the code and are in that directory when they run cargo foo) it's likely they are wanting to use the code in that directory, not whatever has been published to crates.

@michaelkirk this method was initially suggested by @frewsxcv in https://github.com/georust/geo/pull/742#issuecomment-1089252376 -- and it worked wonderfully, without requiring patches that pointed to unreleased git branches. This makes it possible to cleanly review, merge, and release geo-types without breaking or patching any other crates or their CI, and will allow us to work on updating other crates afterwards independently of all the 3D/M related breaking changes.

and it worked wonderfully, without requiring patches that pointed to unreleased git branches.

Just to make sure we're on the same page here, the changes in this commit will only affect people who have checked out this repository and are running cargo commands from within their checked out workspace. Right?

I don't think it's inherently bad to build unreleased dependencies when building unreleased code. And actually, I think it's what most people expect. That's why it's set up that way. I expect this change will be worse for most people.

Just to make sure we're on the same page here, the changes in this commit will only affect people who have checked out this repository and are running cargo commands from within their checked out workspace. Right?

yes -- only devs working with this repo will temporarily (until this PR is reverted) will be affected. But doing it this way we ensure that we can cleanly isolate the entire "PR→review→merge→publish" cycle for the geo-types crate without affecting other crates in this repo, as well as the proj and wkt repos. They can be modified/upgraded/released on its own schedule. Potentially we may even want to keep it cleanly separated this way going forward, but this is a separate TBD discussion outside of the scope here.

I don't think it's inherently bad to build unreleased dependencies when building unreleased code. And actually, I think it's what most people expect. That's why it's set up that way. I expect this change will be worse for most people.

I kinda agree with you here, but only if we would have had all round-about dependencies (proj & wkt) together in one mono-repo. All code would then be easily changed at once, and CI would ensure internal repo consistency. I think linking to other repo's unreleased/unmerged code violates this CI consistency, and might cause more concerns than to solve them.

To help me understand, can you lay out a specific "bad" scenario that might plausibly arise if we did not merge this PR?

To help me understand, can you lay out a specific "bad" scenario that might plausibly arise if we did not merge this PR?

The biggest issues I think are the scope creep, difficulty of review, less certainty in CI's results, and historical revision stability.

• My approach only changes 24 files, which is actually 3 PRs - 798, 772, and 797. They are easier to review one by one, with stable publishable code base after each merge, and changes limited to just one crate. It is possible to go back in history and compile each revision and expect it to pass. It does not rely on ephemeral (unpublished) branches of any other repo. Lastly, CI does its job as it is suppose to -- it tells us that each crate in all repos can be safely published after each merge, and users can expect all of them to work properly. (this approach is not possible without this PR)

• With the 742 approach you suggested, it requires changes to 35 files with a single PR that touches code in multiple crates, plus 3 files in wkt/85, and the main branch in this PR relies on the continuous presence of a non-main branch in wkt (it could be deleted at any moment without realizing that geo 's main branch CI depends on it -- so you cannot reliably revert to it and expect it to compile as before. CI in this case does not really tell us if geo-types or any other crate in this repo can be published because it only passes due to the patching hack, not because each crate's code is stable on its own.

As I understand it, the scenario of concern is:

If the unreleased commits in the dependency (e.g. proj) get deleted, then people might not be able to build the unreleased commits in this library.

Is that right?

It turns out this is a moot point! The moment proj is released, we can merge TZM changes without this PR -- all thanks to the default generic parameters as described in https://github.com/georust/geo/pull/797#issuecomment-1094436658

cc: @categulario @urschrei -- two latest contributors to proj

P.S. For the sake of completeness -- @michaelkirk , you are correct. Plus another scenario -- someone in the future tries to find the source of a bug using git bisect, and they are unable to use it because checking out and compiling an old revision would fail because old revision depends on external state.

I'm still not really sold on the changes in this PR. I expect people who have checked out the repo and are building the local workspace are going to be surprised when using geo-types other than what's in the repo.

The moment proj is released, we can merge TZM changes without this PR.

But I have no reservations about shepherding along a proj release, if that's helpful! https://github.com/georust/proj/pull/123

Closing for inactivity. Feel free to reopen if you resume work on this.