-
-
Notifications
You must be signed in to change notification settings - Fork 308
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
win: patch from OSGeo4W applied #4121
base: main
Are you sure you want to change the base?
Conversation
A dependency yet missing? From the CI log:
|
Not a dependency, but https://github.com/OSGeo/grass/blob/main/mswindows/osgeo4w/libpng-config (see #2679). |
Add basic tests for GeoPackage and Shapefile export which use import to test the result (so they test round trip but focus on the export).
This reverts commit 42ef646.
Note that build_osgeo4w.sh and osgeo4w.yml are GRASS things, that are not used in osgeo4w - they were just aligned with package.sh, which actually is. |
After reintroducing mingw-w64-x86_64-libpng the CI pass successfully. @jef-n Please could you elaborate why you are removing this dependency in the workflow (https://github.com/jef-n/OSGeo4W/blob/master/src/grass/osgeo4w/patch#L9)? On the other side it's installed in |
um, osgeo4w has libpng. not sure why it was only removed from build_osgeo4w.sh. As said I don't use that. Maybe I wanted to replace msys libpng with's OSGeo4W's png, replaced it in both and failed to re-add it in build_osgeo4w.sh, when I found that it was not working. But I don't recall… In OSGeo4W it's just a patch to an unused file ;) |
…geo4w's libpng (refs OSGeo/grass#4121)
There was a change in libpng that broke our copy of libpng-config - adapted. |
Any idea why osgeo4w CI is failing on tests while compilation is successful? It seems to be unrelated to this PR... |
There were a lot more failed files, like 58% of files passed, instead of 81% |
Solved by b435503 |
There seems to be a bad rebase somewhere, there are some unrelated commits in the PR. (It doesn't really matter if we adjust the squashed commit message) Was there a logic in the new order of the OSGeo4W dependencies, was it in the patch that it was in that order? It's not alphabetical neither, I don't find the logic. I'll try to compare manually which ones are changed, as word-based diff highlighting doesn't show the changes clearly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me in general. I have some thoughts on one change, which I'd like to look into though.
@@ -234,7 +234,7 @@ USE_PTHREAD = @USE_PTHREAD@ | |||
#OpenMP | |||
OPENMP_INCPATH = @OPENMP_INCPATH@ | |||
OPENMP_LIBPATH = @OPENMP_LIBPATH@ | |||
OPENMP_LIB = @OPENMP_LIB@ | |||
OPENMP_LIB = @OPENMP_CFLAGS@ @OPENMP_LIB@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OPENMP_CFLAGS@ shouldn't be needed to be added to OPENMP_LIB (?!), OPENMP_CFLAGS should be added to Makefiles that need it (via EXTRA_CFLAGS).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested successfully locally without this change. There should be no need to add OPENMP_CFLAGS
to OPENMP_LIB
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the contrary, I tried using the package.sh build on CI, more closer to OSGeo4W build, and I had some gomp errors: https://github.com/echoix/grass/actions/runs/10641753457/job/29503311365?pr=207#step:9:1864
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we document somewhere why and which DLL is needed to include in the winGRASS package? IIRC I was involved in a trial/error-way to check which of the DLLs we added finally back then.
@@ -248,7 +241,9 @@ if [ -n "$PACKAGE_PATCH" ]; then | |||
|
|||
# copy dependencies (TODO: to be reduced) | |||
cp -uv $DLLS apps/grass/grass$POSTFIX/bin | |||
cp -uv /mingw64/etc/fonts/fonts.conf apps/grass/grass$POSTFIX/etc | |||
|
|||
# copy R batch files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be checked whether we include these files already in another way, e.g. in the NSIS installer-
@@ -8,6 +8,8 @@ REM Uncomment if you want to use Bash instead of Cmd | |||
REM Note that msys package must be also installed | |||
REM set GRASS_SH=%OSGEO4W_ROOT%\apps\msys\bin\sh.exe | |||
|
|||
set PYTHONPATH=%OSGEO4W_ROOT%\apps\grass\grass@POSTFIX@\etc\python;%PYTHONPATH% | |||
set GRASS_COMPATIBILITY_TEST=0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see my inline comments for a final check
The current changes are: cairo-devel
-fftw
freetype-devel
gdal-devel
-gdal-ecw
-gdal-mrsid
geos-devel
+libjpeg-turbo-devel
liblas-devel
libpng-devel
libpq-devel
libtiff-devel
-libxdr
netcdf-devel
-pdal-devel
-pdcurses
proj-devel
-python3-matplotlib
+python3-core
python3-numpy
python3-ply
python3-pywin32
+python3-six
python3-wxpython
-regex-devel
+sqlite3-devel
zstd-devel |
Thanks @nilason! I don't have a strong opinion on the changed dependencies, apart from matplotlib that I thought it was included as a dependency in other builds, but I don't know if it is really used in our modules or just in addons... |
I fixed the conflicts, and took the new deps and sorted them into the action (it would have worked without the multiline list too, but now the diff is clear) There was one conflict in the configure flags that was recently changed to just have --with-lapack, whilst here it had an include path. Please make sure you agree with my choice |
Fixed conflicts |
@landam Let us continue the OpenMP issue from ML here. Extracting from the logs from CI and https://wingrass.fsv.cvut.cz reveals the following: Win CI runner (main)
package.sh
The "package" configure finds a working This might be the result of ddl copying: grass/mswindows/osgeo4w/package.sh Line 127 in 3f61cd7
The following difference is striking: grass/mswindows/osgeo4w/build_osgeo4w.sh Line 26 in 3f61cd7
grass/mswindows/osgeo4w/package.sh Line 157 in 3f61cd7
I don't see any changes that could influence failure in 8.4.1dev (compared to the 8.4.0 release): |
osgeo4w installs libomp.dll: which is probably why configure hooks up on it. Perhaps -/mingw64/bin/libgomp-1.dll
+/mingw64/bin/libomp.dll in grass/mswindows/osgeo4w/package.sh Line 127 in 3f61cd7
could be the solution? |
At least |
The conflicts in the PR I might have time this weekend to address them, it's not too long, it's the sorted lines that need to be chosen with respect to the changes this PR made |
This PR applies https://github.com/jef-n/OSGeo4W/blob/master/src/grass/osgeo4w/patch (author: @jef-n)