Skip to content

python: add C++ compiler support for distutils#19585

Merged
FRidh merged 3 commits intoNixOS:stagingfrom
veprbl:distutils_fix
Oct 25, 2016
Merged

python: add C++ compiler support for distutils#19585
FRidh merged 3 commits intoNixOS:stagingfrom
veprbl:distutils_fix

Conversation

@veprbl
Copy link
Member

@veprbl veprbl commented Oct 15, 2016

Motivation for this change

Fix #18729

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
    • OS X
    • 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

@veprbl, thanks for your PR! By analyzing the history of the files in this pull request, we identified @cstrahan, @vbgl and @FRidh to be potential reviewers.

@domenkozar domenkozar added the 6.topic: python Python is a high-level, general-purpose programming language. label Oct 15, 2016
@veprbl veprbl changed the title Distutils fix python: add C++ compiler support for distutils Oct 15, 2016
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

We should of course try to fix the situation on Darwin, while at the same time stick as much as possible to upstream.

It's not yet clear to me whether this situation is being dealt with upstream. Furthermore, if we fix an issue, we should fix it for all versions of the interpreter that are affected.

Copy link
Member

Choose a reason for hiding this comment

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

Is this patch accepted upstream? How does this affect non-Darwin? Does this fix anything on non-Darwin?

Copy link
Member

Choose a reason for hiding this comment

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

and how about other Python versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

You can see in the original issue http://bugs.python.org/issue1222585 that distutils were considered frozen at the time, so this has became a won't fix. This Arfrever person was maintaining it outside of the source tree. The patch is supposed to fix things that get compiled with non-gcc on any platform, but the main target is of course Darwin.

There are also patches presented for python 3.3 and 3.4, it doesn't look like it made to python 3.5. I personally don't consciously use any python3 software and I don't know if they use distutils or if they use C++ code at all.

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 see in the original issue http://bugs.python.org/issue1222585 that distutils were considered frozen at the time, so this has became a won't fix. This Arfrever person was maintaining it outside of the source tree. The patch is supposed to fix things that get compiled with non-gcc on any platform, but the main target is of course Darwin.

If its not accepted upstream, then I think we should only use it when it is necessary. If I understand correctly, it is necessary on Darwin, so I suggest using an optional stdenv.isDarwin for appending this patch and the others as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about optional !(stdenv.cc.isGCC or false)?

Copy link
Member

Choose a reason for hiding this comment

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

That's fine as well.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the "or false" part. Wouldn't !(stdenv.cc.isGCC) yield the same result?

Copy link
Member Author

@veprbl veprbl Oct 17, 2016

Choose a reason for hiding this comment

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

or false is a special syntax for the case when isGNU is not defined to prevent evaluation from failing

Copy link
Member

Choose a reason for hiding this comment

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

has this patch/issue been reported upstream (numpy)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I made this patch myself. I don't see the point of reporting the issue before the main distutils patch is merged to python2, which won't happen.

Copy link
Member

@FRidh FRidh Oct 26, 2016

Choose a reason for hiding this comment

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

This patch, which has landed on master, breaks building scipy on Linux.

Copy link
Member

Choose a reason for hiding this comment

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

it should probably be lib.optionals (isPy27 && !(stdenv.cc.isGNU or false))

Copy link
Member

Choose a reason for hiding this comment

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

has this patch/issue been reported upstream (cython)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

thanks

@veprbl veprbl force-pushed the distutils_fix branch 2 times, most recently from f6a5ce3 to e653490 Compare October 17, 2016 07:55
+ 'SO', 'AR', 'ARFLAGS')
+
+ cflags = ''
+ cxxflags = ''
Copy link
Member Author

@veprbl veprbl Oct 17, 2016

Choose a reason for hiding this comment

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

I don't understand this part very well. The hunk of the original patch that I removed, namely:

--- Makefile.pre.in
+++ Makefile.pre.in
@@ -455,7 +455,7 @@
        *\ -s*|s*) quiet="-q";; \
        *) quiet="";; \
    esac; \
-   $(RUNSHARED) CC='$(CC)' LDSHARED='$(BLDSHARED)' OPT='$(OPT)' \
+   $(RUNSHARED) CC='$(CC)' LDSHARED='$(BLDSHARED)' CFLAGS='$(CFLAGS)' \
        $(PYTHON_FOR_BUILD) $(srcdir)/setup.py $$quiet build

 # Build the platform-specific modules

seems to set CFLAGS in these config vars, however this chunk seems to make it so that the CFLAGS value from the makefile is ignored. So this looks like an inconsistency in original patch.

Perhaps a gentoo bug https://bugs.gentoo.org/show_bug.cgi?id=548776 needs to be tried out.

@FRidh FRidh added the 6.topic: darwin Running or building packages on Darwin label Oct 25, 2016
@FRidh FRidh merged commit fea2302 into NixOS:staging Oct 25, 2016
@veprbl
Copy link
Member Author

veprbl commented Oct 26, 2016

Yes, because this patch depends on the other one

26 окт. 2016 г., в 10:45, Frederik Rietdijk notifications@github.com написал(а):

@FRidh commented on this pull request.

In pkgs/development/python-modules/numpy.nix:

@@ -12,6 +12,12 @@ in buildPythonPackage (args // rec {
buildInputs = args.buildInputs or [ gfortran nose ];
propagatedBuildInputs = args.propagatedBuildInputs or [ passthru.blas ];

  • patches = lib.optionals isPy27 [
  • See cpython 2.7 patches.

  • numpy.distutils is used by cython during it's check phase

  • ./numpy-distutils-C++.patch
    it should probably be lib.optionals (isPy27 && !(stdenv.cc.isGNU or false))


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

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

Labels

6.topic: darwin Running or building packages on Darwin 6.topic: python Python is a high-level, general-purpose programming language.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants