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

Xournal: Darwin support #21400

Merged
merged 2 commits into from Dec 27, 2016
Merged

Xournal: Darwin support #21400

merged 2 commits into from Dec 27, 2016

Conversation

johbo
Copy link
Contributor

@johbo johbo commented Dec 25, 2016

Motivation for this change

This PR has two small changes:

  1. It allows to build Xournal on Darwin.
  2. In enables xinput also on Darwin, so that e.g. pressure sensitivity works in Xournal.
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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.

@mention-bot
Copy link

@johbo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jwiegley, @dguibert and @lverns to be potential reviewers.

@LnL7 LnL7 added the 6.topic: darwin Running or building packages on Darwin label Dec 25, 2016
LnL7
LnL7 reviewed Dec 25, 2016
View reviewed changes
pkgs/applications/graphics/xournal/default.nix
@@ -1,8 +1,10 @@
{ stdenv, fetchurl, makeDesktopItem
{ stdenv, lib, fetchurl, makeDesktopItem
Copy link
Member

Choose a reason for hiding this comment

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

You can access lib using stdenv.lib

pkgs/applications/graphics/xournal/default.nix
@@ -50,8 +54,8 @@ stdenv.mkDerivation rec {
meta = {
Copy link
Member

Choose a reason for hiding this comment

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

You can use meta = with stdenv.lib; ... to get the attributes like licenses into scope here.

@johbo
Copy link
Contributor Author

johbo commented Dec 25, 2016

Updated the style.

@LnL7 is using lib in the attributes at the head discouraged? I was not too sure about it, think I saw different styles in different files.

mdaiter
mdaiter suggested changes Dec 25, 2016
View reviewed changes
Copy link
Contributor

@mdaiter mdaiter left a comment

Choose a reason for hiding this comment

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

Good, but a bit of touch-up requested

pkgs/applications/graphics/xournal/default.nix
@@ -2,7 +2,9 @@
, ghostscript, atk, gtk2, glib, fontconfig, freetype
, libgnomecanvas, libgnomeprint, libgnomeprintui
, pango, libX11, xproto, zlib, poppler
, autoconf, automake, libtool, pkgconfig}:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed? Seems like it's the same package list, but the end-bracket's on a different line.

pkgs/applications/graphics/xournal/default.nix
platforms = stdenv.lib.platforms.linux;
maintainers = [ maintainers.guibert ];
license = licenses.gpl2;
platforms = platforms.linux ++ platforms.darwin;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be written as platforms = with platforms; [ linux darwin ];

Copy link
Member

Choose a reason for hiding this comment

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

While nested lists should work on hydra, it's not considered to be a best practice.

@LnL7
Copy link
Member

LnL7 commented Dec 26, 2016

is using lib in the attributes at the head discouraged? I was not too sure about it, think I saw different styles in different files.

Not really, but AFAIK stdenv.lib is used a lot more for package definitions, probably because a most packages already need stdenv for mkDerivation, ...

@lverns
Copy link
Contributor

lverns commented Dec 27, 2016

These changes seem to build and run fine for me on NixOS.

@johbo
Copy link
Contributor Author

johbo commented Dec 27, 2016

Updated the style as requested - hope it's ready to go now 😄

LnL7
LnL7 reviewed Dec 27, 2016
View reviewed changes
pkgs/applications/graphics/xournal/default.nix
platforms = stdenv.lib.platforms.linux;
maintainers = [ maintainers.guibert ];
license = licenses.gpl2;
platforms = with platforms; [ linux darwin ];
Copy link
Member

Choose a reason for hiding this comment

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

Attributes in platforms are lists so this will result in nested lists: [ [ "i686-linux" ... ] [ "x86_64-darwin" ] ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LOL, got trapped by the other comments, I'll fix this back again

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the confusion, my previous comment wasn't very clear.

@LnL7 LnL7 merged commit 7d4e04c into NixOS:master Dec 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdaiter mdaiter mdaiter requested changes

@LnL7 LnL7 LnL7 approved these changes

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@johbo @mention-bot @LnL7 @lverns @mdaiter