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

proj_create: +geogrids breaks Windows pathnames with embedded spaces #1152

Closed
hlind49 opened this Issue Oct 12, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@hlind49
Copy link

hlind49 commented Oct 12, 2018

proj_create() calls pj_trim_argc(), which breaks the create string into string tokens. In particular, Windows pathnames with embedded spaces are thus broken.

Example input: +geoidgrids=C:\Program Files\foo\Geoid Files\foobar.gtx

@kbevers

This comment has been minimized.

Copy link
Member

kbevers commented Oct 13, 2018

I don't think you can blame proj_create() for this. Try adding quotes around the path: +geoidgrids="C:\Program Files\foo\Geoid Files\foobar.gtx"

@kbevers kbevers closed this Oct 15, 2018

@hlind49

This comment has been minimized.

Copy link
Author

hlind49 commented Oct 15, 2018

@kbevers

This comment has been minimized.

Copy link
Member

kbevers commented Oct 16, 2018

I think it is a bug.

Indeed it is. Thanks for getting back on this, it was definitely closed prematurely.

For reference:

cs2cs +proj=latlong +datum=WGS84 +to +proj=latlong +geoidgrids="C:\Temp\dir with space\egm96_15.gtx"
12 56 0
12dE    56dN -36.597
>cct +proj=latlong +geoidgrids="C:\Temp\dir with space\egm96_15.gtx"
cct: Bad transformation arguments - (failed to load datum shift file)
    'cct -h' for help

@kbevers kbevers reopened this Oct 16, 2018

@busstoptaktik

This comment has been minimized.

Copy link
Member

busstoptaktik commented Oct 18, 2018

Originally, this really isn't a bug, but a deliberate decision: Since PROJ by convention uses the apostrophe to indicate minutes-of-arc, and the quotation mark to indicate seconds-of-arc, including quoted whitespace was (mostly) impossible anyway: It would interfere with the +lon_0=12d34'56" syntax (supported through the r option of pj_param()).

Allowing quoted whitespace would require us to disallow this syntax - perhaps switching to the (IMHO more readable) 12:34:56 syntax used by GeographicLib and hence the PROJ geod subsystem.

Alternatively, we could adopt repeated quotes as a way to indicate quoted whitespace.

Both alternatives will, however, be quite involved: The former breaking backward compatibility, the latter being a minor nightmare of housekeeping and probably introducing hard-to-debug de facto syntax differences between Unix/Linux and Windows, due to differing conventions for the shell’s handling of quotes.

Under all circumstances, we will have to discuss syntax and breaking backwards compatibility to repair this, since formally, the bug is not the one reported, but a matter of incompatible legacy syntax.

Whatever we do, retrofitting support for quoted whitespace is a bit involved, including changes in all of:

  • pj_chomp() (special care needed for handling '#' style comments)
  • pj_shrink()
  • pj_trim_argc()
  • pj_trim_argv()
  • ...looking into all the gazillion places where pj_param is used.

Since I wrote this code, and have done a fair bit of working-around the original design of pj_param(), I also volunteer to bring it forward into a whitespace-in-paths supporting future.

It will, however, take some discussion and deliberation to figure out how exactly to do it: No matter how, we will break some backward compatibility.

Personally, I'm mostly in favour of disallowing the 12d34'56" syntax, since apostrophes and quotation marks on the command line leads to dubious and platform dependent syntax anyway, so we will be eliminating a source of confusion.

@busstoptaktik busstoptaktik self-assigned this Oct 18, 2018

@busstoptaktik busstoptaktik added this to the 6.0.0 milestone Oct 18, 2018

@rouault

This comment has been minimized.

Copy link
Member

rouault commented Oct 18, 2018

@busstoptaktik I'm not really seing the backward compatibility issue (but I admit to have ignored that part of the code). Why not treating the pattern =" in a special way ? I think only string_to_paralist() and pj_mkparam_ws() should be impacted. Actually pj_mkparam_ws() should return some pointer to string_to_paralist() so that one knows directly where the parameter ends, so as to skip directly to the next one

@busstoptaktik

This comment has been minimized.

Copy link
Member

busstoptaktik commented Oct 18, 2018

@rouault proj_create() is the main entry point for creating a PJ, and it calls the pj_trim_argv()/pj_trim_argc() combo before going on and letting pj_init_ctx() do the hard work, so in many cases the material has been mangled before reaching string_to_paralist() etc., so it will have to be handled at the pj_trim_argv()/pj_trim_argc() stage as well.

But indeed, I had missed the stuff going on in pj_init_*(), where the mess must be repaired once more. Thanks for noticing.

Also, I think your suggestion of special-casing the =" pattern is probebly a viable road, although the free format case should allow =(arbitrary whitespace here)" as well, so to implement that special case it's probably most straightforward to start with a preprocessing removing the (arbitrary whitespace here) part, then going on to decide which additional whitespace to remove.

I once tried to do something similar with LEX - but PROJ syntax is compLEX...

rouault added a commit to rouault/proj.4 that referenced this issue Jan 19, 2019

@rouault

This comment has been minimized.

Copy link
Member

rouault commented Jan 19, 2019

Implemented par PR #1152

@kbevers kbevers closed this in 53a8cbb Jan 20, 2019

kbevers added a commit that referenced this issue Jan 20, 2019

Merge pull request #1230 from rouault/space_in_grid_names
 Add support for spaces in grid name parameters (fixes #1152)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.