Remove deprecated xy from grdimage's THIS_MODULE_OPTIONS and other fixes#8257
Remove deprecated xy from grdimage's THIS_MODULE_OPTIONS and other fixes#8257PaulWessel merged 4 commits intomasterfrom
Conversation
That was the only plotting module that still had the old lower case xy in the MODULE OPTIONS list. We then added x for multicore, hence got 2 -x options to check and that yielded the trouble in #8256. This lead to more relevant fixes in grdfilter test scripts.
test/grdfilter/filtersample.sh
Outdated
| ps=filtersample.ps | ||
|
|
||
| if [[ ${HAVE_GLIB_GTHREAD} =~ TRUE|ON ]]; then | ||
| if [ "X$(gmt-config --has-gthreads)" = "Xyes" ]; then |
There was a problem hiding this comment.
This will break on Windows where the gmt-config does not exist.
There was a problem hiding this comment.
We have had a mix of TRUE|ON vs gmt-config. The TRUE|ON does not work for me. Perhaps we need a short shell plug-n that checks OS and applies these under if [ $OS =....]?
There was a problem hiding this comment.
So some checking shows that it is only the Glib thread cases that use -a:
/grdfilter/threads.sh: _thread_opt=-x+a
./grdfilter/filtertest.sh: _thread_opt=-x+a
./grdfilter/spikes.sh: _thread_opt=-x+a
./grdfilter/filtersample.sh: _thread_opt=-x+a
All OpenMP modules compiled with gcc-mp automatically use all cores available unless -x1 is set, so presumably those have all worked fine. But looks like we need to improve the checking for theGlib threads.
ANother issue: Why does your glib parsing deal with a modifier +a that is the default for OpenMP parser? If OPenMP is build in then the default (in say grdmath) is what you call
l -x+a. Dont you want the glib threads to be the default is they are available, so we could standardise on one rst file for -x and one synopsis string for -x? Here is from grdfilter:
See below for GMT standard:
GMT shows no modifier +a. Can we fix this so we can clean up those 4 scripts (no more checking since the module knows and with elimination of +a will automatically run in parallel and no kludge in the scrips are needed; similar to OpenMP)
There was a problem hiding this comment.
I guess -x+a predates that unified OMP -x
But let's release 6.5 otherwise these last changes never finish.
|
I guess it is not critical that some tests fail under windows since we only run tests on branches and master and those will update soon enough. I agree no point waiting on this for 6.5. But the bug PR needs to be enacted and we have to rebuild installers (at least I do). I will start a PR branch to standardise -x for the future. |
|
BTW, could you reproduce Meiji's bug report with grdimage -x2? I could so it needs to be fixed. Presumable you get the same since it is C code based. |
|
Yes, I get the same thing. But this bug is so innocuous. It has been around for so long and just decided to show up after the installers have been created. |
|
So the old tests for if [[ ${HAVE_GLIB_GTHREAD} =~ TRUE|ON ]]; then |
|
Yes, old test worked. Did not try new branch but running |
|
Can the error be fixed? all tests are bash scripts, gone are they day of .bat, no? What error? |
|
Well, that is what lack of tests other than the random ones does. Both @seisman and @weiji14 have run Open MP commands and found different issues that were fixed. The one from Dongdong was dealt with some weeks ago and the other one we had introduced relatively recently (< 1 year) but again with no testing this dummy was not caught. It only applied to grdimage and it only got triggered after whatever change we did < 1 year ago. So good enough reason by itself to redo the installers after approving #8257. BTW I rese the tests to old GLIB check. |
|
Tests reverted to what they were re GLIB. |
|
The error is
|
|
OK, will update macOS arm64 installer, next-macos-arm tar ball (reason why you and I should get together at some point on GMT/MEX), the tar xz and bz2 balls. Will update sha256 on the release issue page later. |
That was the only plotting module that still had the old lower case xy in the MODULE OPTIONS list. We then added x for multicore, hence got 2 -x options to check and that yielded the trouble in #8256. This lead to more relevant fixes in grdfilter test scripts.
Also handle -x better in usage and synopsis. When not built for MP we should not list -x in the synopsis or the explaining. This was never dealt with. The 3 modules using Glib threads did that with GMT_xg_OPT in those modules, but the rest of the modules used GMT_x_OPT everywhere. Now limited to those that actually have it, I think. Also improved the rst docs on when -x is available.
I've run all tests botn MP and non-MP and 100% passes. Please check on your end and approve as needed. Closes #8256.