Skip to content

Make concrete message_passing implementation public#65

Open
saksham-chawla wants to merge 1 commit intocodex/replay-20260319023913-pr-130-basefrom
codex/replay-20260319023913-pr-130-head
Open

Make concrete message_passing implementation public#65
saksham-chawla wants to merge 1 commit intocodex/replay-20260319023913-pr-130-basefrom
codex/replay-20260319023913-pr-130-head

Conversation

@saksham-chawla
Copy link
Collaborator

@saksham-chawla saksham-chawla commented Mar 18, 2026

The alias alone is not usable from other modules.


Make concrete message_passing implementation public

This pull request modifies the score/message_passing/BUILD file to change the visibility of the message_passing_unix_domain and message_passing_qnx_dispatch C++ libraries. Their visibility is updated from a restricted //score/mw/com/impl:__subpackages__ to //visibility:public, allowing other modules to directly depend on these concrete message passing implementations.


This summary was automatically generated by @propel-code-bot

The alias alone is not usable from other modules.
@saksham-chawla saksham-chawla marked this pull request as ready for review March 18, 2026 20:00
Copy link

@propel-code-bot propel-code-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor maintainability suggestion to add a QNX platform constraint for the public target.

Status: Minor Suggestions | Risk: Low | Readiness: 8/10 (Mostly Ready)

Issues Identified & Suggestions
  • Add QNX platform constraint to prevent misuse of public target: score/message_passing/BUILD
Review Details

📁 1 files reviewed | 💬 1 comments

👍 / 👎 individual comments to help improve reviews for you

visibility = [
"//score/mw/com/impl:__subpackages__",
],
visibility = ["//visibility:public"],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommended

[Maintainability] message_passing_qnx_dispatch is now publicly visible, but unlike the test target qnx_dispatch_test (line 263), it lacks a target_compatible_with constraint. Any non-QNX consumer that accidentally depends on this target directly (instead of the message_passing alias which uses select()) will get confusing build failures from QNX-specific dependencies (@score_baselibs//score/os/qnx:channel, etc.).

Add a platform constraint to prevent misuse now that the target is public:

Suggested change
visibility = ["//visibility:public"],
visibility = ["//visibility:public"],
target_compatible_with = ["@platforms//os:qnx"],
Context for Agents
`message_passing_qnx_dispatch` is now publicly visible, but unlike the test target `qnx_dispatch_test` (line 263), it lacks a `target_compatible_with` constraint. Any non-QNX consumer that accidentally depends on this target directly (instead of the `message_passing` alias which uses `select()`) will get confusing build failures from QNX-specific dependencies (`@score_baselibs//score/os/qnx:channel`, etc.).

Add a platform constraint to prevent misuse now that the target is public:
```suggestion
    visibility = ["//visibility:public"],
    target_compatible_with = ["@platforms//os:qnx"],
```

File: score/message_passing/BUILD
Line: 184
Chain of Thought
  • Validation Decision: accepted
  • Confidence: 0.90
  • Likelihood Score: 4/5
  • Likelihood Reason: The target is now publicly visible to anyone in the repo. Without a target_compatible_with constraint, any non-QNX consumer that directly depends on message_passing_qnx_dispatch (rather than the message_passing alias with select()) will get confusing build errors from QNX-specific dependencies. This is especially likely given the public visibility makes the target discoverable and referenceable by all.
  • Suggested Action: keep
  • Tool Calls Used: 4

Reasoning

The comment is accurate and actionable:

  1. Issue is introduced by this PR: In BASE, message_passing_qnx_dispatch had restricted visibility (//score/mw/com/impl:__subpackages__ and a couple of logging subpackages). The PR changes it to //visibility:public. This materially increases the blast radius — any target in the entire repo can now depend on it.

  2. QNX-specific deps confirmed: The target depends on @score_baselibs//score/os/qnx:channel, @score_baselibs//score/os/qnx:dispatch, @score_baselibs//score/os/qnx:iofunc, and @score_baselibs//score/os/qnx:timer — all QNX-only libraries that will fail on non-QNX platforms.

  3. Pattern already exists in the file: The qnx_dispatch_test target at line 266 already uses target_compatible_with = ["@platforms//os:qnx"], establishing this as the expected pattern for QNX-specific targets.

  4. The message_passing alias uses select(): The intended public entry point (the message_passing alias at line 44-51) properly uses select() to choose between QNX and Unix domain implementations. Making the concrete QNX implementation public without a platform constraint creates a footgun — consumers might bypass the alias and depend directly on the QNX target.

  5. Concrete suggestion: The comment provides a specific, copy-paste-ready fix that follows existing patterns in the file.

The only consideration is whether the QNX deps themselves might already carry target_compatible_with constraints (which Bazel would propagate), but even so, adding the constraint explicitly is clearer and follows the existing pattern in the same BUILD file.

Evidence Summary

1. File Snapshot (HEAD)

  • File: score/message_passing/BUILD
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...

2. File Snapshot (BASE)

  • File: score/message_passing/BUILD
1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...

Fix in Cursor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants