Update proj::Proj constructors to return Result instead of Option.

[frewsxcv]

Surface the underlying error from PROJ.

[frewsxcv]

Test failure look unrelated?

[lnicola]

I'm not familiar with the proj API, but should this be pj_ctx_set_errno(ctx, 0) to match the thread-safe proj_context_errno below?

[frewsxcv]

@lnicola proj-sys doesn't expose proj_ctx_set_errno. Do you know why that might be?

[lnicola]

It looks like it's now called proj_context_errno_set, but that one's missing also.

[lnicola]

Do we even need to reset errno between calls?

[michaelkirk]

I understand the impulse to want to reset the errno - it's good hygiene in general. That said, in the current code, the two places where transform_string is used, we've just created the ctx, so I don't think it could have any stale error hanging around.

Maybe we could make that more obvious by creating the ctx here in this method rather than passing it in.

[lnicola]

Would it be hard to write a test that creates a context, executes something that fails, then something that succeeds to check for this? If it passes, we can probably skip clearing errno.

[michaelkirk]

Should this block me from merging?

To recap my understanding of things:

We're dealing with two entities:

Proj - represents a transform
Context - represents a threading context

Every Proj has a context. Even if you specify a "null" context then you are in fact still referring to a context - the "default" context. The same applies here, where a null Proj results in affecting the default Context (via https://github.com/OSGeo/PROJ/blob/master/src/ctx.cpp#L46)

So in passing a null ptr like this, we are potentially clobbering some state that we don't intend to.

e.g. Say I am using the default context somewhere, and concurrently I use this method to create a new Proj, then I have just clobbered whatever error state existed in the default context, which is potentially bad news.

So whatever solution you arrive at, I'm pretty confident that unsafe { proj_errno_reset(std::ptr::null()) }; needs to go away.

[michaelkirk]

In fact, I think that the only change that needs to happen is to delete that line. I don't think it needs to be replaced with anything at all. Since we've just created the ctx it can't have any error's set (At least, I think that's true...).

[frewsxcv]

Something I just learned. Every PJ object contains a reference to PJ_CONTEXT. And when we call proj_errno_reset(pj), it also resets the errno of the underlying context.

https://github.com/OSGeo/PROJ/blob/ab504340fc9cdca9044a9339acbb4e9ba0367bb6/src/4D_api.cpp#L2022-L2057

Notice that proj_errno_reset calls proj_context_errno_set.

[frewsxcv]

@michaelkirk @lnicola How do the latest commits look? Is this good to go?

[michaelkirk]

Test failure look unrelated?

Indeed! I'm looking into that.

[michaelkirk]

Test failure look unrelated?

Indeed! I'm looking into that.

I feel pretty confident that the current output of CI is failing not due to your changes. Unfortunately, I don't have any leads on fixing the problem yet. 😓

You can look at #99 to see where I'm gathering data about the issue.

[michaelkirk]

LGTM

I guess I'm ok merging with the failing CI since they were failing before... I'll try to scrape together some more free time to dig in again next week unless someone else does first (please feel encouraged to help. I'm really at a loss.).

[frewsxcv]

Squashed

bors r=michaelkirk

[bors]

Build failed:

• ci result