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

powershell: fix the build #41907

Merged
merged 2 commits into from Jun 13, 2018
Merged

powershell: fix the build #41907

merged 2 commits into from Jun 13, 2018

Conversation

yrashk
Copy link
Contributor

@yrashk yrashk commented Jun 13, 2018

For some reason, building powershell derivation started failing with
these errors:

  liblttng-ust.so.0 -> not found!
  ...
  libpam.so.0 -> not found!

This patch adds these dependencies

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@matthewbauer
Copy link
Member

I think you will need to add it to the library path for this to fix your problem.

@yrashk
Copy link
Contributor Author

yrashk commented Jun 13, 2018

@matthewbauer I can do that, however currently .so's referring to these libraries resolve well:

~/P/n/nixpkgs (powershell-fix|…) $ ldd /nix/store/wb1nbgl7kav2wdsy6hbb20ngznwpbzma-powershell-6.0.2/share/powershell/*.so | grep lttng
	liblttng-ust.so.0 => /nix/store/08ziz6wz4gznjmn5j3n7g9jk5ydk7r3j-lttng-ust-2.10.1/lib/liblttng-ust.so.0 (0x00007fef06584000)
	liblttng-ust-tracepoint.so.0 => /nix/store/08ziz6wz4gznjmn5j3n7g9jk5ydk7r3j-lttng-ust-2.10.1/lib/liblttng-ust-tracepoint.so.0 (0x00007fef054c7000)
~/P/n/nixpkgs (powershell-fix|…) $ ldd /nix/store/wb1nbgl7kav2wdsy6hbb20ngznwpbzma-powershell-6.0.2/share/powershell/*.so | grep pam
	libpam.so.0 => /nix/store/bx17bcz9z9gh7bjh22vi3qk3s8206izw-linux-pam-1.3.0/lib/libpam.so.0 (0x00007fed8c8a6000)
	libpam.so.0 => /nix/store/bx17bcz9z9gh7bjh22vi3qk3s8206izw-linux-pam-1.3.0/lib/libpam.so.0 (0x00007f10ec332000)

Do you think I need to cover that in makeLibraryPath anyway?

@yrashk
Copy link
Contributor Author

yrashk commented Jun 13, 2018

Amended the patch to include all libraries in the wrapper.

Mic92
Mic92 reviewed Jun 13, 2018
View reviewed changes
pkgs/shells/powershell/default.nix Outdated
@@ -22,7 +23,7 @@ stdenv.mkDerivation rec {
};

buildInputs = [ autoPatchelfHook makeWrapper ];
propagatedBuildInputs = [ libunwind libuuid icu curl cacert less openssl ];
propagatedBuildInputs = [ cacert less ] ++ libraries;
Copy link
Member

Choose a reason for hiding this comment

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

Why are all those packages in propagatedBuildInputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps I don't fully understand that part yet. The intention here was to submit that those are runtime dependencies, and not explosively build-time ones. If there's another, correct way to do this, please let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Nix figures out runtime dependencies by inspecting what files are referenced by a derivation.
Sometimes it cannot do this if for example an program do PATH search to run a different program.
In this case we use for example makeWrapper to extend the programs PATH.
propagatedBuildInputs will only be pulled if it this package is used as an build input for a different package.
This is also the case for nix-shell by accident, which is why people sometimes think propagatedBuildInputs affect the runtime dependencies. Python is an exception to this rule since it uses propagatedBuildInputs in its own wrapper to extend the PYTHONPATH to contain all required python libraries used in an python script.

Copy link
Member

Choose a reason for hiding this comment

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

less is already covered by the PAGER variable. Where is cacert used in powershell?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for PAGER, this is because there's a bug handling PAGER with arguments in the current release and there's a comment in this in the original nix file for PowerShell:

# TODO: remove PAGER after upgrading to v6.1.0-preview.1 or later as it has been addressed in
# https://github.com/PowerShell/PowerShell/pull/6144

Without this setting, current version will fail commands like Help:

PS /home/yrashk/Projects/nix/nixpkgs> help                                                                                                                                                                          
Get-Command : The term 'less -R' is not recognized as the name of a cmdlet, function, script file, or operable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.

Once 6.1 is released, this can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've simplified build inputs to make this a much cleaner derivation as per your guidance.

Mic92
Mic92 reviewed Jun 13, 2018
View reviewed changes
pkgs/shells/powershell/default.nix
@@ -31,7 +32,7 @@ stdenv.mkDerivation rec {
mkdir -p $out/share/powershell
cp -r * $out/share/powershell
rm $out/share/powershell/DELETE_ME_TO_DISABLE_CONSOLEHOST_TELEMETRY
makeWrapper $out/share/powershell/pwsh $out/bin/pwsh --prefix ${platformLdLibraryPath} : "${stdenv.lib.makeLibraryPath [ libunwind libuuid icu openssl curl ]}" \
makeWrapper $out/share/powershell/pwsh $out/bin/pwsh --prefix ${platformLdLibraryPath} : "${stdenv.lib.makeLibraryPath libraries}" \
Copy link
Member

@Mic92 Mic92 Jun 13, 2018 •
edited

Choose a reason for hiding this comment

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

Are the packages opened with dlopen? Adding libraries to library paths is probably only required on macOS, where no patchelf is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they are not explicitly linked to this libraries, that's why this approach was used in the first place.

This patch doesn't change that, just adds more libraries that /for some reason/ weren't an issue before.

@yrashk
powershell: fix the build
7a0b913 
For some reason, building powershell derivation started failing with
these errors:

```
  liblttng-ust.so.0 -> not found!
  ...
  libpam.so.0 -> not found!
```

This patch adds these dependencies
@Mic92 Mic92 merged commit 9d9f6bc into NixOS:master Jun 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@yrashk @matthewbauer @Mic92 @GrahamcOfBorg