release/v2.0.0#4
Conversation
…ests as well as summary stats
Lurk390
requested review from
GJames-RiversideResearch,
Copilot and
melbasiouny-riverside
Lurk390
added
documentation
There was a problem hiding this comment.
Pull request overview
This PR reintroduces and wires up multi-language bindings (Python via SWIG, Java/JNI via SWIG, and a header-only C++ wrapper), and updates the SCons/CI test flow to run binding tests with per-language output sections and a final summary.
Changes:
- Add SWIG-based Python and Java/JNI bindings plus a header-only C++ wrapper, each with basic smoke tests and SCons build/test targets.
- Introduce a
tools/test_reporter.pywrapper to label streamed test output and record pass/fail counts for a final summary. - Update SCons/CI/docs to support building/testing bindings alongside core tests and to modernize org/package naming (
upstandinghackers→riversideresearch).
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/test_reporter.py | Runs tests with labeled headers and writes pass/fail counts to a results file. |
| tests/t_misc.c | Updates test token-type namespace strings to com.riversideresearch.... |
| src/t_misc.c | Same namespace update in source-side test. |
| src/bindings/swig/hammer.i | New unified SWIG interface with Python typemaps and Java/JNI typemaps/extends. |
| src/bindings/python/setup.py | Setuptools/SWIG build script for Python extension module. |
| src/bindings/python/hammer_tests.py | Python unittest smoke tests for the Python bindings. |
| src/bindings/python/SConscript | SCons build/test/install wiring for Python bindings. |
| src/bindings/python/README.md | Python binding build/test/usage documentation. |
| src/bindings/java/SConscript | SCons build/test/install wiring for Java/JNI bindings and jar packaging. |
| src/bindings/java/README.md | Java binding build/test/usage documentation. |
| src/bindings/java/HammerTests.java | Java smoke tests emitting Results: X passed, Y failed. |
| src/bindings/cpp/hammer/hammer_test.hpp | C++ gtest assertion helpers for parsing. |
| src/bindings/cpp/hammer/hammer.hpp | Header-only C++ wrapper API around the C library. |
| src/bindings/cpp/cpp_tests.cpp | C++ gtest smoke tests for the wrapper. |
| src/bindings/cpp/SConscript | SCons build/test/install wiring for C++ wrapper and tests. |
| src/bindings/cpp/README.md | C++ binding build/test/usage documentation. |
| src/SConscript | Integrates binding test execution/results reporting into scons test. |
| ruff.toml | Excludes build/ from Ruff linting. |
| SConstruct | Adds bindings option, exports binding result lists, and prints summary table after tests. |
| README.md | Documents newly supported language bindings and updates formatting/links. |
| DEVELOPMENT.md | Adds instructions for building/testing bindings and improves command formatting. |
| .github/workflows/pipeline.yml | Adds dependency installs and a CI step to run binding tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pyinstallexec = pythonenv.Command( | ||
| None, libhammer_python, "%s %s install" % (env["python"], pysetup) |
There was a problem hiding this comment.
installpython runs setup.py install, which typically ignores SCons’ prefix/DESTDIR settings and installs directly into the interpreter’s site-packages (often requiring root). This is inconsistent with the rest of the build’s install semantics and complicates packaging. Consider installing into $prefix/$DESTDIR via pip install --prefix .../--root ..., or using env.Install() to place artifacts under the SCons-controlled install tree.
| pyinstallexec = pythonenv.Command( | |
| None, libhammer_python, "%s %s install" % (env["python"], pysetup) | |
| destdir = env.get("DESTDIR", "") | |
| prefix = env.get("prefix", sys.prefix) | |
| install_cmd_parts = [env["python"], "-m", "pip", "install", "--no-deps"] | |
| if destdir: | |
| install_cmd_parts.extend(["--root", destdir]) | |
| if prefix: | |
| install_cmd_parts.extend(["--prefix", prefix]) | |
| install_cmd_parts.append(pydir) | |
| install_cmd = " ".join(install_cmd_parts) | |
| pyinstallexec = pythonenv.Command( | |
| None, | |
| libhammer_python, | |
| install_cmd, |
| Export("libhammer_static") | ||
| Export("libhammer_static libhammer_shared") | ||
|
|
||
| for b in env.get("bindings", []): |
There was a problem hiding this comment.
env.get('bindings', []) will include the default ['none'] from the ListVariable, so this loop will attempt to load src/bindings/none/SConscript and fail. After normalizing the bindings option in SConstruct, this loop should only iterate real binding names (or be guarded to skip none).
| for b in env.get("bindings", []): | |
| for b in env.get("bindings", []): | |
| if b == "none": | |
| continue |
| Parser p = Uint8(); | ||
| EXPECT_TRUE(ParsesTo(p, "\x78", "u0x78")); | ||
| EXPECT_TRUE(ParseFails(p, "")); | ||
| } |
There was a problem hiding this comment.
All of these tests above are testing the same failure case for too short of a length passed in, should we add other value type tests with failure cases for taking too much input?
| EXPECT_TRUE(ParseFails(p, " ab")); | ||
| } | ||
|
|
||
| #include <ctype.h> |
There was a problem hiding this comment.
Put this at the top
| Import("env libhammer_shared testruns targets binding_results binding_test_stamps") | ||
|
|
||
| if libhammer_shared is None: | ||
| print("Warning: C++ bindings require the shared library (not available in coverage/gprof builds). Skipping.") |
There was a problem hiding this comment.
Does this imply that you need to build with tests to get the C++ bindings? Could we allow c++ bindings without needing test packages?
| import com.riversideresearch.hammer.*; | ||
|
|
||
| /** | ||
| * Basic smoke tests for the Hammer Java bindings, mirroring hammer_tests.py. |
There was a problem hiding this comment.
Can this get reworded to be more specific?
| Or run the full test suite including all language bindings: | ||
|
|
||
| ```bash | ||
| scons bindings=python,java test |
There was a problem hiding this comment.
This would appear to lose CPP bindings?
| ```bash | ||
| scons bindings=python | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Can we investigate how this interacts with python VENVs
| } | ||
| %typemap(out) struct HParseResult_* { | ||
| if ($1 == NULL) { | ||
| // TODO: raise parse failure |
There was a problem hiding this comment.
We should cleanup inline TODOs and bring them to readme or fix them
GJames-RiversideResearch
left a comment
GJames-RiversideResearch
left a comment
There was a problem hiding this comment.
Generally looks good, I would just like to see more consistent combinator reachability across different bindings, the addition of h_put_value, h_get_value, and h_free_value, and some other minor cleanups
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… expand docs and tests
… expand docs and tests
upstandinghackerspackage references, renaming toriversideresearchbindings=allsupport to build and test all language bindings togetherscons bindings=all teststep to the CI pipelinescons testoutput with labeled sections per language and a final summary table of pass/fail counts