Skip to content

rmp-py.i: include the same headers for bazel/non-bazel#10641

Open
hzeller wants to merge 1 commit into
The-OpenROAD-Project:masterfrom
hzeller:feature-20260611-rmp-inc
Open

rmp-py.i: include the same headers for bazel/non-bazel#10641
hzeller wants to merge 1 commit into
The-OpenROAD-Project:masterfrom
hzeller:feature-20260611-rmp-inc

Conversation

@hzeller

@hzeller hzeller commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

In the bazel version, there were a bunch of header files missing.

In the bazel version, there were a bunch of header files missing.

Signed-off-by: Henner Zeller <h.zeller@acm.org>
@hzeller hzeller requested a review from a team as a code owner June 11, 2026 15:25
@hzeller hzeller requested a review from QuantamHD June 11, 2026 15:25
@hzeller

hzeller commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator Author

it is unclear to me why there is a difference between Bazel and non-bazel compilation, but I also don't know the details of Swig.

At least now the difference between these two Versions is minimized.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request adds the "//src/dbSta" dependency to the "src/rmp" Bazel build and reorganizes includes and forward declarations in "src/rmp/src/rmp-py.i". A critical issue was identified where removing forward declarations from the SWIG section (outside the "%{ ... %}" block) will prevent SWIG from recognizing these types under "#ifdef BAZEL", potentially leading to incorrect type wrappers or runtime errors. It is recommended to restore these forward declarations.

Comment thread src/rmp/src/rmp-py.i
Comment on lines +47 to 49
#ifdef BAZEL
namespace cut {
class Blif

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

By removing the forward declarations of utl::Logger, odb::dbBlock, odb::dbInst, and sta::dbSta from the SWIG section (outside the %{ ... %} block), SWIG will no longer recognize these types when parsing the manual declaration of class Blif under #ifdef BAZEL.

Note that the %{ ... %} block is only visible to the C++ compiler and is completely ignored by SWIG. Therefore, moving or keeping these declarations only inside %{ ... %} does not help SWIG. Without these forward declarations in the SWIG section, SWIG may generate incorrect type wrappers (e.g., failing to map them to the correct Python class proxies or generating type mismatch warnings/errors at runtime).

We should restore these forward declarations in the SWIG section under #ifdef BAZEL.

#ifdef BAZEL
namespace utl {
class Logger;
}

namespace odb {
class dbBlock;
class dbInst;
}

namespace sta {
class dbSta;
}

namespace cut {
class Blif

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

//src/rmp:_rmp_py.so compiles successfully.

@hzeller

hzeller commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

did the CI get stuck ?

@maliberty

Copy link
Copy Markdown
Member

ci is done

@maliberty

Copy link
Copy Markdown
Member

it is unclear to me why there is a difference between Bazel and non-bazel compilation, but I also don't know the details of Swig.

I went down the rabbit hole on this question. In short bazel is trying to make modules that can be loaded standalone into python3 as wheels and cmake is not. This is a bad idea as it means each lib has a mostly full copy of OR. It is a bigger project to fix this.

@hzeller

hzeller commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

ok, but having the necessary headers should not make a difference, right ? The size of the resulting bazel-bin/src/rmp/_rmp_py.so is the same in both cases.

@maliberty

Copy link
Copy Markdown
Member

Agreed, just wondered myself

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants