Skip to content

r.geomorphon: changed the JSON output to use parson lib#7151

Open
Dasux wants to merge 8 commits intoOSGeo:mainfrom
Dasux:rewrite_JSON_with_parsor_r.geomorphon
Open

r.geomorphon: changed the JSON output to use parson lib#7151
Dasux wants to merge 8 commits intoOSGeo:mainfrom
Dasux:rewrite_JSON_with_parsor_r.geomorphon

Conversation

@Dasux
Copy link
Copy Markdown
Contributor

@Dasux Dasux commented Mar 4, 2026

I see there is an existing PR for this issue, but it has been inactive for some time...
This PR provides an updated implementation to move this issue forward.
This pr addresses #6970

I replaced the manual json formatting in profile.c with the grass parson wrapper (gjson). instead of printing json line by line using fprintf-style logic, i now create a root json object in memory and build the structure using the G_json_* api.

I created the required json objects and arrays, add the key–value pairs to them, and then attach everything to the root object to preserve the original nesting structure. after the full structure is built, it is serialized using the pretty serializer and written to output.

The output remains functionally the same as before, but the implementation is now structured, easier to maintain, and aligned with grass's internal json utilities.

Please let me know if any other changes are required...

AI assistance (chatgpt) was used to understand parts of the Parson/G_json API. All changes were reviewed, tested and validated by me to ensure correctness and alignment with GRASS standards.

Output before parson integration (manual JSON)

image

Output after parson integration

image

@github-actions github-actions bot added raster Related to raster data processing C Related code is in C module labels Mar 4, 2026
@Dasux Dasux force-pushed the rewrite_JSON_with_parsor_r.geomorphon branch from 476027e to 45fe3d2 Compare March 19, 2026 03:52
@Dasux
Copy link
Copy Markdown
Contributor Author

Dasux commented Mar 19, 2026

please approve the workflows @echoix

@echoix
Copy link
Copy Markdown
Member

echoix commented Mar 19, 2026

Needs an update for the CMake build

@Dasux
Copy link
Copy Markdown
Contributor Author

Dasux commented Mar 19, 2026

yes...i see it---
my best guess is .. its a path issue--- grass: command not found .... the thing cannot see where grass is installed...

echo "${HOME}/install/bin" >> "${GITHUB_PATH}" 

that's on line 81... in cmake.yml ---- and it assumes that everyone installs grass in the same dir...

@Dasux
Copy link
Copy Markdown
Contributor Author

Dasux commented Mar 19, 2026

I modified the path handling to ensure that grass is always picked up from where user installed it.
Cmake should work now--

@echoix
Copy link
Copy Markdown
Member

echoix commented Mar 19, 2026

I modified the path handling to ensure that grass is always picked up from where user installed it.

Cmake should work now--

I don't think that was the problem, you changed the dependencies in the makefile to add parson, but there wasn't an equivalent change in a CMakeLists.txt file.

@github-actions github-actions bot added the CI Continuous integration label Mar 19, 2026
@Dasux
Copy link
Copy Markdown
Contributor Author

Dasux commented Mar 19, 2026

I don't think that was the problem, you changed the dependencies in the makefile to add parson, but there wasn't an equivalent change in a CMakeLists.txt file.

That makes sense..... I'll definitely look into it...

Also, i'm curious, ... was this based on the specific error output from the CMake workflow? or is this a pattern you've seen before?

@echoix
Copy link
Copy Markdown
Member

echoix commented Mar 19, 2026

Since we started to have the CMake build, each change of a makefile usually needs a CMakeLists equivalent. No CMake files were changed in this PR. Adding a dependency in the autotools-based build usually means the CMake build will need to reflect that too.

@Dasux
Copy link
Copy Markdown
Contributor Author

Dasux commented Mar 20, 2026

I see, ...
i linked grass_parson in CMakeLists.txt ... it should work now.. .

Also, I still kept the changes in cmake.yml tho... .it handles path better and doesn't assume grass is built in the same directory for everybody.....
i could revert that change later if needed

@github-actions github-actions bot added the CMake label Mar 20, 2026
@Dasux
Copy link
Copy Markdown
Contributor Author

Dasux commented Mar 22, 2026

I reverted cmake.yml.... That wasn't the issue at all.. You were right...

@Dasux
Copy link
Copy Markdown
Contributor Author

Dasux commented Mar 23, 2026

i think all checks have passed @echoix .. can we merge this?

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 CI Continuous integration CMake module raster Related to raster data processing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants