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

Fixes for macos #24700

Merged
merged 3 commits into from Apr 12, 2017
Merged

Fixes for macos #24700

merged 3 commits into from Apr 12, 2017

Conversation

hamishmack
Copy link
Contributor

@hamishmack hamishmack commented Apr 7, 2017 •
edited by LnL7

Motivation for this change

Gets WebKitGTK+, GtkSourceView and gtk-mac-integration working on MacOS

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

@hamishmack, thanks for your PR! By analyzing the history of the files in this pull request, we identified @groxxda, @aszlig and @bendlas to be potential reviewers.

periklis
periklis reviewed Apr 7, 2017
View reviewed changes
Copy link
Contributor

@periklis periklis left a comment

Choose a reason for hiding this comment

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

Thx for the great work, i've proposed some enhnacements.

pkgs/development/libraries/webkitgtk/2.14.nix Outdated
patch -p0 < ${./PR-153138.patch}
patch -p0 < ${./PR-157554.patch}
patch -p0 < ${./PR-157574.patch}
'';
Copy link
Contributor

Choose a reason for hiding this comment

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

Put these in patches and do not use the patch util manually.

pkgs/development/libraries/webkitgtk/2.14.nix Outdated
Source/WebKit2/CMakeFiles/WebProcess.dir/link.txt
sed -i "s|\.\./\.\./lib/libWTFGTK\.a|-Wl,-all_load ../../lib/libWTFGTK.a|" \
Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/link.txt
'';
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible try to use of substituteInPlace

pkgs/development/libraries/webkitgtk/2.14.nix Outdated
@@ -37,24 +61,40 @@ stdenv.mkDerivation rec {
cmakeFlags = [
"-DPORT=GTK"
"-DUSE_LIBHYPHEN=0"
"-DENABLE_GLES2=ON"
];
] ++ (if stdenv.isDarwin then [
Copy link
Contributor

Choose a reason for hiding this comment

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

Use of stdenv.lib.optionals (stdenv.isDarwin) [...] is a much nicer syntax that avoids parenthesis nesting and plays well with additions for other platforms.

Copy link
Member

@Mic92 Mic92 Apr 7, 2017 •
edited

Choose a reason for hiding this comment

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

Might be an exception here, because of the else branch. What would be the other platform? (!stdenv.isDarwin)?

pkgs/development/libraries/webkitgtk/2.14.nix Outdated
libxml2 libsecret libxslt harfbuzz libpthreadstubs libtasn1 p11_kit
gst-plugins-base libxkbcommon epoxy at_spi2_core
] ++ optional enableGeoLocation geoclue2
++ (with xlibs; [ libXdmcp libXt libXtst ]);
++ (with xlibs; [ libXdmcp libXt libXtst ])
++ (if stdenv.isDarwin then [ libedit readline mesa ] else [ wayland ]);
Copy link
Contributor

Choose a reason for hiding this comment

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

As above use stdenv.lib.optionals and make the platform explicit for wayland.

@Mic92 Mic92 added the 6.topic: darwin Running or building packages on Darwin label Apr 7, 2017
@hamishmack hamishmack force-pushed the fixes-for-macos branch 2 times, most recently from d835991 to 90f4361 Compare April 7, 2017 18:58
@hamishmack
Copy link
Contributor Author

Ok I think I have the enhancements right now. @Mic92 I used isLinux for the else case.

LnL7
LnL7 reviewed Apr 8, 2017
View reviewed changes
pkgs/development/libraries/webkitgtk/2.14.nix
patches = [ ./finding-harfbuzz-icu.patch ];
patches = [ ./finding-harfbuzz-icu.patch ]
++ optionals stdenv.isDarwin [
./PR-152650-2.patch
Copy link
Member

Choose a reason for hiding this comment

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

Can you use fetchpatch for these?

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 don't think we can fetch them directly from macports if that is what you were suggesting. I had to modify them slightly. The macports patches are designed to work with patch -p0. I had to add an extra directory to the paths to make them work with patches in nix (90f4361#diff-7e2407d988a384c067352687b99c845e is the commit with that change).

Copy link
Member

@LnL7 LnL7 Apr 9, 2017 •
edited

Choose a reason for hiding this comment

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

Alright

@matthewbauer
Copy link
Member

So if this gets merged I think we can probably close #14284 because it looks like it's roughly the same purpose.

@matthewbauer
Copy link
Member

Also: make sure your commit messages meet the contributing guideles.

@hamishmack
Copy link
Contributor Author

@matthewbauer I have merged the Use nix idiomatic patch and replace functions commit into the first commit and added some detail of on the second commit message.

@LnL7
Copy link
Member

LnL7 commented Apr 10, 2017

It's preferred to format the first like webkgitgtk: fix 2.14 on macOS. Not mandatory, but it makes it easier for people to read the changelog if everything is formatted consistently.

@hamishmack
webkgitgtk: fix 2.14 on macOS
da1d13b 
Includes the patches from macports and fixes an issue with the
`otool -L` output that was causing g-ir-scanner to fail.
The paths in the macports patches have been modified so that they
will work with `patches` in nix (an extra directory level was
added).
@hamishmack
gtksourceview: fix version 3 on macOS
5dfe0f7 
Add darwin to the list of supported platforms for gtksourceview.
@hamishmack
gtk-mac-integration: fix gtk3 version
4d5ed18 
Include a Gtk3 version of gtk-mac-integration.
Add gobjectIntrospection to gtk-mac-integration.
Use propagatedBuildInputs for gtk.
@hamishmack
Copy link
Contributor Author

I have moved the package names to the start of the commit messages. Split the request into three commits (one for each package). I also moved the gtk from buildInputs to propergatedBuildInputs in the gtk-mac-integration package.

@LnL7 LnL7 merged commit 5ba4457 into NixOS:master Apr 12, 2017
@hamishmack hamishmack deleted the fixes-for-macos branch April 12, 2017 23:48
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

@periklis periklis periklis left review comments

@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
6 participants
@hamishmack @mention-bot @matthewbauer @LnL7 @Mic92 @periklis