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

Fix "Gtk: cannot open display" error on Linux #386

Merged
merged 1 commit into from Jun 20, 2018

Conversation

@garrettr
Copy link
Contributor

garrettr commented Jun 19, 2018

Fixes #385.

Chromium's GTK frontend requires the $DISPLAY environment variable to be set. $DISPLAY is set correctly by default in a graphical session on Ubuntu 16.04, but was not being inherited by the child process spawned in util.run. Environment variables should be inherited by default according to the child_process docs, but this behavior was broken by the following recent-ish change:

if (!options.env) options.env = {}

@RyanJarv Can you confirm that this PR does not otherwise break the intended behavior of #229, which introduced the bug? If it does, we'll need a more sophisticated fix. Generally I think we always want to inherit the environment in subprocesses spawned by the yarn scripts, since we/Chromium use environment variables (not just $DISPLAY) for a variety of purposes.

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.

Test Plan:

Same as #385 STR.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
@garrettr garrettr requested a review from RyanJarv Jun 19, 2018
@RyanJarv
Copy link
Contributor

RyanJarv commented Jun 20, 2018

Thanks for tracking this down! This was unintended and doesn't seem to affect my fix as far as I can tell, believe it was left around after a few reverts/rebases when I was working on that.

Plan to add some tests here since these scripts are pretty easy to break, will make sure to add a test for this as well.

@garrettr garrettr merged commit 648ebd9 into master Jun 20, 2018
@garrettr garrettr deleted the fix-gtk-cannot-open-display branch Jun 20, 2018
@bbondy
Copy link
Member

bbondy commented Jun 25, 2018

Thanks for fixing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.