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

jsoncpp: 1.9.2 -> 1.9.4 #102379

Merged
merged 1 commit into from Nov 7, 2020
Merged

jsoncpp: 1.9.2 -> 1.9.4 #102379

merged 1 commit into from Nov 7, 2020

Conversation

CrystalGamma
Copy link
Contributor

Merges both patches previously applied. Also seems to change the path that the library is written to.

Motivation for this change

Fixes a build failure on platforms with unsigned char type, which was fixed with a patch on ARM only, but is needed on ppc64le (CC @r-burns) too. Updating to latest upstream release is probably cleaner than adding another platform test to the patch.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@@ -23,27 +23,13 @@ stdenv.mkDerivation rec {
# Hack to be able to run the test, broken because we use
# CMAKE_SKIP_BUILD_RPATH to avoid cmake resetting rpath on install
preBuild = if stdenv.isDarwin then ''
export DYLD_LIBRARY_PATH="`pwd`/src/lib_json''${DYLD_LIBRARY_PATH:+:}$DYLD_LIBRARY_PATH"
export DYLD_LIBRARY_PATH="`pwd`/lib''${DYLD_LIBRARY_PATH:+:}$DYLD_LIBRARY_PATH"
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
export DYLD_LIBRARY_PATH="`pwd`/lib''${DYLD_LIBRARY_PATH:+:}$DYLD_LIBRARY_PATH"
export DYLD_LIBRARY_PATH="$(pwd)/lib''${DYLD_LIBRARY_PATH:+:}$DYLD_LIBRARY_PATH"

'' else ''
export LD_LIBRARY_PATH="`pwd`/src/lib_json''${LD_LIBRARY_PATH:+:}$LD_LIBRARY_PATH"
export LD_LIBRARY_PATH="`pwd`/lib''${LD_LIBRARY_PATH:+:}$LD_LIBRARY_PATH"
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
export LD_LIBRARY_PATH="`pwd`/lib''${LD_LIBRARY_PATH:+:}$LD_LIBRARY_PATH"
export LD_LIBRARY_PATH="$(pwd)/lib''${LD_LIBRARY_PATH:+:}$LD_LIBRARY_PATH"

Copy link
Contributor

@r-burns r-burns left a comment

Choose a reason for hiding this comment

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

Looks good, can confirm the ppc64le issue is now fixed. WFM on darwin and non-nixos linux. Might want to reduce closure size with outputs = [ "out" "dev" ] and "-DBUILD_OBJECT_LIBS=OFF"

Merges both patches previously applied. Also seems to change the path that the library is written to.
@CrystalGamma
Copy link
Contributor Author

Thanks for the feedback. I went ahead and replaced the backtick expansion (can't actually quote it here because I can't get escaping the backticks to work right) by $PWD which is even cleaner than $(pwd) (IMO) and implemented the closure reduction options suggested.

@r-burns r-burns mentioned this pull request Nov 5, 2020
10 tasks
@cpages cpages merged commit 9430fd9 into NixOS:master Nov 7, 2020
@CrystalGamma CrystalGamma deleted the pr-jsoncpp-update branch November 10, 2020 16:03
@austinbutler austinbutler mentioned this pull request Mar 18, 2022
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants