tcl: add a fallback from hardcoded TCLRL_LIBRARY path #5950

oharboe · 2024-10-15T11:14:54Z

TL;DR This fixes the last niggling little deployment problem with non-Ubuntu systems and bazel-orfs GUI.

Not pretty, but good 'nuf to use around the house.

Signed-off-by: Øyvind Harboe <[email protected]>

github-actions · 2024-10-15T11:18:06Z

clang-tidy review says "All clean, LGTM! 👍"

gadfort · 2024-10-15T12:05:53Z

@oharboe why is this needed? If TCL readline is installed correctly the TCLRL_LIBRARY should be defined in the header:
https://github.com/flightaware/tclreadline/blob/20f1425cec34eeef8f24f1e942e0be4038b8df3a/tclreadline.h.in#L16
Also not a huge fan of hardcoding the version number into the embedded tcl script, second, you should either define this in a namespace for tcl do "find_path" isn't just a random function in the tcl environment or you need to cleanup after yourself.

I would like to know better what the actual issue was that this is addressing, since in a headless build for bazel, why add tclreadline?

oharboe · 2024-10-15T12:11:59Z

@oharboe why is this needed?

It is the final hack at the end of a long story...

If TCL readline is installed correctly the TCLRL_LIBRARY should be defined in the header: https://github.com/flightaware/tclreadline/blob/20f1425cec34eeef8f24f1e942e0be4038b8df3a/tclreadline.h.in#L16

Also not a huge fan of hardcoding the version number into the embedded tcl script,

Will fix.

second, you should either define this in a namespace for tcl do "find_path" isn't just a random function in the tcl environment or you need to cleanup after yourself.

Thanks! I think I can do without a proc entirely. Will fix.

I would like to know better what the actual issue was that this is addressing, since in a headless build for bazel, why add tclreadline?

In bazel we also use the user-interface.

In bazel-orfs we use the Docker image(as it is the point in the CI infrastructure that we can pick up a build), but we run the docker image without docker, which is possible if you run through some hoops.

This is the niggling little edge case that we didn't find out a way to sort out otherwise.

It doesn't change the behavior for any case where the library is found today, so hopefully the cruft will be acceptable.

Signed-off-by: Øyvind Harboe <[email protected]>

gadfort · 2024-10-15T12:19:34Z

@oharboe why is this needed?

It is the final hack at the end of a long story...

If this cruft is to be "acceptable" I think it's worth knowing what the story is. Keep in mind there are no comments or anything describing the issue, so the next person who encounters this code will wonder why this is needed.

This feels like there is an issue with your tclreadline install and it would be better to fix that than have some code that isn't in opensta or the tclshrl builds.

github-actions · 2024-10-15T12:22:57Z

clang-tidy review says "All clean, LGTM! 👍"

oharboe · 2024-10-15T12:30:43Z

@gadfort Added some gory details on why we have to look up the path dynamically.

Signed-off-by: Øyvind Harboe <[email protected]>

github-actions · 2024-10-15T12:33:00Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-10-15T12:34:20Z

clang-tidy review says "All clean, LGTM! 👍"

oharboe · 2024-10-15T14:00:59Z

@maliberty I don't understand the pr-merge failure, pr-head passes and this PR is on top of the most recent master, so pr-merge and pr-head should be identical.

maliberty · 2024-10-15T14:02:42Z

src/Main.cc

+  // https://github.com/The-OpenROAD-Project/bazel-orfs/blob/main/docker.BUILD.bazel
+  // for the details on how this is done.


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.

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

maliberty · 2024-10-15T14:02:57Z

src/Main.cc

+  // 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.


Why do you want to do this?

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.

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.

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.

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.

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

I believe they use a monorepo and build from source.

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...

src/Main.cc

maliberty · 2024-10-15T16:36:37Z

This feels like there is an issue with your tclreadline install and it would be better to fix that than have some code that isn't in opensta or the tclshrl builds.

This is a good point. The sta app will not work with this approach. I believe you are sta for power reporting.

Signed-off-by: Øyvind Harboe <[email protected]>

github-actions · 2024-10-15T17:36:12Z

clang-tidy review says "All clean, LGTM! 👍"

oharboe · 2024-10-15T20:19:26Z

@maliberty mystery pr-merge error?

maliberty · 2024-10-15T22:26:37Z

I see the same two tests failed in many builds:
626 - psm.gcd_write_sp_test_vdd (Failed)
628 - psm.gcd_em_test_vdd (Failed)

I think it worth taking a look at it doesn't seem to be some random flakiness.

oharboe · 2024-10-16T06:21:27Z

@maliberty ctest --test-dir build -j 32 had failures locally without #5961

I propose to merge this as-is or that I wait for #5961 to be merged, then merge with master and do a rebuild of this PR.

github-actions · 2024-10-16T06:41:50Z

clang-tidy review says "All clean, LGTM! 👍"

tcl: add a fallback from hardcoded TCLRL_LIBRARY path

ae6a38a

Signed-off-by: Øyvind Harboe <[email protected]>

oharboe mentioned this pull request Oct 15, 2024

gui: less click+wait, populate timing paths as part of loading timing The-OpenROAD-Project/OpenROAD-flow-scripts#2445

Merged

tcl: clean up code to get path to tclreadline library

5ff3bb2

Signed-off-by: Øyvind Harboe <[email protected]>

tcl: add some narrative on why we have to look up the path

cdfabc7

Signed-off-by: Øyvind Harboe <[email protected]>

oharboe force-pushed the tcl-init-workaround branch from c50f37e to cdfabc7 Compare October 15, 2024 12:31

oharboe requested a review from maliberty October 15, 2024 14:00

maliberty reviewed Oct 15, 2024

View reviewed changes

src/Main.cc Outdated Show resolved Hide resolved

tclinit: fallback case only is changed

1858ebd

Signed-off-by: Øyvind Harboe <[email protected]>

oharboe requested a review from maliberty October 15, 2024 17:33

Merge remote-tracking branch 'origin/master' into HEAD

aee810a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tcl: add a fallback from hardcoded TCLRL_LIBRARY path #5950

tcl: add a fallback from hardcoded TCLRL_LIBRARY path #5950

oharboe commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024

gadfort commented Oct 15, 2024

oharboe commented Oct 15, 2024

gadfort commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

oharboe commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

oharboe commented Oct 15, 2024

maliberty Oct 15, 2024

oharboe Oct 15, 2024

maliberty Oct 15, 2024

oharboe Oct 15, 2024

oharboe Oct 15, 2024

gadfort Oct 15, 2024

oharboe Oct 15, 2024

gadfort Oct 16, 2024

maliberty Oct 16, 2024

oharboe Oct 16, 2024

maliberty commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

oharboe commented Oct 15, 2024

maliberty commented Oct 15, 2024

oharboe commented Oct 16, 2024

github-actions bot commented Oct 16, 2024

		// https://github.com/The-OpenROAD-Project/bazel-orfs/blob/main/docker.BUILD.bazel
		// for the details on how this is done.

tcl: add a fallback from hardcoded TCLRL_LIBRARY path #5950

Are you sure you want to change the base?

tcl: add a fallback from hardcoded TCLRL_LIBRARY path #5950

Conversation

oharboe commented Oct 15, 2024 • edited Loading

github-actions bot commented Oct 15, 2024

gadfort commented Oct 15, 2024

oharboe commented Oct 15, 2024

gadfort commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

oharboe commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

oharboe commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maliberty commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

oharboe commented Oct 15, 2024

maliberty commented Oct 15, 2024

oharboe commented Oct 16, 2024

github-actions bot commented Oct 16, 2024

oharboe commented Oct 15, 2024 •

edited

Loading