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

rasterlib: Rast_print_json_colors() added (taken from r.colors.out) #4665

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

NishantBansal2003
Copy link
Contributor

Added Rast_print_json_colors()(Renamed from print_json_colors) to the raster library for use in both r.colors.out and v.colors.out to avoid duplication, as discussed in #4660 (comment).

@github-actions github-actions bot added raster Related to raster data processing C Related code is in C libraries module labels Nov 7, 2024
@NishantBansal2003
Copy link
Contributor Author

NishantBansal2003 commented Nov 7, 2024

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?

@neteler
Copy link
Member

neteler commented Nov 7, 2024

The error is

json_color_out.c:21:10: fatal error: grass/parson.h: No such file or directory

which looks like a development package yet missing from the CI workflow.

@petrasovaa
Copy link
Contributor

The problem is in the Makefile, maybe somebody else can help here?

@wenzeslaus
Copy link
Member

Perhaps parson headers are only compiled (build/copied) after the raster library. I didn't look at the makefiles, though.

lib/raster/Makefile Outdated Show resolved Hide resolved
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
@NishantBansal2003
Copy link
Contributor Author

Not sure why the r.colors.out and r3.colors.out pytests are failing in OSGeo4W, especially since they run fine on my local setup and in other CI builds.
cc: @nilason
Could these tests be flaky?

@echoix
Copy link
Member

echoix commented Nov 9, 2024

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 <nishant.bansal.282003@gmail.com>
@echoix
Copy link
Member

echoix commented Nov 9, 2024

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)?

@github-actions github-actions bot added the Python Related code is in Python label Nov 9, 2024
@echoix
Copy link
Member

echoix commented Nov 9, 2024

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.

@NishantBansal2003
Copy link
Contributor Author

NishantBansal2003 commented Nov 9, 2024

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.
cc: @nilason

@NishantBansal2003
Copy link
Contributor Author

I tried to debug this, and it looks like only json_object_set_string is not working. When I print the color output string, the string is fine. Also, when I check if the json_object_set_string does not return an error, it runs fine. So, I am not sure what the issue is. Is there anything you can refer me to so I can learn and understand what might be causing the issue and how to fix it?
cc: @nilason @echoix

@echoix
Copy link
Member

echoix commented Nov 11, 2024

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.

@NishantBansal2003
Copy link
Contributor Author

Screenshot 2024-11-12 at 4 25 29 PM 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 .

@NishantBansal2003
Copy link
Contributor Author

Key Points to Figure Out the Root Cause of the Issue:

  • The issue does not occur when running the code in the module.
  • The issue arises when running the code in the library.
  • The code works in the library except when using OSGeo4W workflow.
  • Only json_object_set_string is causing the issue; json_object_set_number works without problems.
  • The output string passed to json_object_set_string appears fine, and no errors are returned, but the string is not being processed as expected.

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
@NishantBansal2003
Copy link
Contributor Author

Fixes the issue after discussion on: GitHub discussion link.
Could you please let me know if this is an appropriate way to make changes in the library?

@nilason
Copy link
Contributor

nilason commented Nov 13, 2024

Fixes the issue after discussion on: GitHub discussion link. Could you please let me know if this is an appropriate way to make changes in the library?

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 json_object_set_string, which is problematic is also part of JSON-C API. JSON-C is embedded in GDAL and as the GRASS raster library links against GDAL there is a symbol conflict!

A more stable fix, might be to create a thin GRASS wrapper API (with functions prefixed like G_json_object_set_string()). Such a move definitely deserves a separate PR. Thoughts, @wenzeslaus, @cwhite911 ?

@nilason
Copy link
Contributor

nilason commented Nov 13, 2024

Fixes the issue after discussion on: GitHub discussion link. Could you please let me know if this is an appropriate way to make changes in the library?

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 json_object_set_string, which is problematic is also part of JSON-C API. JSON-C is embedded in GDAL and as the GRASS raster library links against GDAL there is a symbol conflict!

A more stable fix, might be to create a thin GRASS wrapper API (with functions prefixed like G_json_object_set_string()). Such a move definitely deserves a separate PR. Thoughts, @wenzeslaus, @cwhite911 ?

New related issue reported with #4697.

@nilason
Copy link
Contributor

nilason commented Nov 13, 2024

(I should add noticing the tools I used for this on CI: adding mingw-w64-x86_64-ntldd package and executing ntldd --list-imports dist.x86_64-w64-mingw32/lib/libgrass_raster.8.5.dll, ntldd --list-exports dist.x86_64-w64-mingw32/lib/libgrass_raster.8.5.dll. See ntldd help.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C libraries module Python Related code is in Python raster Related to raster data processing
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

6 participants