fix(someip): fail fast when opensomeip native extension is unavailable#629
fix(someip): fail fast when opensomeip native extension is unavailable#629vtz wants to merge 2 commits intojumpstarter-dev:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 55 minutes and 20 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes add a runtime precondition check in the SOME/IP driver to validate that the OpenSOME/IP C++ native extension is available at initialization. If the extension is unavailable, a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py (1)
348-351: Test interaction with the autouse_mock_get_extfixture.The
@patch(...)decorator here is applied after the autouse fixture'spatch(...)context enters, so the decorator's patch wins for the duration of the test and restores correctly on exit — good. One small strengthening: also assert the message mentions_opensomeip(the concrete symbol users will grep for) in addition toC++ extension, so a future reword of the generic phrase doesn't silently drop the actionable symbol name. Optional.Optional tightening
- with pytest.raises(RuntimeError, match="C\\+\\+ extension"): + with pytest.raises(RuntimeError, match=r"_opensomeip.*not available"): SomeIp(host="127.0.0.1", port=30490)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py` around lines 348 - 351, The test test_someip_raises_when_native_extension_unavailable should assert the RuntimeError message mentions the concrete symbol users will grep for; update the pytest.raises match to include both "C++ extension" and "_opensomeip" (e.g., a regex that contains both substrings) so when SomeIp(host="127.0.0.1", port=30490) raises due to the patched get_ext returning None the assertion verifies the actionable symbol name _opensomeip is present in the error message.python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py (2)
96-103: Guard looks good; fails fast with actionable guidance.Placement after transport-mode validation and before any
ClientConfig/Endpointconstruction is correct, and the error message points users to the concrete macOS rebuild fix from issue#628. Nit: the message is macOS-specific—Linux users hitting e.g. a glibc/ABI mismatch will still see the macOS hint. Consider prefixing with "On macOS:" or linking to the troubleshooting doc referenced in the PR description so non-macOS failures don't look like they need macOS-only steps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py` around lines 96 - 103, The RuntimeError message raised when get_ext() is None in driver.py is macOS-specific and may confuse Linux users; update the message in the get_ext() guard (the raise RuntimeError block) to prefix the macOS-specific rebuild instruction with "On macOS:" and optionally append a short cross-reference like "See troubleshooting documentation for other platforms." so non-macOS failures aren't misdirected; keep the actionable rebuild command and overall wording intact.
10-10: Consider wrapping the private API import in a try/except for robustness.
_bridgeis a private submodule ofopensomeip, soget_extis not part of the public contract and could change upstream. While the code already defensively checksif get_ext() is None, wrapping the import intry/except ImportErrorwould make the driver more resilient to upstream refactors. This way, if opensomeip's internal structure changes, the driver gracefully fails at import time with a clear error rather than a silent function lookup failure.Note:
HAS_NATIVEis not exposed in opensomeip's public API, so the try/except approach is the recommended pattern for now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py` at line 10, Wrap the private import "from opensomeip._bridge import get_ext" in a try/except ImportError block so import failures don't raise at module import time; on ImportError define a safe fallback for get_ext (e.g. a callable that returns None) so existing checks like "if get_ext() is None" continue to work and the module degrades gracefully; update any related initialization that relies on HAS_NATIVE/get_ext to use the fallback and optionally log the import failure for diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py`:
- Around line 348-351: The test
test_someip_raises_when_native_extension_unavailable should assert the
RuntimeError message mentions the concrete symbol users will grep for; update
the pytest.raises match to include both "C++ extension" and "_opensomeip" (e.g.,
a regex that contains both substrings) so when SomeIp(host="127.0.0.1",
port=30490) raises due to the patched get_ext returning None the assertion
verifies the actionable symbol name _opensomeip is present in the error message.
In
`@python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.py`:
- Around line 96-103: The RuntimeError message raised when get_ext() is None in
driver.py is macOS-specific and may confuse Linux users; update the message in
the get_ext() guard (the raise RuntimeError block) to prefix the macOS-specific
rebuild instruction with "On macOS:" and optionally append a short
cross-reference like "See troubleshooting documentation for other platforms." so
non-macOS failures aren't misdirected; keep the actionable rebuild command and
overall wording intact.
- Line 10: Wrap the private import "from opensomeip._bridge import get_ext" in a
try/except ImportError block so import failures don't raise at module import
time; on ImportError define a safe fallback for get_ext (e.g. a callable that
returns None) so existing checks like "if get_ext() is None" continue to work
and the module degrades gracefully; update any related initialization that
relies on HAS_NATIVE/get_ext to use the fallback and optionally log the import
failure for diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5fd2f3b-cc2b-456c-b87d-e7cd97c1e4a8
📒 Files selected for processing (3)
python/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/conftest.pypython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver.pypython/packages/jumpstarter-driver-someip/jumpstarter_driver_someip/driver_test.py
- Wrap private opensomeip._bridge import in try/except ImportError - Add cross-platform guidance to the RuntimeError message - Tighten test assertion regex to match _opensomeip symbol name Made-with: Cursor
|
@vtz not passing type checking or linter, you can run them locally with "make ty" or "make lint" |
2 participants
Closes #628