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

Jump to bottom

libcaca: make x11 optional, disabled on darwin #57687

Merged
merged 1 commit into from Mar 26, 2019
Merged

libcaca: make x11 optional, disabled on darwin #57687

merged 1 commit into from Mar 26, 2019

Conversation

jacereda
Copy link
Contributor

@jacereda jacereda commented Mar 15, 2019 •
edited

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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@jacereda
Copy link
Contributor Author

cacaclock binary fails because it can't find the font, but I guess it isn't related to this change.

aanderse
aanderse requested changes Mar 24, 2019
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

I built this on NixOS and ran the cacademo, cacaview, and cacafire binaries successfully 👍. Just a few minor details to cleanup.

pkgs/development/libraries/libcaca/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/libcaca/default.nix Outdated Show resolved Hide resolved
aanderse
aanderse reviewed Mar 24, 2019
View reviewed changes
pkgs/development/libraries/libcaca/default.nix
(if x11Support then "--enable-x11" else "--disable-x11")
];

NIX_CFLAGS_COMPILE = stdenv.lib.optional (!x11Support) "-DX_DISPLAY_MISSING";
Copy link
Member

Choose a reason for hiding this comment

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

I didn't check the libcaca docs but wanted to make double check just to be sure... explicitly defining X_DISPLAY_MISSING is still required even after passing --enable-x11 to the configure flags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC the code that checks the define is in imlib2. Is there something like a propagatedCPPFlags? I guess it would be cleaner if imlib2-nox propagated that define.

Copy link
Member

Choose a reason for hiding this comment

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

Without digging into that much deeper I'm happy with that answer then. Thanks!

aanderse
aanderse approved these changes Mar 25, 2019
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@aanderse
Copy link
Member

@LnL7 @the-kenny as the 2 most recent people to merge commits for this package are you interested in merging this PR?

@LnL7
Copy link
Member

LnL7 commented Mar 26, 2019

@GrahamcOfBorg build libcaca

@LnL7 LnL7 merged commit b0f4ef9 into NixOS:master Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LnL7 LnL7 LnL7 approved these changes

@aanderse aanderse aanderse approved these changes

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 11-100 10.rebuild-linux: 101-500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jacereda @aanderse @LnL7 @GrahamcOfBorg