-
-
Notifications
You must be signed in to change notification settings - Fork 310
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
rasterlib: Rast_print_json_colors() added (taken from r.colors.out) #4665
base: main
Are you sure you want to change the base?
Conversation
Not sure why the build failed here: https://github.com/OSGeo/grass/actions/runs/11716779361/job/32635385282?pr=4665#step:10:887, but it's working fine on my local. Working on a fix... Edit: Is there any way to reproduce this issue locally so I can identify and test the root cause? |
The error is
which looks like a development package yet missing from the CI workflow. |
The problem is in the Makefile, maybe somebody else can help here? |
Perhaps parson headers are only compiled (build/copied) after the raster library. I didn't look at the makefiles, though. |
Signed-off-by: Nishant Bansal <[email protected]>
Signed-off-by: Nishant Bansal <[email protected]>
d343812
to
a6b1815
Compare
Not sure why the |
How do we know if the data result is saved in a file, and that file is not the same between tests? How do we make sure that these files are correctly teared down after each test? It is using pytest-xdist, where different modules (test files) can run in parallel. |
Signed-off-by: Nishant Bansal <[email protected]>
What about trying to manually run a command in the windows CI, before the tests, to see if the parson output on windows is really what are tests are giving (to see if it is a problem in tests, or a problem with the module output itself on windows)? |
Ie: edit the test_thorough.bat, comment out pytest in osgeo4w.yml, as it runs before. You can try it on a separate PR in against your fork if you don't want to wait as much for CI time, and debug as much as you'd like. |
I tried to debug this with a separate PR on my fork, and it looks like there is a problem with the module output on Windows. I attempted to run the r.colors.out command with some print statements, but I'm completely clueless about how to fix this issue. Any help would be appreciated: link to the output. |
I tried to debug this, and it looks like only |
I've looked up issues in the parson repo for some ideas. From kgabis/parson#154, it makes me think of encoding problems, like the fact that Windows likes UTF16 LE (https://stackoverflow.com/questions/13499920/what-unicode-encoding-utf-8-utf-16-other-does-windows-use-for-its-unicode-da, https://learn.microsoft.com/en-us/windows/win32/intl/unicode). There they talk about the file itself, I'm thinking about the console code page. Otherwise, there was this that was interesting: kgabis/parson#151 talks about using arrayRecord vs root_object, and thus the types JsonValue vs JsonObject. They link to https://stackoverflow.com/questions/49957648/how-to-construct-json-array-with-parson. There's a similarity with missing values in the json. Then why only windows doesn't work? Maybe try with a bleeding edge GCC version to see if it does the same on Linux, as the toolchain in msys2/osgeo4w is quite up to date. |
I tried building GRASS (from this PR) using GCC 14.2.0 on an Ubuntu VM, and I did not encounter any issues that I experienced on Windows. The program runs successfully. However, I have opened a discussion to see if anyone else has encountered a similar issue and can provide a fix. You can check it out here: https://github.com/kgabis/parson/discussions/216 . |
Key Points to Figure Out the Root Cause of the Issue:
|
Signed-off-by: Nishant Bansal <[email protected]>
Fixes the issue after discussion on: GitHub discussion link. |
That is a workaround which may not be guaranteed to work in the future. It took quite a deal of debugging to finally track this one down. The symbol A more stable fix, might be to create a thin GRASS wrapper API (with functions prefixed like |
New related issue reported with #4697. |
(I should add noticing the tools I used for this on CI: adding |
Added
Rast_print_json_colors()
(Renamed fromprint_json_colors
) to the raster library for use in bothr.colors.out
andv.colors.out
to avoid duplication, as discussed in #4660 (comment).