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.

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

Allow openroad gui to run inside "make mount" #740

Merged
merged 2 commits into from
Nov 30, 2021

Conversation

maliberty
Copy link
Member

Signed-off-by: Matt Liberty mliberty@eng.ucsd.edu

Signed-off-by: Matt Liberty <mliberty@eng.ucsd.edu>
@maliberty maliberty requested a review from donn November 30, 2021 01:03
@maliberty
Copy link
Member Author

This works for me and mostly addresses #484

@maliberty
Copy link
Member Author

@msaligane FYI

@msaligane
Copy link
Contributor

Yes, works for me too. Thanks @maliberty

Copy link
Collaborator

@donn donn left a comment

Choose a reason for hiding this comment

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

This is already supported by the current Makefile if you pass the environment variable DOCKER_DISPLAY=1.

@maliberty
Copy link
Member Author

The DISPLAY_OPTION is missing the mapping of .Xauthority so it doesn't work for me. I don't see any reason for it to be an option (it's never harmful to have and non-obvious). #484 was not updated with this info. I don't see it in the docs.

I'll update this PR to handle the "ifeq ($(UNAME_S),Linux)" bit.

Signed-off-by: Matt Liberty <mliberty@eng.ucsd.edu>
@maliberty maliberty requested a review from donn November 30, 2021 04:00
@donn
Copy link
Collaborator

donn commented Nov 30, 2021

The DISPLAY_OPTION is missing the mapping of .Xauthority so it doesn't work for me. I don't see any reason for it to be an option (it's never harmful to have and non-obvious). #484 was not updated with this info. I don't see it in the docs.

As we've discussed in our private weekly sync, I had no intention to document this option because I don't want to go around supporting different X setups. The .Xauthority thing is a perfect example: mine works fine without it, but yours won't. There's also the thing with ssh and --network host.

I'm not opposed to turning it on by default given your statement that it's never harmful, however.

@donn donn merged commit 540e2a2 into The-OpenROAD-Project:master Nov 30, 2021
@maliberty
Copy link
Member Author

I expect the reason is that I ssh into another machine where I run the container and the DISPLAY is not local.

@maliberty maliberty deleted the gui branch November 30, 2021 09:46
@umarcor
Copy link

umarcor commented Nov 30, 2021

As explained in the references provided by @mithro above, I strongly recommend using mviereck/x11docker and mviereck/runx (on Windows) in order to run GUI applications in containers.

See https://joss.theoj.org/papers/10.21105/joss.01349 and https://github.com/mviereck/x11docker/wiki.

Disclaimer: I've been involved in testing x11docker on Fedora and on Windows, as I pushed for adding Windows support to x11docker in order to ditch my multi-platforms scripts/makefiles.

@maliberty
Copy link
Member Author

@umarcor It would be great if you can jump in with a PR implementing a better solution. I just got tired of people saying it doesn't work and passing around workarounds on gitter/slack.

@donn
Copy link
Collaborator

donn commented Dec 1, 2021

@umarcor @maliberty +1 - please do be advised though, if this changes our current container build system significantly or impedes headless operation in any form, it will not be merged.

The GUI support continues to be very much an at-your-own-risk-type deal.

@umarcor
Copy link

umarcor commented Dec 1, 2021

@maliberty unfortunately I am not familiar enough with the use cases of the makefiles in this repo, and why/when do users need a GUI and when they do not. It seems that a single Makefile is used for multiple use cases, including running tests, unattended usage or interactive usage. Hence, I can only suggest how to proceed by looking at the current mount target. The following target should be equivalent:

x11docker:
	cd $(OPENLANE_DIR) && \
		x11docker -i --user=0 $(X11DOCKER_OPTIONS) -- \
			-e PDK_ROOT=$(PDK_ROOT) \
			-v $(OPENLANE_DIR):/openlane \
			-v $(PDK_ROOT):$(PDK_ROOT) \
			$(DOCKER_OPTIONS) \
			-- $(OPENLANE_IMAGE_NAME) \
			bash

At the same time, remove from DOCKER_OPTIONS all the stuff related to DISPLAY, xauth, etc. (x11docker takes care of it).

X11DOCKER_OPTIONS can be used for providing custom options to x11docker. For instance, on MSYS2 I would use X11DOCKER_OPTIONS=--runx --no-auth, because I use VcxSrv as the X server.

@donn, the x11docker target should be completely unrelated to the build system, the execution of tests, or any other usage of the makefile. It is used for passing PDK_ROOT, OPENLANE_DIR, DOCKER_OPTIONS and OPENLANE_IMAGE_NAME to the container. Any other approach to pass those arguments would work. In fact, I believe that the x11docker target might be written in a different makefile, which includes the current one.

@grant-h
Copy link

grant-h commented Mar 30, 2023

Note that on Ubuntu 20.04 --security-opt apparmor:unconfined is also required to be added to DOCKER_OPTIONS. Otherwise you'll get issues like audit: type=1107 audit(1680185873.107:2557): pid=1232 uid=103 auid=4294967295 ses=4294967295 subj=unconfined msg='apparmor="DENIED" operation="dbus_signal" in dmesg when running KLayout which says:

OpenLane Container (4cd0986):/openlane$ klayout -e -nn $PDK_ROOT/sky130A/libs.tech/klayout/tech/sky130A.lyt \
>    -l $PDK_ROOT/sky130A/libs.tech/klayout/tech/sky130A.lyp \
>    ./designs/spm/runs/openlane_test/results/final/gds/spm.gds
process 13: The last reference on a connection was dropped without closing the connection. This is a bug in an application. See dbus_connection_unref() documentation for details.
Most likely, the application was supposed to call dbus_connection_close(), since this is a private connection.
  D-Bus not built with -rdynamic so unable to print a backtrace

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.

6 participants