Sample 3D/M implementation without WKT

[nyurik]

This is a test PR to demonstrate circular dependency of GEO and WKT repos

[lnicola]

Why do you need this?

[nyurik]

@lnicola try to compile without it. This PR requires the changes from georust/wkt#85

[lnicola]

I get an error on your branch:

error: failed to select a version for `cc`.
    ... required by package `cmake v0.1.48`
    ... which satisfies dependency `cmake = "^0.1.48"` of package `proj-sys v0.22.0 (https://github.com/georust/proj?rev=e39a671d9afd52c13e180516d6dffd9b79a89d2e#e39a671d)`
    ... which satisfies git dependency `proj-sys` of package `proj v0.25.2 (https://github.com/georust/proj?rev=e39a671d9afd52c13e180516d6dffd9b79a89d2e#e39a671d)`
    ... which satisfies git dependency `proj` of package `geo v0.19.0 (/home/grayshade/Projects/georust/geo/geo)`
versions that meet the requirements `^1.0.72` are: 1.0.73, 1.0.72

all possible versions conflict with previously selected packages.

  previously selected package `cc v1.0.71`
    ... which satisfies dependency `cc = "^1.0.62"` (locked to 1.0.71) of package `ring v0.16.20`
    ... which satisfies dependency `ring = "^0.16.20"` (locked to 0.16.20) of package `rustls v0.20.2`
    ... which satisfies dependency `rustls = "^0.20.1"` (locked to 0.20.2) of package `hyper-rustls v0.23.0`
    ... which satisfies dependency `hyper-rustls = "^0.23"` (locked to 0.23.0) of package `reqwest v0.11.9`
    ... which satisfies dependency `reqwest = "^0.11.9"` of package `proj v0.25.2 (https://github.com/georust/proj?rev=e39a671d9afd52c13e180516d6dffd9b79a89d2e#e39a671d)`
    ... which satisfies git dependency `proj` of package `geo v0.19.0 (/home/grayshade/Projects/georust/geo/geo)`

Never mind, removing my lockfile fixed it. Instead, I get:

    Checking geo-test-fixtures v0.1.0 (/home/grayshade/Projects/georust/geo/geo-test-fixtures)
error[E0432]: unresolved import `wkt`
 --> geo-test-fixtures/src/lib.rs:4:5
  |
4 | use wkt::{Geometry, Wkt, WktFloat};
  |     ^^^ use of undeclared crate or module `wkt`

[lnicola]

Your code doesn't build for whatever reason, but I can revert your wkt version change, bump some versions (because these changes are actually breaking) and it builds fine for me:

So I'm not convinced there's any valid reason to move wkt from one repo to another.

cargo test still fails, but that seems to be due to jts-test-runner, which I agree is cursed.

[nyurik]

@lnicola once again, this is not about being able to run cargo check locally - it might pass because it is a test compile that fails, not a production one. This is about being unable to get CI to pass. Our CI will not allow your code in, so we cannot use bors to check it in without some hacking around the issues

[nyurik]

I mean that if I merge wkt as a module into the geo repo, it works well because i can override which wkt version is being used from the top Crates.toml

[lnicola]

My whole point is that we shouldn't move wkt to this repo.

[nyurik]

@lnicola I'm all for keeping them separate if we have a good process in place for introducing the breaking changes into the core repo without CI going bonkers :)

[rmanoka]

@nyurik Agree that a good robust CI is what we all aim for. To evaluate our options, could you please create a simpler PR; it's difficult to pin-point the issues / have a streamlined discussion when many different files are modified even if they're conceptually simple.

Here's a suggestion. Create a PR that only:

1. declares CoordTZM<T, Z, M>, and makes the existing Coordinate<T> a type-alias
2. fix any issues with using Coordinate<T> as a constructor / function (as it is now a type-alias)
3. make no changes to the other geometry types / algorithms
4. make no changes to any Cargo.toml / other top-level config files
5. do not bring in any other repo. in

In essence, I'm requesting the simplest step towards introducing ZM. This already helps us to discuss:

1. is CI going to break / why?
2. what sort of vector calc. to impose on M (Add/Sub/Neg), and what are the implications?

[nyurik]

@rmanoka makes perfect sense, thx. I might have to take some time off to do this (getting ready for the SOTM-US conference next week), but should be able to do it soon. In the mean time, let me know if i can help with merging the other non-breaking PRs -- they should also help with making the change simpler. Thanks for all your support!!!

[nyurik]

I just tested to see if merging #791 into this PR would solve wkt requirement, but it seems like it is only a part of the solution. This jts-test-runner snippet fails because it uses published wkt to de-serialize WKT into the geotypes object, but that object uses published geo-type, not the local one. So if geo-type breaks, deserialized-one wouldn't match the one being tested locally.

geo/jts-test-runner/src/input.rs

Lines 41 to 42 in 6b3aba5

	#[serde(deserialize_with = "wkt::deserialize_geometry")]
	pub(crate) a: Geometry<f64>,

[rmanoka]

Should we move geo-types to a separate repo? Then, breaking changes can be propogated geo-types -> wkt -> geo.

[nyurik]

Should we move geo-types to a separate repo? Then, breaking changes can be propogated geo-types -> wkt -> geo.

@rmanoka I am beginning to suspect that yes - either we will have to combine wkt into the geo repo, or move geo-types to a separate repo. I am a fan of the first approach, but the community might want the later (either one will work just fine). Is #791 ready to merge? (might make it a bit easier to reason out all the changes).

That said - I am still planning to create a simple change that demonstrates the issue as you suggested above.

[lnicola]

Is this still a problem after #791? I thought I showed why wkt is not problematic.

[nyurik]

Is this still a problem after #791? I thought I showed why wkt is not problematic.

@lnicola yes, still a problem. This PR now includes #791 content, and it will still fail to build because jts uses wkt to parse geo types, and compare the parsed results with the results generated by the local geo algorithms. Local algorithms use different geo-types, thus cannot be compared. See

geo/jts-test-runner/src/input.rs

Lines 41 to 42 in 6b3aba5

	#[serde(deserialize_with = "wkt::deserialize_geometry")]
	pub(crate) a: Geometry<f64>,

[lnicola]

The problem here is using serde to deserialize into gep-types structs. If jts-test-runner used the wkt types and converted then manually to their geo-types equivalents, it would work fine.

[nyurik]

The problem here is using serde to deserialize into gep-types structs. If jts-test-runner used the wkt types and converted then manually to their geo-types equivalents, it would work fine.

@lnicola I was thinking of something along those lines - basically take the geo-types as they come from wkt, and have a giant match statement to convert every geo-type to its local geo-type equivalent. This is extremely hacky IMO, as it relies on the fact that the list of geotypes never changes, and it is always possible to convert one type version to another, but I guess it will work for some of the breaking changes. Do we really want to go this route?

[lnicola]

wkt has its own geometry types, there's no need to use the version of geo-types it can be built with. So we could convert those to the ones in this repo, and if these change, we can update jts-test-runner at the same time.

It's not really hacky. See the geo-test-fixtures crate, which does exactly that for line strings.

[nyurik]

@lnicola I implemented your proposal in https://github.com/georust/geo/pull/742/files#diff-d540e35931c0109d7b7d24d53436d1f2e5d883fc264fcfa22d517b8a43ec7cbb -- closing this PR in favor of #742. Note that there is still a problem in there that i am not certain we can resolve as easily. See #742 (comment)