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

Update to upstream ctypesgen version #1651

Merged
merged 15 commits into from
Oct 1, 2021

Conversation

nilason
Copy link
Contributor

@nilason nilason commented Jun 16, 2021

Following discussion on #1647 I swapped content of python/grass/ctypes/ctypesgen with latest ctypesgen code from ctypesgen/ctypesgen@0681f8e.

This for more easy overview for initial estimation on degree of diversion and may serve as a starting point in a eventual transition.

I made no effort to make it actually work. Don't try this at home :)

This is now a working PR with latest version of ctypesgen. This needs to be tested.

Addresses trac ticket #3900.

@nilason nilason changed the title WIP: Test updating to upstream ctypesgen version WIP: Update to upstream ctypesgen version Jun 17, 2021
@nilason
Copy link
Contributor Author

nilason commented Jun 17, 2021

It turned out it was almost as straightforward as swapping ctypesgen version code base.

It now actually works, as tested on Mac. It now seems that updating to latest version of ctypesgen is actually feasible. I keep it in draft mode, but I modified the title of the PR to reflect this.

Please do test!

I also would appreciate suggestions to fix CIs Flake8 failure. Preamble code is now included in every generated py file (with an indentical String class among others in each). With the current (old) version preamble code is collected in a (one) common file.

@nilason nilason added this to the 8.0.0 milestone Jun 17, 2021
@nilason nilason added the enhancement New feature or request label Jun 17, 2021
@neteler
Copy link
Member

neteler commented Jun 17, 2021

Great work! I have checked out this PR and compiled it successfully on Fedora 33. The GUI works.
What to look for specifically? Perhaps you have test recommendations...

@neteler
Copy link
Member

neteler commented Jun 17, 2021

OK, I managed to trigger one.. switching to 3D NVIZ mode in the GUI, leads to

Starting 3D view mode...                                                        
Exception in thread
Thread-13
:
Traceback (most recent call last):
  File "/usr/lib64/python3.9/threading.py", line 954, in
_bootstrap_inner

self.run()
  File "/home/mneteler/software/grass_master/dist.x86_64-pc-
linux-gnu/gui/wxpython/nviz/mapwindow.py", line 68, in run

self._display = wxnviz.Nviz(self.log, self.progressbar)
  File "/home/mneteler/software/grass_master/dist.x86_64-pc-
linux-gnu/gui/wxpython/nviz/wxnviz.py", line 124, in
__init__

G_set_error_routine(errfunc)
ctypes
.
ArgumentError
:
argument 1: <class 'TypeError'>: expected CFunctionType
instance instead of CFunctionType
Exception ignored in:
<function Nviz.__del__ at 0x7f22ba163f70>
Traceback (most recent call last):
  File "/home/mneteler/software/grass_master/dist.x86_64-pc-
linux-gnu/gui/wxpython/nviz/wxnviz.py", line 143, in __del__

del self.data
AttributeError
:
data
Traceback (most recent call last):
  File "/home/mneteler/software/grass_master/dist.x86_64-pc-
linux-gnu/gui/wxpython/mapdisp/toolbars.py", line 239, in
OnSelectTool

self.parent.AddNviz()
  File "/home/mneteler/software/grass_master/dist.x86_64-pc-
linux-gnu/gui/wxpython/mapdisp/frame.py", line 489, in
AddNviz

self.MapWindow3D = GLWindow(
  File "/home/mneteler/software/grass_master/dist.x86_64-pc-
linux-gnu/gui/wxpython/nviz/mapwindow.py", line 195, in
__init__

self.decoration["arrow"]["size"] = self._getDecorationSize()
  File "/home/mneteler/software/grass_master/dist.x86_64-pc-
linux-gnu/gui/wxpython/nviz/mapwindow.py", line 1253, in
_getDecorationSize

size = self._display.GetLongDim() / 8.0
AttributeError
:
'NoneType' object has no attribute 'GetLongDim'

@nilason
Copy link
Contributor Author

nilason commented Jun 18, 2021

OK, I managed to trigger one.. switching to 3D NVIZ mode in the GUI, leads to

Same for me, will look into this.

@nilason nilason force-pushed the test-upstream-ctypesgen branch 3 times, most recently from 4451276 to 79b4dbb Compare June 30, 2021 15:05
@nilason
Copy link
Contributor Author

nilason commented Jun 30, 2021

So, now I believe I have actually solved remaining major issues, including nviz 3D mode. Running the tests locally now shows no regression from master, and remaining ones are likely unrelated to this PR.

I included a directory with patches already applied to ctypesgen source, to keep the record. Description of patches needed, which in some way or another need to be addressed upstreams (for this to make any sense):

POINTER patch

https://trac.osgeo.org/grass/ticket/2748
https://trac.osgeo.org/grass/ticket/3641

Every generated GRASS library bridge part (gis.py, raster.py etc.) is a standalone product.
E.g. parts of libgis (include/grass/gis.h) used in libraster (etc/python/grass/lib/raster.py) are also defined in raster.py. This way there is a definition of struct_Cell_head in both gis.py and raster.py -- a situation ctypes doesn't approve of. This patch seems to fix related errors in GRASS. Manually remove e.g. the gis.py parts in raster.py and make a "import" also did the work, but that would be more difficult to implement as part of ctypesgen file generation.

Loader and preamble patch

Replaces sed introduced with:
"Move ctypesgen boilerplate to common module"
59eeff4

Mac patches

#385
#981

@neteler
Copy link
Member

neteler commented Jul 1, 2021

Excellent @nilason: switching to 3D NVIZ mode in the GUI now works! I justed tested that for now (Fedora 33) but will keep the patch locally active to explore it further.

@nilason nilason linked an issue Jul 1, 2021 that may be closed by this pull request
@nilason
Copy link
Contributor Author

nilason commented Jul 1, 2021

Excellent @nilason: switching to 3D NVIZ mode in the GUI now works! I justed tested that for now (Fedora 33) but will keep the patch locally active to explore it further.

Great! Thanks for further testing. I'm rather confident this will work in general use, but there may always be surprises.

@neteler
Copy link
Member

neteler commented Jul 1, 2021

Probably time to merge in order to receive testing on Windows and Mac?

@nilason nilason changed the title WIP: Update to upstream ctypesgen version Update to upstream ctypesgen version Jul 1, 2021
@nilason nilason marked this pull request as ready for review July 1, 2021 19:39
@nilason
Copy link
Contributor Author

nilason commented Jul 1, 2021

Probably time to merge in order to receive testing on Windows and Mac?

That would be the ultimate testing arena!

@petrasovaa
Copy link
Contributor

I will test myself, but also there are some failing tests that seem related (in Ubuntu 18/20 tests).

@wenzeslaus
Copy link
Member

Please, rebase or merge with the master branch to get a better working test result reporting (stderr in CI instead of stdout, downloadable report on failure).

@nilason nilason force-pushed the test-upstream-ctypesgen branch 2 times, most recently from 42284f2 to a4e1075 Compare July 5, 2021 18:08
@nilason
Copy link
Contributor Author

nilason commented Jul 8, 2021

Locally running python3 -m grass.gunittest.main --grassdata $HOME/grassdata --location nc_spm_full_v2alpha2 --location-type nc --min-success 100 gives 3 (and a half) error:

python/grass/pygrass/vector/testsuite/test_pygrass_vector_doctests.py
        ...
        File "etc/python/grass/pygrass/vector/table.py", line 789, in grass.pygrass.vector.table.Link.connection
        Failed example:
            cur.execute("SELECT cat, name, value from %s" %
                        link.table_name)              # doctest: +ELLIPSIS
        Expected:
            <sqlite3.Cursor object at ...>
        Got:
            <pysqlite2.dbapi2.Cursor object at 0x7f8391b903b0>
        ....


scripts/r.import/testsuite/test_parallel.sh
scripts/r.import/testsuite/test_r_import.py
        ....
        ERROR: Unable to open datasource <data/data2.asc>
        ERROR: Unable to open datasource <data/data2.asc>
        ERROR: Unable to read GDAL dataset <data/data2.asc>
        ....


temporal/t.rast.export/testsuite/test_rast_export.py
        ....
        ERROR: Value <AAIGrid> out of range for parameter <format>
        	Legal range: VRT,GTiff,COG,NITF,HFA,ELAS,DTED,PNG,JPEG,MEM,GIF,XPM,BMP,PCIDSK,PCRaster,ILWIS,SGI,SRTMHGT,Leveller,Terragen,ISIS3,ISIS2,PDS4,VICAR,ERS,FIT,GRIB,RMF,WMS,RST,INGR,GSAG,GSBG,GS7BG,R,WEBP,Rasterlite,MBTiles,CALS,WMTS,MRF,PNM,PAux,MFF,MFF2,BT,LAN,IDA,LCP,GTX,NTv2,CTable2,KRO,ROI_PAC,RRASTER,BYN,ARG,USGSDEM,NWT_GRD,ADRG,BLX,PostGISRaster,SAGA,XYZ,HF2,JPEGLS,ZMap,SIGDEM,GPKG,NGW,ENVI,EHdr,ISCE
        ERROR: Unable to export raster map <a_0>
        ....

Two are GDAL related, the other apparently using a different python sqlite driver than expected. Non are seemingly ctypesgen related.

@petrasovaa
Copy link
Contributor

Two are GDAL related, the other apparently using a different python sqlite driver than expected. Non are seemingly ctypesgen related.

Before the rebasing there were errors in the Ubuntu 18/20 CI tests, I think it was related to 3D rasters, they seemed to be related to ctypesgen. I would check once the CI is working again.

@nilason
Copy link
Contributor Author

nilason commented Jul 8, 2021

Two are GDAL related, the other apparently using a different python sqlite driver than expected. Non are seemingly ctypesgen related.

Before the rebasing there were errors in the Ubuntu 18/20 CI tests, I think it was related to 3D rasters, they seemed to be related to ctypesgen. I would check once the CI is working again.

I did at some point see similar errors, but I can't remember when, how...
I think we should wait for all CIs giving green light before merging, but it would be great if someone could spare the time compiling and running the tests locally. If there are indeed failures, it will be a lot easier to track those issues down.

@nilason
Copy link
Contributor Author

nilason commented Sep 27, 2021

At last, I found which old patch for Windows that was needed for ctypesgen to work! This now runs on all platforms without regression and independent of OSGeo4W changes. This may now be merged.

@nilason
Copy link
Contributor Author

nilason commented Sep 30, 2021

If there are no objections, I will go ahead and merge this in a few days.

@marisn
Copy link
Contributor

marisn commented Oct 1, 2021

If there are no objections, I will go ahead and merge this in a few days.

Yes, please.

@nilason
Copy link
Contributor Author

nilason commented Oct 1, 2021

I take it no one objects merging this, so I'll throw it in.

@nilason nilason merged commit ca2d28a into OSGeo:main Oct 1, 2021
@nilason nilason deleted the test-upstream-ctypesgen branch October 1, 2021 20:15
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
* update ctypesgen to community version

* ctypesgen patches needed for mac

* ctypesgen patch for python printer of preamble and loader

* patches applied to ctypesgen source code

* patch for ctypesgen POINTER

* Use ReturnString to create String from str object

* remove lextab.py from gitignore

* remove sed-file for preamble/loader replacement

* black on preamble patch

* add README

- with instructions on updating ctypesgen version
- remove patches dir and separate files

* set RASTER3D_DEFAULT_WINDOW=0

* libimagery unittest: adapt to community ctypesgen

* add win patches to README

* patches needed for Windows

* update README and run Black on a patch
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
* update ctypesgen to community version

* ctypesgen patches needed for mac

* ctypesgen patch for python printer of preamble and loader

* patches applied to ctypesgen source code

* patch for ctypesgen POINTER

* Use ReturnString to create String from str object

* remove lextab.py from gitignore

* remove sed-file for preamble/loader replacement

* black on preamble patch

* add README

- with instructions on updating ctypesgen version
- remove patches dir and separate files

* set RASTER3D_DEFAULT_WINDOW=0

* libimagery unittest: adapt to community ctypesgen

* add win patches to README

* patches needed for Windows

* update README and run Black on a patch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] ctypesgen does not support C bool (_Bool) data type
6 participants