gis/parser: disable file existence check for stdout '-' #1245

tmszi · 2021-01-13T13:49:46Z

For testing purpose:

Create file '-' in home dir echo "test" > '-'
Launch GRASS GIS from home dir
Try launch wxGUI Profile tool or using r.profile, r.horizon, ~~r.out.bin~~ module (with output option output=- (stdout))

neteler · 2021-01-14T21:51:11Z

I made a test with the unpatched G78git: Strangely I do not get the "overwrite" issue:

grass78 ~/grassdata/nc_spm_08_grass7/user1/
...
Welcome to GRASS GIS 7.8.6dev (af07fc916)
...

GRASS :~ > r.out.bin input=elevation output=-
Exporting raster as floating values (bytes=4)
�- 0%
 100%
C�
  �B

But the file - is there (as I created it previously as written above):

GRASS :~ > ls -la '-' 
-rw-rw-r-- 1 mneteler mneteler 5 Jan 14 22:11 -

Shoudn't I see an error without this PR? I also tested in G79, same behaviour.

tmszi · 2021-01-15T05:50:08Z

I made a test with the unpatched G78git: Strangely I do not get the "overwrite" issue:
grass78 ~/grassdata/nc_spm_08_grass7/user1/
...
Welcome to GRASS GIS 7.8.6dev (af07fc916)
...

GRASS :~ > r.out.bin input=elevation output=-
Exporting raster as floating values (bytes=4)
�- 0%
 100%
C�
  �B
But the file - is there (as I created it previously as written above):
GRASS :~ > ls -la '-' 
-rw-rw-r-- 1 mneteler mneteler 5 Jan 14 22:11 -
Shoudn't I see an error without this PR? I also tested in G79, same behaviour.
output parameter

Sorry, my mistake I took a closer look at the code r.out.bin and it has an otherwise defined output parameter (isn't affected with this bug) than r.horizon and r.profile.

neteler · 2021-01-17T17:27:27Z

I see. I can replicate the original behavior with r.profile:

# no overwrite enabled
GRASS nc_spm_08_grass7/user1:~ > r.profile elevation resolution=1000 file=- << EOF
> 641712,226095
> 641546,224138
> 641546,222048
> 641049,221186
> EOF
ERROR: option <output>: <-> exists. To overwrite, use the --overwrite flag

# --overwrite
GRASS nc_spm_08_grass7/user1:~ > export GRASS_OVERWRITE=1
GRASS nc_spm_08_grass7/user1:~ > r.profile elevation resolution=1000 file=- << EOF
641712,226095
641546,224138
641546,222048
641049,221186
EOF
Using resolution: 1000 [meters]
Output columns:
Along track dist. [meters], Elevation
Approx. transect length: 1964.027749 [meters]
 0.000000 78.522743
 1000.000000 78.522743
Approx. transect length: 2090.000000 [meters]
 1964.027749 78.522743
 2964.027749 78.522743
 3964.027749 78.522743
Approx. transect length: 995.014070 [meters]
 4054.027749 78.522743

However, isn't - a legal file name (I agree, a bit unexpected)?

Not sure if we may happily overwrite it to fix the GUI issue shown in #1242.
An alternative might be:

when in GUI:
- move - to a tmp name
- do the plotting
- move tmp name back to -

Disadvantage: it is a kind of race condition in case the user wants to access the real file $HOME/- in the very moment that the GUI is plotting stuff.

Am I right that this PR kind of intercept output=- and avoids the real file $HOME/- being affected? If yes, that sounds reasonable to me.

tmszi · 2021-01-17T19:57:33Z

However, isn't - a legal file name (I agree, a bit unexpected)?

Yes it's allowed char https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_282

Not sure if we may happily overwrite it to fix the GUI issue shown in #1242.
An alternative might be:
* when in GUI:
  
  * move `-` to a tmp name
  * do the plotting
  * move tmp name back to `-`
Disadvantage: it is a kind of race condition in case the user wants to access the real file $HOME/- in the very moment that the GUI is plotting stuff.

The error is that even though this char - means stdout, but module parser takes it as a file and checks if it exists.

Am I right that this PR kind of intercept output=- and avoids the real file $HOME/- being affected? If yes, that sounds reasonable to me.

PR adds the condition if the output file parameter is stdout output=- and then do not check the existence of this 'file', because it is not a file but a stdout indicator.

neteler

Tested successfully with r.profile.

neteler · 2021-01-18T22:03:16Z

Could @cmbarton or @nilason make a test of this PR on Mac, please?

cmbarton · 2021-01-19T03:35:03Z

Sure. I can try it tomorrow or Wednesday Michael

…

______________ Michael Barton Sent from my iPhone Please excusr any typoz

On Jan 18, 2021, at 3:03 PM, Markus Neteler ***@***.***> wrote: Could @cmbarton or @nilason make a test of this PR on Mac, please? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

nilason · 2021-01-19T19:43:37Z

I couldn't reproduce this issue. Following log without this PR:

GRASS : grass > touch ~/-                                               Mapset <user1> in <nc_spm_08>
GRASS : grass > r.profile elevation resolution=1000 file=- << EOF       Mapset <user1> in <nc_spm_08>
641712,226095
641546,224138
641546,222048
641049,221186
EOF
Using resolution: 1000 [meters]
Output columns:
Along track dist. [meters], Elevation
Approx. transect length: 1964.027749 [meters]
 0.000000 84.511765
 1000.000000 98.179062
Approx. transect length: 2090.000000 [meters]
 1964.027749 83.541069
 2964.027749 89.507294
 3964.027749 77.892715
Approx. transect length: 995.014070 [meters]
 4054.027749 73.837120
GRASS : grass >                                                         Mapset <user1> in <nc_spm_08>

nilason · 2021-01-19T19:47:00Z

I did also test in GUI without issue.

cmbarton · 2021-01-20T22:09:38Z

I tried to test this today and could not get GRASS to launch. Since this is an unmarked PR, I just downloaded the zip rather than clone it, to make sure I only had the code with this PR. I compiled it using the procedure that @nilason automated and which has worked very well. It compiled without error. But when I tried to launch GRASS, I got this error in the terminal. (I compiled and tried it twice). No idea what the problem is, since I compiled 7.9dev a few days ago and it runs fine.

Last login: Wed Jan 20 14:05:14 on ttys001
/usr/bin/env -i HOME=/Users/cmbarton LANG=en_US.UTF-8 LC_ALL=en_US.UTF-8 BASH_SILENCE_DEPRECATION_WARNING=1 SHELL=/bin/bash PATH=/usr/bin:/bin:/usr/sbin:/etc:/usr/lib /Applications/GRASS-7.9.app/Contents/MacOS/Grass.sh
cmbarton@TCLASHE-C02V435YJ1GQ ~ % /usr/bin/env -i HOME=/Users/cmbarton LANG=en_US.UTF-8 LC_ALL=en_US.UTF-8 BASH_SILENCE_DEPRECATION_WARNING=1 SHELL=/bin/bash PATH=/usr/bin:/bin:/usr/sbin:/etc:/usr/lib /Applications/GRASS-7.9.app/Contents/MacOS/Grass.sh
Rebuilding Addon HTML manual pages index...
Rebuilding Addon menu...
usage: grass79 [-h] [-v] [--text] [--gtext] [--gui] [-c [CRS]] [-e]
               [--config [CONFIG [CONFIG ...]]] [--tmp-mapset]
               [--tmp-location CRS] [-f] [--exec ...]
               [PATH]
grass79: error: unrecognized arguments: -gui

nilason · 2021-01-20T22:14:09Z

I tried to test this today and could not get GRASS to launch. Since this is an unmarked PR, I just downloaded the zip rather than clone it, to make sure I only had the code with this PR. I compiled it using the procedure that @nilason automated and which has worked very well. It compiled without error. But when I tried to launch GRASS, I got this error in the terminal. (I compiled and tried it twice). No idea what the problem is, since I compiled 7.9dev a few days ago and it runs fine.

I don't know what went wrong at the moment. But please, do test with a working copy (without this PR) to see if you can reproduce the issue at all. I couldn't.

cmbarton · 2021-01-20T22:18:20Z

When I deleted a file named '-' (who knows where it came from) in my home folder, as suggested by @tmszi the problem went away. I now have slightly newer versions on all my machines and do not have a problem, even without the PR and adding back a file named '-'.

nilason · 2021-01-20T22:30:51Z

@cmbarton See https://trac.osgeo.org/grass/wiki/HowToGit#Checkingoutpullrequestslocally for how-to checkout a GitHub PR for testing.

cmbarton · 2021-01-20T23:20:09Z

@cmbarton See https://trac.osgeo.org/grass/wiki/HowToGit#Checkingoutpullrequestslocally for how-to checkout a GitHub PR for testing.

Right. But since I'm not contributing code, I have a much simpler setup for now. I just clone and update the main repository to make binaries, rather than keeping my own fork in sync. My understanding is that I should have been able to just download or clone the PR fork to test. If this is not correct, let me know. Thanks.

wenzeslaus · 2021-01-21T03:21:41Z

However, isn't - a legal file name (I agree, a bit unexpected)?

Yes it's allowed char https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_282

I think our approach should be, - always means stdout (or stdin). If module creates (or accepts) files named -, it is a bug. - should either result in using stdout (or stdin) or result in an error that -/stdin/stdout is not supported. In other words, we don't support file name named - even when stdout (or stdin) is not supported. This may not be the behavior of all modules right now, but it is aligned with this PR.

ninsbl · 2022-02-20T13:04:50Z

Bumping up milestone as 8.0.1 is due in two days, while this has not been part of RC1 and there has not been activity for some time.

…stdout-existence-check

HuidaeCho · 2024-03-06T22:52:03Z

I couldn't reproduce this issue. Following log without this PR:

GRASS : grass > touch ~/-                                               Mapset <user1> in <nc_spm_08>
GRASS : grass > r.profile elevation resolution=1000 file=- << EOF       Mapset <user1> in <nc_spm_08>
641712,226095
641546,224138
641546,222048
641049,221186
EOF
Using resolution: 1000 [meters]
Output columns:
Along track dist. [meters], Elevation
Approx. transect length: 1964.027749 [meters]
 0.000000 84.511765
 1000.000000 98.179062
Approx. transect length: 2090.000000 [meters]
 1964.027749 83.541069
 2964.027749 89.507294
 3964.027749 77.892715
Approx. transect length: 995.014070 [meters]
 4054.027749 73.837120
GRASS : grass >                                                         Mapset <user1> in <nc_spm_08>

That is because this bug only affects "new" files.

grass/lib/gis/parser.c

Lines 1717 to 1726 in 9cb4745

    
           if (strcmp(age, "new") == 0) { 
        
               int i; 
        
               char found; 
        
               for (i = 0; opt->answers[i]; i++) { 
        
                   found = FALSE; 
        
                   if (strcmp(element, "file") == 0) { 
        
                       if (access(opt->answers[i], F_OK) == 0) 
        
                           found = TRUE; 
        
                   }

HuidaeCho · 2024-03-06T22:55:34Z

However, isn't - a legal file name (I agree, a bit unexpected)?

Yes it's allowed char https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_282

I think our approach should be, - always means stdout (or stdin). If module creates (or accepts) files named -, it is a bug. - should either result in using stdout (or stdin) or result in an error that -/stdin/stdout is not supported. In other words, we don't support file name named - even when stdout (or stdin) is not supported. This may not be the behavior of all modules right now, but it is aligned with this PR.

I agreed. I think currently different modules behave differently with - filenames. We need to standardize it. For that, we need to handle existing - files as well (maybe, spit out a warning message saying the module will use stdin, not the existing - file), not just "new" ones.

What do we do with - files then? input=- < - or output=- > - ;-)

tmszi added the bug Something isn't working label Jan 13, 2021

tmszi mentioned this pull request Jan 13, 2021

[Bug] Profile tool fails in GUI #1242

Open

tmszi requested a review from neteler January 14, 2021 14:20

neteler previously approved these changes Jan 18, 2021

View reviewed changes

neteler added this to the 8.0.1 milestone Dec 9, 2021

ninsbl modified the milestones: 8.0.1, 8.0.2 Feb 20, 2022

wenzeslaus modified the milestones: 8.0.2, 8.4.0 Feb 27, 2022

gis/parser: disable file existence check for stdout '-'

5983aa9

tmszi force-pushed the parse-disable-file-stdout-existence-check branch from 4b4de28 to 5983aa9 Compare August 29, 2022 15:31

wenzeslaus modified the milestones: 8.3.0, 8.4.0 Feb 10, 2023

Merge remote-tracking branch 'upstream/main' into parse-disable-file-…

b88fb18

…stdout-existence-check

tmszi dismissed neteler’s stale review via b88fb18 February 14, 2024 11:36

HuidaeCho assigned tmszi Mar 6, 2024

wenzeslaus modified the milestones: 8.4.0, Future Apr 27, 2024

Merge branch 'main' into parse-disable-file-stdout-existence-check

81e247c

github-actions bot added C Related code is in C libraries labels Oct 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gis/parser: disable file existence check for stdout '-' #1245

gis/parser: disable file existence check for stdout '-' #1245

tmszi commented Jan 13, 2021 •

edited

Loading

neteler commented Jan 14, 2021

tmszi commented Jan 15, 2021

neteler commented Jan 17, 2021

tmszi commented Jan 17, 2021

neteler left a comment

neteler commented Jan 18, 2021

cmbarton commented Jan 19, 2021 via email

nilason commented Jan 19, 2021

nilason commented Jan 19, 2021

cmbarton commented Jan 20, 2021

nilason commented Jan 20, 2021

cmbarton commented Jan 20, 2021

nilason commented Jan 20, 2021

cmbarton commented Jan 20, 2021

wenzeslaus commented Jan 21, 2021

ninsbl commented Feb 20, 2022

HuidaeCho commented Mar 6, 2024

HuidaeCho commented Mar 6, 2024 •

edited

Loading

gis/parser: disable file existence check for stdout '-' #1245

Are you sure you want to change the base?

gis/parser: disable file existence check for stdout '-' #1245

Conversation

tmszi commented Jan 13, 2021 • edited Loading

neteler commented Jan 14, 2021

tmszi commented Jan 15, 2021

neteler commented Jan 17, 2021

tmszi commented Jan 17, 2021

neteler left a comment

Choose a reason for hiding this comment

neteler commented Jan 18, 2021

cmbarton commented Jan 19, 2021 via email

nilason commented Jan 19, 2021

nilason commented Jan 19, 2021

cmbarton commented Jan 20, 2021

nilason commented Jan 20, 2021

cmbarton commented Jan 20, 2021

nilason commented Jan 20, 2021

cmbarton commented Jan 20, 2021

wenzeslaus commented Jan 21, 2021

ninsbl commented Feb 20, 2022

HuidaeCho commented Mar 6, 2024

HuidaeCho commented Mar 6, 2024 • edited Loading

tmszi commented Jan 13, 2021 •

edited

Loading

HuidaeCho commented Mar 6, 2024 •

edited

Loading