Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tcl: add a fallback from hardcoded TCLRL_LIBRARY path #5950

Merged
merged 5 commits into from
Oct 16, 2024
Merged

tcl: add a fallback from hardcoded TCLRL_LIBRARY path #5950

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ae6a38a
tcl: add a fallback from hardcoded TCLRL_LIBRARY path
oharboe Oct 15, 2024
5ff3bb2
tcl: clean up code to get path to tclreadline library
oharboe Oct 15, 2024
cdfabc7
tcl: add some narrative on why we have to look up the path
oharboe Oct 15, 2024
1858ebd
tclinit: fallback case only is changed
oharboe Oct 15, 2024
aee810a
Merge remote-tracking branch 'origin/master' into HEAD
oharboe Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 52 additions & 1 deletion src/Main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,53 @@ static int tclReadlineInit(Tcl_Interp* interp)
}
#endif

namespace {
// A stopgap fallback from the hardcoded TCLRL_LIBRARY path for OpenROAD,
// not essential for OpenSTA
std::string findPathToTclreadlineInit(Tcl_Interp* interp)
{
// TL;DR it is possible to run the OpenROAD binary from within the
// official Docker image on a different distribution than the
// distribution within the Docker image.
Comment on lines +344 to +346
Copy link
Member

Choose a reason for hiding this comment

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

Why do you want to do this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because running docker in bazel-orfs is extremely problematic; don't do it. Because I want CI, docker provides this. Because this is a solution that exists and is working, less this niggling litt nit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As to your question why I "want to dothis", I think it is more of a question as to which poison I want to pick. I have picked this poison.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems like there is something not right about the usecase, which is leading to this solution. Personally, I would prefer not to bring this in, if the solution is to properly build OpenROAD for bazel (or whatever platform) instead of building on one platform and moving the binary around.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is not one or the other; there's no conflict here.

This PR is boarding up a broken window (lack of resources for CI + programming), longer term I hope to clean up bazel-orfs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How does @QuantamHD handle the OpenROAD build for his bazel setup? He must have encountered a similar issue

Copy link
Member

Choose a reason for hiding this comment

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

I believe they use a monorepo and build from source.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also saw some patching of OpenROAD going on, so my short answer is that it would take a bit of time to investigate what they are doing.

In the future, I'd like to see an officially maintained bazel build of OpenROAD with an artifact repository that I can use, just like Docker images. Would save tons of building time for anyone involved + be single source of truth + better chance of everyone running the exact same version w.r.t. bug reports and communicating in the community...

Perhaps bazel_hdl could use it too?

That's not happening tomorrow though...

//
// In this case we have to look up
// the location of the tclreadline scripts instead of using the hardcoded
// path.
//
// It is helpful to use the official Docker image as CI infrastructure and
// also because it is a good way to have as similar an environment as possible
// during testing and deployment.
//
// See
// https://github.com/The-OpenROAD-Project/bazel-orfs/blob/main/docker.BUILD.bazel
// for the details on how this is done.
Comment on lines +357 to +358
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any details there that explain how it is done. Its just a list of files and there is no mention of docker.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Documentation on how this is done belongs in bazel-orfs. Admittedly it could be better.

//
// Running Docker within a bazel isolated environment introduces lots of
// problems and is not really done.
const char* tclScript = R"(
namespace eval temp {
foreach dir $::auto_path {
set folder [file join $dir]
set path [file join $folder "tclreadline)" TCLRL_VERSION_STR
R"(" "tclreadlineInit.tcl"]
if {[file exists $path]} {
return $path
}
}
error "tclreadlineInit.tcl not found in any of the directories in auto_path"
}
)";

if (Tcl_Eval(interp, tclScript) == TCL_ERROR) {
std::cerr << "Tcl_Eval failed: " << Tcl_GetStringResult(interp)
<< std::endl;
return "";
}

return Tcl_GetStringResult(interp);
}
} // namespace

// Tcl init executed inside Tcl_Main.
static int tclAppInit(int& argc,
char* argv[],
Expand Down Expand Up @@ -376,9 +423,13 @@ static int tclAppInit(int& argc,
// script is done.
Tcl_StaticPackage(
interp, "tclreadline", Tclreadline_Init, Tclreadline_SafeInit);

if (Tcl_EvalFile(interp, TCLRL_LIBRARY "/tclreadlineInit.tcl")
!= TCL_OK) {
printf("Failed to load tclreadline\n");
std::string path = findPathToTclreadlineInit(interp);
if (path.empty() || Tcl_EvalFile(interp, path.c_str()) != TCL_OK) {
printf("Failed to load tclreadline\n");
}
}
}
#endif
Expand Down
Loading