Update Grass7Utils.py. Fixes #20632 #20502 #8557

juanmpd · 2018-11-27T09:19:23Z

Related to https://issues.qgis.org/issues/20632
This seems to solve this issue, but I don't really know if this change may be introducing some collateral effect. It should be revised by someone with good knowledge of this piece of code.

Related to https://issues.qgis.org/issues/20632 This seems to solve this issue, but I don't really know if this change may be introducing some collateral effect. It should be revised by someone with good knowledge of this piece of code.

luipir · 2018-11-27T10:14:09Z

It should be revised by someone with good knowledge of this piece of code

well that part of code is used to hide the terminal during run popen command in windows context... Are you shure it does fix? "si" is set already just before https://github.com/qgis/QGIS/blob/master/python/plugins/processing/algs/grass7/Grass7Utils.py#L359

can be "si" modified by the first subprocess call?

luipir · 2018-11-27T10:21:44Z

in your test environment, can you monitorize what happen to "si" during it's lifecycle? e.g. just dump it's values before and after the first popen call https://github.com/qgis/QGIS/blob/master/python/plugins/processing/algs/grass7/Grass7Utils.py#L364

juanmpd · 2018-11-27T10:29:35Z

I really don't know what is expected to happen to "si", but I saw that during the second call it looked as a vector of size 6 instead of size 3, including the values of the first call. It made me wonder if not being initialized again before the second call could be the reason of the problem. I then only tried this change, and it worked...

luipir · 2018-11-27T10:32:09Z

this is a no documented behaviour of subprocess! can you share the values of the vector please?

juanmpd · 2018-11-27T10:35:30Z

The values of the vector are only numerical values that change on every call, they seem to be file descriptor numbers... I suspect they are stdin, stdout, and stderr related... Do you still want me to post their values in a sample execution?

luipir · 2018-11-27T10:42:00Z

I suspect that the previous "si" is not the same of the current used due to SIP
the same address is used to create copy https://github.com/python/cpython/blob/master/Lib/subprocess.py#L1137
That justify the new "extended" values!

luipir · 2018-11-27T10:42:56Z

they seem to be file descriptor numbers

no. here the definition https://github.com/python/cpython/blob/master/Lib/subprocess.py#L164

well... not only at least

PedroVenancio · 2018-11-27T10:47:01Z

@juanmpd

I tested your patch to check if it solves this one https://issues.qgis.org/issues/20502
and the result is:

Processing algorithm…
Algorithm 'r.fillnulls' starting…
Input parameters:
{ 'GRASS_RASTER_FORMAT_META' : '', 'GRASS_RASTER_FORMAT_OPT' : '', 'GRASS_REGION_CELLSIZE_PARAMETER' : 0, 'GRASS_REGION_PARAMETER' : None, 'edge' : 3, 'input' : 'D:/MDT/dem_srtm_pttm06_80m.tif', 'lambda' : 0.01, 'method' : 2, 'npmin' : 600, 'output' : 'C:/Users/PedroVenancio/AppData/Local/Temp/processing_2874657627454a8a97382b18feac96d0/aabe64cc92504c25904b6ac46eaaf421/output.tif', 'segmax' : 300, 'smooth' : 0.1, 'tension' : 40 }

r.external input="D:\MDT\dem_srtm_pttm06_80m.tif" band=1 output="rast_5bfd1e614a4335" --overwrite -o
g.region n=278040.0 s=-302040.0 e=164040.0 w=-120040.0 res=80.0
r.fillnulls input=rast_5bfd1e614a4335 method="rst" tension=40 smooth=0.1 edge=3 npmin=600 segmax=300 lambda=0.01 output=output9c2197fd216346df92def3c395dcdc65 --overwrite
g.region raster=output9c2197fd216346df92def3c395dcdc65
r.out.gdal -t -m input="output9c2197fd216346df92def3c395dcdc65" output="C:\Users\PedroVenancio\AppData\Local\Temp\processing_2874657627454a8a97382b18feac96d0\aabe64cc92504c25904b6ac46eaaf421\output.tif" format="GTiff" createopt="TFW=YES,COMPRESS=LZW" --overwrite
Cleaning up temporary files...
Starting GRASS GIS...
ATENÇÃO: Concurrent mapset locking is not supported on Windows
Executing <C:\Users\PedroVenancio\AppData\Local\Temp\processing_2874657627454a8a97382b18feac96d0\grassdata\grass_batch_job.cmd> ...
C:\OSGEO41\bin>chcp 1252 1>NUL
C:\OSGEO41\bin>r.external input="D:\MDT\dem_srtm_pttm06_80m.tif" band=1 output="rast_5bfd1e614a4335" --overwrite -o
Input contains an invalid SRS. WKT definition:
LOCAL_CS["Geocoding information not available Projection Name = ETRS_1989_Transverse_Mercator Units = meters Geo",UNIT["unknown",1]]
ATENÇÃO: Unable to convert input map projection information to GRASS format.
Over-riding projection check
Reading band 1 of 1...
r.external completo. Link to raster map <rast_5bfd1e614a4335> created.
C:\OSGEO41\bin>g.region n=278040.0 s=-302040.0 e=164040.0 w=-120040.0 res=80.0
C:\OSGEO41\bin>r.fillnulls input=rast_5bfd1e614a4335 method="rst" tension=40 smooth=0.1 edge=3 npmin=600 segmax=300 lambda=0.01 output=output9c2197fd216346df92def3c395dcdc65 --overwrite
ERRO:Input map has no holes. Check region settings.
Execution of <C:\Users\PedroVenancio\AppData\Local\Temp\processing_2874657627454a8a97382b18feac96d0\grassdata\grass_batch_job.cmd> finished.
Cleaning up temporary files...
Press any key to continue . . .
Cleaning up temporary files...
`
Starting GRASS GIS...
Traceback (most recent call last):
File "C:/OSGEO41/apps/qgis/./python/plugins\processing\algs\grass7\Grass7Algorithm.py", line 413, in processAlgorithm
Grass7Utils.executeGrass(self.commands, feedback, self.outputCommands)
File "C:/OSGEO41/apps/qgis/./python/plugins\processing\algs\grass7\Grass7Utils.py", line 418, in executeGrass
for line in iter(proc.stdout.readline, ''):
File "C:\OSGEO4~1\apps\Python37\lib\codecs.py", line 322, in decode
(result, consumed) = self._buffer_decode(data, self.errors, final)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc7 in position 4: invalid continuation byte

Execution failed after 15.18 seconds

Loading resulting layers
The following layers were not correctly generated.

C:/Users/PedroVenancio/AppData/Local/Temp/processing_2874657627454a8a97382b18feac96d0/aabe64cc92504c25904b6ac46eaaf421/output.tif

You can check the 'Log Messages Panel' in QGIS main window to find more information about the execution of the algorithm.

So, it seems that it raises this issue https://issues.qgis.org/issues/20244 again, in those algorithms (r.mapcalculator or r.fillnulls).

Can you test?

luipir · 2018-11-27T10:50:48Z

@PedroVenancio yes happend to all grass algorithm that use v.out or r.out e.g. are compound commands

PedroVenancio · 2018-11-27T10:53:22Z

@luipir so, this is not completely solved https://issues.qgis.org/issues/20244 right ?

luipir · 2018-11-27T10:55:53Z

@PedroVenancio btw, charset can be an error due the lack of:
https://github.com/qgis/QGIS/blob/master/python/plugins/processing/algs/grass7/Grass7Utils.py#L372
in the second call
can you check if adding charset can solve in compound commands?

juanmpd · 2018-11-27T11:08:51Z

they seem to be file descriptor numbers

no. here the definition https://github.com/python/cpython/blob/master/Lib/subprocess.py#L164

Sorry... I was very imprecise... The vectors are in si.lpAttributeList['handle_list']

In the second call Grass7Utils invokes subprocess.Popen() using the 'si' from the previous call. I suspect it includes file descriptors for stderr, stdin, and stdout that may already be closed after the first call.
subprocess.Popen() ends in a call to _winapi.CreateProcess, where startupinfo ('si') lpAttributeList['handle_list'] has been extended with 3 new values (now 6 elements). I suspect it does that because it founds the 3 previous are not valid. Then, this call ends in an error WinError 87.

Here are the screenshots while debugging the second call, hope it helps:

PedroVenancio · 2018-11-27T11:17:24Z

@luipir I confirm that adding the charset

encoding="cp{}".format(Grass7Utils.getWindowsCodePage()) if isWindows() else None,

to the second call, they work perfectly!

So, it seems that this and @juanmpd changes are needed to solve both issues (https://issues.qgis.org/issues/20502 and https://issues.qgis.org/issues/20632). Completes also this one https://issues.qgis.org/issues/20244.

nyalldawson · 2018-11-27T11:18:54Z

Nice work team!

jef-n · 2018-11-27T12:10:16Z

There might be still an issue - see comments on b39e5a0.

luipir · 2018-11-27T12:13:01Z

@juanmpd great... so in few word! never reuse startupinfo after a subprocess call (old handler remain in handle_list). There is no relation with SIP problems.

@juanmpd can you add the
encoding="cp{}".format(Grass7Utils.getWindowsCodePage()) if isWindows() else None
in the second subprocess and add to your PR? in this way we can fix two issues.

would be fine

add unit test to the failing algorithms (eg. the compound ones)
do also a backport PR to 3.4

whait for the update of this PR to merge... and I agree with @nyalldawson great team work!

luipir · 2018-11-27T12:14:11Z

So, it seems that this and @juanmpd changes are needed to solve both issues (https://issues.qgis.org/issues/20502 and https://issues.qgis.org/issues/20632). Completes also this one https://issues.qgis.org/issues/20244.

@PedroVenancio after PR update I'll modify the PR title to close automatically these issues when merged

luipir · 2018-11-27T12:16:39Z

python/plugins/processing/algs/grass7/Grass7Utils.py

+            if isWindows():
+                si = subprocess.STARTUPINFO()
+                si.dwFlags |= subprocess.STARTF_USESHOWWINDOW
+                si.wShowWindow = subprocess.SW_HIDE
            with subprocess.Popen(


please add also:
encoding="cp{}".format(Grass7Utils.getWindowsCodePage()) if isWindows() else None,
call parameter

I'll add this lines later, now I'm busy with another thing..
You requested "add unit test to the failing algorithms (eg. the compound ones)" and "do also a backport PR to 3.4"... I'm sorry, I just joined github today, and I don't know at all what this unit tests or backport are all about... I don't feel I will be able to do this...

I don't feel I will be able to do this...

np, wait only the add encoding parameter, tnx for the fix

I've already added the line... or at least I think so... :(

If you need anything more from me, please tell me. As I said, I'm not used to github, and I don't know if there is something more that I should have done...

luipir · 2018-11-27T12:30:17Z

There might be still an issue - see comments on b39e5a0.

we may update the code later after creationof
"Maybe we should start grass74.bat from another batchfile and run chcp upfront (maybe even with 65001 - ie. UTF-8)."
Probably we need someone that take more care of this provider and propose more general fixes.

luipir · 2018-11-27T12:46:42Z

wait for an happy travis to merge

juanmpd · 2018-11-27T14:14:18Z

wait for an happy travis to merge

mmm... sorry for my complete ignorance... It's me who is supposed to do a merge?

luipir · 2018-11-27T14:16:06Z

sorry for my complete ignorance... It's me who is supposed to do a merge?

no... wait for the Continuous Integration system (in our case Travis) that run all unit test on every PR to check if it break something!
Thanks for your contribution @juanmpd

PedroVenancio · 2018-11-27T17:20:13Z

@luipir

Do you make the backport to 3.4.x?

luipir · 2018-11-27T17:36:02Z

in this case, I suppose @nyalldawson does "massive" cherry-picks of backportable fixes (if properly labeled)

juanmpd · 2018-12-11T11:31:13Z

Sorry... just to be sure... @nyalldawson is supposed to do "massive" cherry-picks of backportable fixes such as the one in this issue, even if the issue appears as closed?
Is just to know if this fix will get into 3.4.3...
Thanks in advance

nyalldawson · 2018-12-11T11:38:58Z

@juanmpd

I'm not "supposed" to do anything (ie. it's not my job or responsibility to do this!) 😉

But... I DO tend to pick up and backport small fixes like this if the original committer hasn't done it yet and I'm backporting other stuff, just because I want to see 3.4 as a nice stable release and I realise that it's often hard to initially understand the repo structure and the correct process for backporting.

In this case I've already backported as 0c36b90 , so it will be part of 3.4.3

juanmpd · 2018-12-11T11:49:39Z

Thank you very much for your feedback!

PedroVenancio · 2018-12-11T11:54:40Z

@juanmpd

It is already part of 3.4.2-11 nightly build release! You can install it from OSGeo4W.

Meanwhile, I've found the issue that @jef-n had talked about, and commented here b39e5a0

luipir · 2018-12-11T12:25:58Z

I'm not "supposed" to do anything (ie. it's not my job or responsibility to do this!)

my fault sorry

nyalldawson · 2018-12-11T12:28:05Z

@luipir

^^ that was firmly "tongue in cheek"!

Update Grass7Utils.py

9b61eb8

Related to https://issues.qgis.org/issues/20632 This seems to solve this issue, but I don't really know if this change may be introducing some collateral effect. It should be revised by someone with good knowledge of this piece of code.

luipir changed the title ~~Update Grass7Utils.py~~ Update Grass7Utils.py. Fixes #20632 Nov 27, 2018

luipir requested changes Nov 27, 2018

View reviewed changes

Update Grass7Utils.py

31f3f69

luipir approved these changes Nov 27, 2018

View reviewed changes

luipir changed the title ~~Update Grass7Utils.py. Fixes #20632~~ Update Grass7Utils.py. Fixes #20632 #20502 Nov 27, 2018

luipir merged commit 42e30b6 into qgis:master Nov 27, 2018

juanmpd deleted the patch-1 branch November 27, 2018 14:45

luipir added Bugfix Needs Backporting labels Nov 27, 2018

qgib mentioned this pull request May 25, 2019

Problem with GRASS 7 algorithms that call GRASS scripts under Windows #28322

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Grass7Utils.py. Fixes #20632 #20502 #8557

Update Grass7Utils.py. Fixes #20632 #20502 #8557

juanmpd commented Nov 27, 2018

luipir commented Nov 27, 2018

luipir commented Nov 27, 2018

juanmpd commented Nov 27, 2018

luipir commented Nov 27, 2018

juanmpd commented Nov 27, 2018

luipir commented Nov 27, 2018

luipir commented Nov 27, 2018 •

edited

Loading

PedroVenancio commented Nov 27, 2018

luipir commented Nov 27, 2018

PedroVenancio commented Nov 27, 2018

luipir commented Nov 27, 2018

juanmpd commented Nov 27, 2018

PedroVenancio commented Nov 27, 2018 •

edited

Loading

nyalldawson commented Nov 27, 2018

jef-n commented Nov 27, 2018

luipir commented Nov 27, 2018

luipir commented Nov 27, 2018

luipir Nov 27, 2018

juanmpd Nov 27, 2018

luipir Nov 27, 2018

juanmpd Nov 27, 2018

juanmpd Nov 27, 2018

luipir commented Nov 27, 2018

luipir commented Nov 27, 2018

juanmpd commented Nov 27, 2018

luipir commented Nov 27, 2018 •

edited

Loading

PedroVenancio commented Nov 27, 2018

luipir commented Nov 27, 2018 •

edited

Loading

juanmpd commented Dec 11, 2018

nyalldawson commented Dec 11, 2018

juanmpd commented Dec 11, 2018

PedroVenancio commented Dec 11, 2018

luipir commented Dec 11, 2018

nyalldawson commented Dec 11, 2018

Update Grass7Utils.py. Fixes #20632 #20502 #8557

Update Grass7Utils.py. Fixes #20632 #20502 #8557

Conversation

juanmpd commented Nov 27, 2018

luipir commented Nov 27, 2018

luipir commented Nov 27, 2018

juanmpd commented Nov 27, 2018

luipir commented Nov 27, 2018

juanmpd commented Nov 27, 2018

luipir commented Nov 27, 2018

luipir commented Nov 27, 2018 • edited Loading

PedroVenancio commented Nov 27, 2018

luipir commented Nov 27, 2018

PedroVenancio commented Nov 27, 2018

luipir commented Nov 27, 2018

juanmpd commented Nov 27, 2018

PedroVenancio commented Nov 27, 2018 • edited Loading

nyalldawson commented Nov 27, 2018

jef-n commented Nov 27, 2018

luipir commented Nov 27, 2018

luipir commented Nov 27, 2018

luipir Nov 27, 2018

Choose a reason for hiding this comment

juanmpd Nov 27, 2018

Choose a reason for hiding this comment

luipir Nov 27, 2018

Choose a reason for hiding this comment

juanmpd Nov 27, 2018

Choose a reason for hiding this comment

juanmpd Nov 27, 2018

Choose a reason for hiding this comment

luipir commented Nov 27, 2018

luipir commented Nov 27, 2018

juanmpd commented Nov 27, 2018

luipir commented Nov 27, 2018 • edited Loading

PedroVenancio commented Nov 27, 2018

luipir commented Nov 27, 2018 • edited Loading

juanmpd commented Dec 11, 2018

nyalldawson commented Dec 11, 2018

juanmpd commented Dec 11, 2018

PedroVenancio commented Dec 11, 2018

luipir commented Dec 11, 2018

nyalldawson commented Dec 11, 2018

luipir commented Nov 27, 2018 •

edited

Loading

PedroVenancio commented Nov 27, 2018 •

edited

Loading

luipir commented Nov 27, 2018 •

edited

Loading

luipir commented Nov 27, 2018 •

edited

Loading