-
Notifications
You must be signed in to change notification settings - Fork 374
gcc-4.8: Fix Linux portability problems #279
Conversation
Should this also run on OS X? If not, add an if statement to the script. |
What is the best way to test this? |
After some testing on Mac OS 10.10, apparently not. (I'll update the patch.) The only "fixed" header is stdint.h, and it doesn't seem to cause compilation errors on the programs I've tried so far.
Well, ideally, we'd just try compiling a big stack (all of the default Anaconda packages?) on multiple Linux distributions. If nothing seems to break, I guess we're good to go. |
718d65f
to
f265b21
Compare
Done. BTW, I was wrong about this Mac issue I mentioned above:
The Mac I borrowed for this testing had XCode installed, but didn't have the "xcode command-line developer tools". Running I suppose it would be possible to help boneheads like me by running a post-install script that verifies the command-line tools are installed, but it's probably not necessary -- most devs have them installed already. Also, verifying that they are installed is not simple. |
It was not my intention for gcc to depend on the developer tools. Requiring them (almost) defeats the purpose. What error did you get? According to |
Just to clarify, I'm not reporting a problem with using the The problem I ran into is that gcc doesn't know where to find the libc headers (e.g.
gcc doesn't know about this directory. But if you install the "Xcode command-line tools", then all the standard headers are copied into
(Actually, the headers were only part of the problem. If I provided the headers via I don't think it's unreasonable to expect users who want to compile their own packages to have XCode and the command-line tools already installed. |
It would be nice to avoid it if possible, since installing XCode requires root permissions, but I don't plan to dedicate a ton of effort to work around it myself. |
I was hoping for something a little more directed, i.e., something that is broken and easily reproducible with the current package. |
As I mentioned before, this PR works on Fedora 20. But when I tested on Ubuntu 12.04, I discovered yet more portability problems:
Of course, one workaround is to pass explicit
... but then we hit the next problem:
This, too, can be worked around, this time with the
Instead of requiring users to pass these flags to gcc every time they use it, we can augment the gcc binary ass follows:
I've just added a |
@@ -0,0 +1,48 @@ | |||
#!/bin/sh | |||
|
|||
if [ "$(uname)" != "Darwin" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, conda-build really ought to have a way to not even include the file on OS X.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asmeurer This file is not only for OS X though. However, since you said that post-list.sh shouldn't be included on OS X. Could you please say why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is any of this process needed on OS X?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asmeurer The part below fi
statement is kind of needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it needed? As far as I can tell, all it does is test the compiler. I haven't heard of any issues with the existing gcc package on OS X.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire PR is really for Linux. The things that this PR fixes ("fixed" std headers, inconsistent std header locations, and inconsistent locations for crt0.o
, etc.) aren't issues on Mac. If OSX had multiple "distributions" like Linux, that would be a different story. It looks like we can just skip all this stuff on Mac.
The part below fi statement is kind of needed.
The part below the last fi
merely verifies that "Hello World" compiles. Generally, it should always pass -- unless the user didn't install the xcode command-line tools on her machine. (Otherwise gcc can't find the standard library headers.)
If you have to have a post-link, fine. Just make sure it is very reliable (if it fails, the install will be broken), and very portable. |
Regarding testing:
Since this issue is about fixing portability issues, the only way to demonstrate the problem(s) is to try using the package on different OSes. Just to reiterate the above discussion, there are three known portability issues addressed in this PR:
Ubuntu 12.04 appears to be affected by all three issues. To demonstrate issues 2 and 3, try compiling this C program on Ubuntu 12.04 using conda's gcc binary:
To demonstrate issue 1, try compiling this C++ program on Ubuntu 12.04:
I don't know which other Linux distributions are affected by these issues, but I happen to know that Fedora 20 is affected by issue 1, and @jakirkham's issue referenced above suggests that CentOS 6.3 is also affected. |
BTW, my binstar channel has a pre-packaged version of these changes, which you can try out on the VM of your choice:
(Disclosure: I didn't actually build it myself. I just copied your gcc package, removed the |
SYSTEM_GCC=/usr/bin/gcc | ||
|
||
DUMMY_PROGRAM="int main(){return 0;}" | ||
GCC_CMDS=`echo "$DUMMY_PROGRAM" | $SYSTEM_GCC -v -xc - 2>&1` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be a simpler way for me to get the locations of crt1.o
, etc. I'll revise this patch on Monday.
@stuarteberg So, I tried your binary ( https://binstar.org/stuarteberg/gcc-no-fixincludes ) and it did indeed resolve my build issues regarding NumPy on CentOS 6.3. It also worked on SciPy. |
0ab4098
to
ad5586c
Compare
I've added a test step in two places: It's worth noting that the addition of this new test in In other news, I've made some minor simplifications to |
You should bump the build number in the meta.yaml |
@stuarteberg What OS's you tested these pre and post link steps? |
b804b0c
to
3b215ff
Compare
Fedora 20, Ubuntu 12.04, ScientificLinux 6.3, and Mac OS 10.10 (for which I pushed some minor compatibility fixes just now).
OK, done. I bumped it from 1 to 3 because somehow the version in conda already has a build number of 2. |
Apparently I didn't test thoroughly enough. The |
de94399
to
6cab2a0
Compare
This thread was getting long, so I updated the description with a fresh summary of the changes here. See above. |
Sigh. I'm not sure. I guess the best thing would be to check the system gcc on the 32-bit VM. If both directories are listed, then we should use the same order it uses. This command will show the header search directories (among other things):
I don't happen to have a 32-bit Ubuntu VM handy, so I can't test this myself. |
This is what I did. |
That should be just fine. Have you tested it yet? FWIW, I just spun up a new Ubuntu 32-bit VM just to check the include path order Only |
It seems to install and remove fine. I haven't tested it beyond that, though. The builds are on my binstar. |
@asmeurer Can we merge it then? Or are we waiting for something else? |
He already merged it into conda-recipes master. The last step would be to add it to the default conda package repo, so it can be installed via |
Yes, sorry, that's what I meant. @asmeurer is this something we can do? |
I've also put it in on the |
@asmeurer Is this something that we can get added to the default conda repo? Or is that unlikely to happen for whatever reason? |
It will happen. I just want it to get tested a little bit before doing so. |
Great, thanks! Question: Is it worth making a gcc-libraries recipe that simply grabs the important libraries (libstdc++, ...) from gcc? This way other recipes can depend on the gcc libraries without needing all of GCC. I've done this for DyND and it works nicely. |
@izaid, I believe this is already done. ( https://github.com/conda/conda-recipes/blob/master/libgcc/meta.yaml ) |
@jakirkham Ah, okay, good. But, don't we need another one for the gcc-4.8 related to this PR? |
I have a PR for bumping the version to be consistent with the version of |
Also, @izaid, there is a copy of |
oh I forgot about libgcc. I should rebuild that as well. |
Hello, It may be usefull to create a symbolic link For C++, it seems ok ( |
That sounds like a good idea, although I'm wondering if that would break some of my OS X recipes that are currently working with clang. |
Already done: @mjuric already did that in 510897e, which was merged into this PR before it was accepted by @asmeurer. @dfroger, I'm guessing you are using
If you don't include the |
I guess I am primarily thinking of R but that is managed via https://github.com/conda/conda-recipes/blob/7e78b47686c193fb81973d33b3797ed259eaccdf/r-base/build.sh#L61. |
@stuarteberg exactly, I was using |
Yes, it's best for all recipes to be explicit about which compiler they're using, especially if they involve C++ code. That means explicitly setting the For my own project, the above guidelines are spelled out here: https://github.com/ilastik/ilastik-build-conda#appendix-compiler-details |
Thanks for the guidelines. By the way, I'm using this Vagrant box: https://github.com/dfroger/conda-build-env |
Edit: This thread is getting long, so I'll try to keep an up-to-date summary at the top here.
<tl;dr>Without this patch, the
gcc
package is worthless on nearly all Linux distros. But with this patch, it works on all non-ancient Linux distros.</tl;dr>Conda's
gcc
package was built on CentOS 5.11, and only works on that distro. However, we can make it work on more distros by adding apost-link.sh
file.In this PR,
post-link.sh
does three important things:-I
flag to the default gcc flags (via aspecs
file).crt1.o
) by creating some symlinks to them.Also,
post-link.sh
has the following features:I. If you are installing onto OSX, do nothing (
post-link.sh
is a no-op).II. If you are installing onto the same OS that was used to create the
gcc
package, do nothing. (Here, the definition of "same OS" means: compare the output ofcat /etc/*-release
at build-time and install-time.)III. In interest of "failing early" if you're trying this
gcc
package on a system that just can't use it, thepost-link.sh
script compiles a "Hello World" program at install time. If something is broken, runningconda install gcc
will simply fail.Without this PR, the
gcc
package is basically non-portable across Linux distros.It's true: even with this PR, we still can't support every Linux distro under the sun, but at least now we can support a lot more. And thanks to item
II
above, you can always just build thegcc
recipe yourself (in which casepost-link.sh
becomes a no-op). This means the Anaconda build server won't be affected by these changes.(Sadly, if your Linux distro is so old that it has "bad" libc headers, even this PR can't help you. In that case your only option is to build gcc yourself, or obtain it through other means.)
Good news! You can try this PR on own system without needing to build gcc yourself:
conda install -c stuarteberg gcc=4.8.4.99
conda install -c asmeurer gcc=4.8.5
That package is known to work on the following distros:
This PR attempts to fix the issue described on the mailing list here:
https://groups.google.com/a/continuum.io/d/msg/conda/HwUazgD-hJ0/aofO0vD-MhcJ
To summarize:
Update: As described below, now this PR also address two other portability issues: locating system headers and locating system libc code on various distros.