-
-
Notifications
You must be signed in to change notification settings - Fork 307
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
winGRASS: exclude pdal as compilation dependency #2883
Conversation
as seen in the compilation logs of daily winGRASS builds (https://wingrass.fsv.cvut.cz/grass83/logs/): pdal is not available in OSGeo4W due to Mingw64/MSVSC incompatibility see also gh CI build recipe (https://github.com/OSGeo/grass/blob/main/mswindows/osgeo4w/build_osgeo4w.sh#L67)
I see you pushed the PR branch 'hellik-patch-1' directly to the osgeo/grass fork, it should have been to your own fork (see https://github.com/OSGeo/grass/blob/main/CONTRIBUTING.md). |
I did this PR on the github web interface; as I have no local GRASS git environment at hand here on my box. If it is not the best way to do some small PRs from the github web interface, then this should be probably disabled. |
It is actually possible to make PRs from GitHub web interface.
|
GitHub interface does not really allow enabling and disabling arbitrary GitHub features. For example, in the past, we had to have required checks just to avoid direct commits to main. Back to our issue, it is really only those with commit rights to the main repo who are faced with the issue of main repo vs fork. Without the commit rights, you are redirected to the fork. Branches on the main repository create noise, but they work just fine for PRs. We can't avoid them with the helper bots, but for human contributors with commit rights, editing fork is a better way. |
Co-authored-by: Nicklas Larsson <n_larsson@yahoo.com>
thanks for the explanations. @wenzeslaus Then lets close this one and do it that way how @nilason proposed. it's a good exercise for my next PR via gh web interface. ;-) |
Tested on build server:
|
@wenzeslaus @nilason Let's merge this PR (even it was not created from a fork). Any objections? |
@hellik Is any backport really needed? GRASS 82 builds seem to be not affected (https://wingrass.fsv.cvut.cz/grass82/logs/log-r2b2069e2ff-205/) |
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.
No need to remake this PR.
Looks good to me!
I see: https://wingrass.fsv.cvut.cz/grass82/ removing the backport label. though not sure how your build environments differ between 8.2. and 8.3. |
I agree, no need to redo the PR. (The potential noise was already created and the branch will need to be deleted one way or the other.) |
There is no difference in build environments. It seems to be related to #2851. |
There is no pdal in either build. |
The change in #2851 that might affect is that configuring with PDAL is the default. That's why --without-pdal is needed in main (8.3). |
For the record: this was already fixed in grass/mswindows/osgeo4w/build_osgeo4w.sh Lines 66 to 67 in 4f1305d
We should probably have just one build script that is used both for packaging and in CI... |
Co-authored-by: Martin Landa <landa.martin@gmail.com> Co-authored-by: Nicklas Larsson <n_larsson@yahoo.com>
What is the issue here? The grass nightly on osgeo4w is apparently fine with PDAL. |
Is there a log file for that? |
Oh, sorry. Scratch that. Forgot that we already established that PDAL built with VC is not usable from mingw due to the C++ ABI. A clean build failed too. Disabled it on osgeo4w too - jef-n/OSGeo4W@d160d4e. I guess PDAL was meanwhile enabled by default in GRASS. |
Co-authored-by: Martin Landa <landa.martin@gmail.com> Co-authored-by: Nicklas Larsson <n_larsson@yahoo.com>
as seen in the compilation logs of daily winGRASS builds (https://wingrass.fsv.cvut.cz/grass83/logs/):
pdal is not available in OSGeo4W due to Mingw64/MSVSC incompatibility
see also gh CI build recipe (https://github.com/OSGeo/grass/blob/main/mswindows/osgeo4w/build_osgeo4w.sh#L67)