export projection as PROJJSON

[kylebarron]

This is a first attempt at solving #183 but the doctest segfaults 😕 . Does anyone have guidance here?

running 1 test
Test executable failed (signal: 11 (SIGSEGV)).
test src/proj.rs - proj::Proj::to_projjson (line 1068) ... FAILED

failures:

failures:
    src/proj.rs - proj::Proj::to_projjson (line 1068)

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 23 filtered out; finished in 0.31s

[kylebarron]

I'm a total noob to the PROJ C API, but from reading https://proj.org/en/9.3/development/reference/functions.html#transformation-setup it seems that a transformation object and a CRS object are two different things? Looking at the instances where we call proj_create, it seems that the current rust API only allows creating these transformation objects, and it may be that the proj_as_projjson and similar wkt function expect CRS objects?

If that diagnosis is accurate, would the best course of action be to create a new CRS struct that is separate from the Proj struct?

[urschrei]

Looking at the API, it looks like it returns a PJ object. I would be surprised if they were somehow different, but I'm not at an actual screen right now. I'll try to have a look tomorrow though.

[urschrei]

We are not even making it as far as the call to _string…

[kylebarron]

When I get time to come back to this, I'll try to look at how pyproj is using the proj api

[urschrei]

It looks like the options aren't being constructed correctly:

if I do

let opts_pp = CString::new("").unwrap();
let opts_pp = opts_pp.as_ptr();
…
let out_ptr = proj_as_projjson(self.ctx, self.c_proj, opts_pp as _);

I get a JSON string back. So this should be an easy fix.

[kylebarron]

Whoops 😅

NULL-terminated list of strings with "KEY=VALUE" format

I'm not exactly sure where I went wrong looking at the code

[urschrei]

Actually perhaps I spoke too soon: this doesn't work, though it's possible I've messed up the pointer logic to go from *const i8 to *const *const c_char:

let opts_p = CString::new("MULTILINE=YES");
let ptr = opts_p.as_ptr(); // *const i8
let c_char_ptr = ptr as *const c_char;
let correct_ptr = &c_char_ptr as *const *const c_char;

That gives me:

Unknown option :??Z??

And then panics because a null pointer is returned which triggers our todo(). Still, better than the SIGSEGV?

[kylebarron]

I had been trying to use this as a model:

proj/src/proj.rs

Lines 322 to 329 in 8db09d6

	// convert path entries to CString
	let paths_c = individual
	.iter()
	.map(|str| CString::new(*str))
	.collect::<Result<Vec<_>, std::ffi::NulError>>()?;
	// …then to raw pointers
	let paths_p: Vec<_> = paths_c.iter().map(|cstr| cstr.as_ptr()).collect();
	// …then pass the slice of raw pointers as a raw pointer (const char* const*)

[urschrei]

It turns out that two things were wrong here: one is a bit more obvious and we should have caught it. The other thing is not obvious and caused this to take a couple of hours to debug because every time I changed the test from "pass some options" to "pass no options" it would start failing again.

1. The libproj function expects a pointer to a pointer to a single string. If there are multiple options the string must be built up from them and internally delimited by "," or ", ". We were converting a vec of pointers-to-CString to a pointer. It has to be a pointer to a vec containing a single pointer-to-CString.
2. You can't pass an empty string if there are no options. It must be a null pointer. This is not documented anywhere. Annoying.

I pushed my working changes to your branch – feel free to adapt!

[kylebarron]

Sorry to make you take a couple hours of debugging, but awesome to see it working!

With this learning, is there anything that should be reflected in the set_search_paths function? Looking at the C definitions, proj_context_set_search_paths takes const char *const *paths and proj_as_projjson takes const char *const *options, which seems like the same input type? (I'm bad at reading C types)

It looks like the CI is failing on a dependency that was bumped above proj's msrv

[urschrei]

Oh it's nobody's fault. I think the reason it hasn't been caught is that I had no expectation that someone would try to pass an empty string to set_search_paths which will presumably also cause SIGSEGV. I'll write a test when I get a chance to get back to this, and add the same null pointer logic if necessary.

On the failing CI, I think we'll have to switch to the newest georust CI images in proj.

[urschrei]

is_empty didn't become available until 1.71. I'm going for a walk.

[kylebarron]

I thought... that sounds weird! Surely it must work in an if statement. And I pushed a commit and found that did not work 🙈

[kylebarron]

That is so weird. Feel free to unwind/write over the last commit

[kylebarron]

Well, I was testing a little more with the latest commit I pushed and I'm getting

Unknown option :�p%

which looks like it's coming from here:
https://github.com/OSGeo/PROJ/blob/f377ffa7d62fddf18e9d1c5814c0051cfb35d994/src/iso19111/c_api.cpp#L1821

So it would be nice to understand what about these cstrings is going wrong

[lnicola]

This probably needs to finish with a null.

[kylebarron]

Sorry what does finish with a null mean?

[lnicola]

Push (or chain()) a ptr::null() at the end, otherwise it's unterminated.

[urschrei]

I'm confused too. At the end of what?

[lnicola]

At the end of opts_c.

[kylebarron]

@urschrei I think I'm just learning that the way you had it was correct, but I had to make changes to learn that 😅

[kylebarron]

I think this way works, and is just a different style from @urschrei 's commit here, but it separates out the two unsafe calls to proj_as_projjson which may not be desired

[lnicola]

No, the previous version was fine except for the missing null at the end.

[lnicola]

The options are an array of C-style strings, not a single string. But the array has no length, so it had to end with a null pointer.

You can see the implementation at https://github.com/OSGeo/PROJ/blob/master/src/iso19111/c_api.cpp#L1812.

[lnicola]

https://github.com/georust/gdal/blob/master/src/dataset.rs#L108 follows the same pattern.

[urschrei]

https://github.com/georust/gdal/blob/master/src/dataset.rs#L108 follows the same pattern.

Ohhhh OK. Now I get it.

[urschrei]

@kylebarron I've added a new error (since libproj returns NULL from this function in case of error), and we now return it in case of error in libproj.

I think this is ready for review / merge now.

[kylebarron]

Awesome, sounds good!