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

universal-ctags: include libiconv on darwin #29938

Merged
merged 1 commit into from Oct 2, 2017
Merged

universal-ctags: include libiconv on darwin #29938

merged 1 commit into from Oct 2, 2017

Conversation

pmahoney
Copy link
Contributor

Motivation for this change

Failed to compile on darwin.

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 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.

@LnL7 LnL7 added the 6.topic: darwin Running or building packages on Darwin label Sep 29, 2017
@peterhoeg
Copy link
Member

I have noticed that libiconv is often needed on Mac. Would it make sense to have autoreconfHook add it automatically?

Cc: @Ericson2314

Ericson2314
Ericson2314 reviewed Sep 30, 2017
View reviewed changes
pkgs/development/tools/misc/universal-ctags/default.nix Outdated
@@ -12,7 +12,8 @@ stdenv.mkDerivation rec {
};

nativeBuildInputs = [ pythonPackages.docutils ];
buildInputs = [ autoreconfHook pkgconfig ];
buildInputs = [ autoreconfHook pkgconfig ] ++
Copy link
Member

Choose a reason for hiding this comment

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

autoreconfHook and pkgconfig should be native build inputs. I fixed this on staging so doing it here might just help us avoid a merge conflict.

@Ericson2314
Copy link
Member

@peterhoeg I'm not sure how that would work, as we need Nix-level, not just autoconf-level changes? But perhaps I'm missing something.

I've seriously considered trying to build glibc without libiconv, so we can unconditionally use the separate dep everywhere.

@peterhoeg
Copy link
Member

What I meant is that adding autoreconfHook to the nativeBuildInputs will inject autoconf, automake, libtool and gettext as inputs. Couldn't we do exactly the same thing with libiconv when running on darwin from the same hook?

LnL7
LnL7 approved these changes Oct 1, 2017
View reviewed changes
Copy link
Member

@LnL7 LnL7 left a comment

Choose a reason for hiding this comment

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

nitpick

pkgs/development/tools/misc/universal-ctags/default.nix Outdated
@@ -12,7 +12,8 @@ stdenv.mkDerivation rec {
};

nativeBuildInputs = [ pythonPackages.docutils ];
buildInputs = [ autoreconfHook pkgconfig ];
buildInputs = [ autoreconfHook pkgconfig ] ++
(stdenv.lib.optional stdenv.isDarwin [ libiconv ]);
Copy link
Member

Choose a reason for hiding this comment

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

this results in a nested list, use optional stdenv.isDarwin libiconv or optionals stdenv.isDarwin [ libiconv ... ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, thank you!

@pmahoney
Copy link
Contributor Author

pmahoney commented Oct 2, 2017

Updated to (hopefully) avoid merge conflict with @Ericson2314 and to address comment from @LnL7

@LnL7 LnL7 merged commit fced3c2 into NixOS:master Oct 2, 2017
@LnL7
Copy link
Member

LnL7 commented Oct 2, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ericson2314 Ericson2314 Ericson2314 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
4 participants
@pmahoney @peterhoeg @Ericson2314 @LnL7