Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow case insensitive gridname #1121

Closed
mwtoews opened this issue Sep 17, 2018 · 11 comments
Closed

Allow case insensitive gridname #1121

mwtoews opened this issue Sep 17, 2018 · 11 comments

Comments

@mwtoews
Copy link
Member

mwtoews commented Sep 17, 2018

File names provided to nadgrids and geoidgrids are case sensitive on most POSTIX systems (there are some exceptions, like macOS with HFS+). This means PROJ examples that use gridshift file resources behave differently between (e.g.) macOS and Linux:

$ echo -161 62 0 | cs2cs +proj=latlong +datum=WGS84 +to +proj=latlong +nadgrids=alaska
160d59'51.452"W	62d0'2.627"N 0.000
$ echo -161 62 0 | cs2cs +proj=latlong +datum=WGS84 +to +proj=latlong +nadgrids=Alaska
Rel. 5.2.0, September 15th, 2018
<lt-cs2cs>: while processing file: <stdin>, line 1
pj_transform(): failed to load datum shift file
*	* 0.000

Should we allow PROJ to use case insensitive grid file names for these parameters? And to reiterate, it is already case insensitive on Microsoft Windows and most macOS, so this would only change the behaviour for other POSIX systems (e.g. Linux).

@kbevers
Copy link
Member

kbevers commented Sep 17, 2018

I think this would be a nice feature to have. I am not sure how easy it would be though (it's not like there's an fopen_case_insensitive() around for us to use). And then there's all the obvious problems about correct behaviour when both alaska and Alaska exists.

@mwtoews
Copy link
Member Author

mwtoews commented Sep 18, 2018

There is a custom fopen in src/pj_fileapi.c, which can be made to be case insensitive with dirent.h's opendir()/readdir()/closedir(), and string.h's strcasecmp(). This should only be for !defined(_WIN32), and done after a simple fopen returns NULL.

Are there any concerns with allowing case insensitive file opening?

@busstoptaktik
Copy link
Member

I agree that it would be a nice feature to have, especially due to making tests behave consistently across platforms.

I am, however, still undecided whether the nicety is sufficient to support it.

The reason is the question of "where should we stop": Prime meridians (option '+pm') are typically proper nouns (Greenwich, Oslo, Copenhagen, etc), but are implemented without initial cap (greenwich, oslo, copenhagen).

Also many projections are named after their inventor or implementor (Mercator, Bonne, Lambert, etc.) - would we allow grammatically correct spelling here as well?

I do not yet have a firm opinion on this, but I think it should be discussed in depth before going on.

@kbevers
Copy link
Member

kbevers commented Sep 20, 2018

... behave consistently across platforms.

I think this is the important focus point. I get you point regarding capitalization of various other entities but as it is now they at least behave consistently: If you try to set the prime meridian to Oslo you'll get an error on all systems. That is not the case for grids and init-files.

I have two concerns with this issue:

  1. Case-insensitive lookup can lead to some interesting scenarios on case-sensitive file systems: In PROJ_LIB we have the files some_file, SOME_file and some_FILE. We try to open it with +init=SOME_FILE - which do we open?

  2. It introduces non-standard library code that is likely to be a pain in the ass to maintain.

@hobu
Copy link
Contributor

hobu commented Sep 20, 2018

I'm concerned this could lead to many edge cases and unintended consequences.

@mwtoews
Copy link
Member Author

mwtoews commented Sep 20, 2018

  1. Case-insensitive lookup can lead to some interesting scenarios on case-sensitive file systems: In PROJ_LIB we have the files some_file, SOME_file and some_FILE. We try to open it with +init=SOME_FILE - which do we open?

I've done a rough implementation similar to fcaseopen's casepath(), which seems to work well in src/pj_fileapi.c, which will open SOME_FILE before anything else. After that, it's the next case insensitive match that readdir() returns, which does not guarantee any particular order. I'm not sure if this level of matching would be necessary, otherwise a list of matches would need to be created/sorted to pick from (e.g. some_file).

  1. It introduces non-standard library code that is likely to be a pain in the ass to maintain.

I'm rewriting my rough implementation to use the same style of C that I see in PROJ (e.g. char arrays). But whatever PR I'll put forward, it won't add any files, and would only require dirent.h (from the C POSIX library) for non-Windows systems.

I'm concerned this could lead to many edge cases and unintended consequences.

Same. I can't think of any yet, except that I'll need to go thru e.g. pj_open_lib to see how it's used to open certain files, and see if it's anything surprising.

@mwtoews
Copy link
Member Author

mwtoews commented Sep 20, 2018

Oh and quickly on the topic of other case-sensitive matters of the PROJ string, other non-file tokens are case sensitive. There are case sensitive keys with different definitions, e.g., the ellipsoid parameters R_A and R_a have different meanings. So on the plausible "where should we stop" argument, certainly keep all keys case-sensitive, but potentially allow case-insensitive values? (e.g. +pm=Oslo +nadgrids=FILE.GSB). But then does +proj=GStMerc look silly for Gauss–Schreiber transverse Mercator? Or just allow (and document) a few parameters with case-insensitive values?

@cffk
Copy link
Contributor

cffk commented Sep 21, 2018

My $0.02: I recommend on insisting on a specific capitalization of files (and disallowing files with names that differ only in capitalization). I realize that using the wrong capitalization might work on case insensitive systems. But the documentation should flag this as deprecated. In the long run, this rule leads to more predicable behavior.

@kbevers
Copy link
Member

kbevers commented Sep 21, 2018

There's a mix of opinions here, although with a slight preference towards keeping the status quo. Let me offer a solution that at least attempt to fix the problem that started this discussion (various spellings of ntv2_0.gsb).

The most prominent place the ntv2_0.gsb is referenced is here:
https://github.com/OSGeo/proj.4/blob/cf0e6926b21019c835e4f0f11f1a4f11e5fd8fdc/src/pj_datums.c#L49-L51

If we change the +nadgrids string to: +nadgrids=@conus,@alaska,@ntv2_0.gsb,@NTV2_0.GSB,@ntv1_can.dat we at least make sure that PROJ and the proj-datumgrid-north-america packages are aligned in the future while still acknowledging existing setups using the lower-case spelling. It is also an option to add the mixed-case spellings but I don't think they pose a particular big problem in practice.

Is this a way forward that everyone is happy with?

@hobu
Copy link
Contributor

hobu commented Sep 21, 2018

Is this a way forward that everyone is happy with?

As long as there no move to change the default behavior, I'm good. I agree with @cffk that the proposal is fraught and very likely to cause more confusion than it eliminates.

@mwtoews
Copy link
Member Author

mwtoews commented Sep 24, 2018

The status quo is fine, as this does not seem to be a widespread issue in the real world. It's also possible to rewrite stdio_fopen to be case sensitive for everyone, but this may also cause unnecessary issues for some, and I can't say I'm on-board for that option.

As for the NTV2_0.GSB vs. ntv2_0.gsb, probably the simplest solution is to be consistent with PROJ's 15 years history of calling this particular file ntv2_0.gsb; see OSGeo/proj-datumgrid#26 (comment) for rationale, with a primary benefit of no major source changes required to PROJ.

@mwtoews mwtoews closed this as completed Sep 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants