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

chromedriver: add dbus to libraries, correct LD_LIBRARY_PATH wrapping #139723

Merged
merged 4 commits into from
Sep 28, 2021

Conversation

ivan-timokhin
Copy link
Contributor

@ivan-timokhin ivan-timokhin commented Sep 28, 2021

It is apparently required since version 94.

Fixes #139547

Motivation for this change

Chromedriver doesn’t run otherwise.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

It is apparently required since version 94.

Fixes issue NixOS#139547
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Sep 28, 2021
wrapProgram already prepends passed value to the specified environment
variable; no need to specify it explicitly.
@ivan-timokhin
Copy link
Contributor Author

Incidentally, a couple of questions:

  1. Should I add a test to check that chromedriver runs (essentially copy-pasting from the manual), to avoid similar accidents in the future?
  2. The resulting wrapper script includes the original LD_LIBRARY_PATH twice (the new value ends with :$LD_LIBRARY_PATH'${LD_LIBRARY_PATH:+':'}$LD_LIBRARY_PATH), so it seems that the explicit reference to $LD_LIBRARY_PATH in wrapProgram invocation is redundant; should I remove it while I’m here?

If there are no obvious downsides to these changes that I’m oblivious to, should they go into this PR or separate ones?

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Sep 28, 2021

Should I add a test to check that chromedriver runs (essentially copy-pasting from the manual), to avoid similar accidents in the future?

If you want to.

The resulting wrapper script includes the original LD_LIBRARY_PATH twice (the new value ends with :$LD_LIBRARY_PATH'${LD_LIBRARY_PATH:+':'}$LD_LIBRARY_PATH), so it seems that the explicit reference to $LD_LIBRARY_PATH in wrapProgram invocation is redundant; should I remove it while I’m here?

Yes please, line 50 should be.

wrapProgram "$out/bin/chromedriver" --prefix LD_LIBRARY_PATH : "${libs}"

If there are no obvious downsides to these changes that I’m oblivious to, should they go into this PR or separate ones?

Just add them to this one.

@ivan-timokhin
Copy link
Contributor Author

Thanks!

Comment on lines 54 to 56
passthru.tests = {
version-check = callPackage ./tests.nix { };
};

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
passthru.tests = {
version-check = callPackage ./tests.nix { };
};
passthru.tests.version =
testVersion { package = chromedriver; };

#121896

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

@SuperSandro2000 SuperSandro2000 changed the title chromedriver: add dbus to libraries chromedriver: add dbus to libraries, correct LD_LIBRARY_PATH wrapping Sep 28, 2021
@SuperSandro2000 SuperSandro2000 merged commit 56cf4e1 into NixOS:master Sep 28, 2021
@ivan-timokhin ivan-timokhin deleted the chromedriver-add-dbus branch September 29, 2021 05:07
@github-actions
Copy link
Contributor

Successfully created backport PR #139838 for release-21.05.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chromedriver can’t find libdbus and crashes
3 participants