Skip to content

Commit

Permalink
ntl: Add darwin support
Browse files Browse the repository at this point in the history
  • Loading branch information
@jbaum98
jbaum98 committed Aug 13, 2018
1 parent 0371570 commit 1bfbe76
Showing 1 changed file with 4 additions and 0 deletions.
4 changes: 4 additions & 0 deletions pkgs/development/libraries/ntl/default.nix
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ stdenv.mkDerivation rec {
sha256 = "04avzmqflx2a33n7v9jj32g83p7m6z712fg1mw308jk5ca2qp489";
};

patchPhase = stdenv.lib.optionalString stdenv.isDarwin ''
substituteInPlace DoConfig --replace g++ c++
'';

buildInputs = [
gmp
];
Expand Down

15 comments on commit 1bfbe76

@timokau
Copy link
Member

Choose a reason for hiding this comment

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

@victorshoup do you think this should be upstreamed? Darwin doesn't have g++, requiring this patch to build ntl on darwin.

@vcunat
Copy link
Member

@vcunat vcunat commented on 1bfbe76 Aug 18, 2018

Choose a reason for hiding this comment

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

I think so. BTW, stdenv.cc.isClang would be more precise, though I don't know about any other standard nixpkgs configurations using clang stdenv.

@timokau
Copy link
Member

Choose a reason for hiding this comment

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

For the record: @victorshoup is the upstream ntl maintainer (author?), which is why I pinged him here.

@jbaum98
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a side note if this commit is under discussion, I think a cleaner way to do this would have been to set the CXX makefile variable.

@victorshoup
Copy link

@victorshoup victorshoup commented on 1bfbe76 Aug 18, 2018 via email

Choose a reason for hiding this comment

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

@victorshoup
Copy link

Choose a reason for hiding this comment

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

Not sure what the issues are exactly, but why not: ./configure CXX=c++
NOTE: NTL's configure script does not read environment variables (I admit that it probably should), so export CXX=c++; ./configure doesn't work.

@globin
Copy link
Member

@globin globin commented on 1bfbe76 Aug 18, 2018

Choose a reason for hiding this comment

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

By default the configure script should use c++ instead of g++ as both GCC and llvm/clang provide that.

@victorshoup
Copy link

Choose a reason for hiding this comment

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

I didn't realize that! Maybe I should change NTL's configure script to make that the default. There used to be some arcane reason why I preferred g++ to c++, but I don't remember what it was.

@victorshoup
Copy link

@victorshoup victorshoup commented on 1bfbe76 Aug 18, 2018 via email

Choose a reason for hiding this comment

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

@jbaum98
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should I make a new pull request to use configure flags instead of modifying DoConfig?

@victorshoup
Copy link

@victorshoup victorshoup commented on 1bfbe76 Aug 18, 2018 via email

Choose a reason for hiding this comment

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

@vcunat
Copy link
Member

@vcunat vcunat commented on 1bfbe76 Aug 19, 2018

Choose a reason for hiding this comment

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

OK, pushed directly as ef6b76c, for simplicity.

@timokau
Copy link
Member

Choose a reason for hiding this comment

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

Any reason the configure script doesn't respect environment variables? I think if that was the case our build system would take care of it automatically.

@victorshoup
Copy link

@victorshoup victorshoup commented on 1bfbe76 Aug 19, 2018 via email

Choose a reason for hiding this comment

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

@timokau
Copy link
Member

Choose a reason for hiding this comment

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

Okay, thanks for your input :) The solution now is definitely good enough for us, just consider it a "nice to have" feature request at the bottom of your priority list.

Please sign in to comment.