-
Notifications
You must be signed in to change notification settings - Fork 2.4k
gcc: fix darwin dispatch/object.h header issue #1518
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.
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
Conversation
|
Haven't tested it yet, but will tomorrow (gcc is a long build, and so is cmake). |
c10d2ac to
d3b3c33
Compare
|
Fixed regex error in transcribing from sed (POSIX regex) to Python regex. gcc is currently building now, will check the header when it's done (probably around when I wake up), and try installing cmake with gcc. |
d3b3c33 to
61d53ec
Compare
|
Added flake8 fixes. |
| # is GCC incompatible by replacing non-GCC compliant macros | ||
| if sys.platform == 'darwin': | ||
| if isfile("/usr/include/dispatch/object.h"): | ||
| new_dispatch_dir = prefix + "include/dispatch" |
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.
Does this need a slash after prefix?
61d53ec to
f66172f
Compare
|
@adamjstewart Pushed some of those fixes, still need to test again. |
f66172f to
cf7f298
Compare
|
Made the code a little more debuggable. The good news: the section of code I added does what it's supposed to, which is to copy the gcc-incompatible header to the correct location and perform the right replacement. The bad news: I couldn't finish the build before I leave work, because building gcc with |
|
After several false starts and 2-3h compiles of gcc, the good news is: gcc compiles and the patched header is copied in the right place, so this patch "works". The bad news is: If the fix from hashdist/hashstack#771 indeed fixes the issue for HashDist, then maybe switching the order of compiler flags will force gcc to pick up the patched version of |
|
@goxberry I think your best try is the compiler wrapper : |
|
@alalazo Thanks! Looking in those places helped, and gave me ideas on testing. |
|
@tgamblin Narrowed the issue down to the following: the compiler still doesn't seem to pick up the patched gcc header before the non-compliant system header. So far I've tried:
In all cases, I've encountered the same error. I don't know what else to try at this point; I can't replicate the behavior from hashdist/hashstack#771, so at this point, I need to reach out to Ondrej or Aron and ask them what is actually happening with that patch. |
|
@tgamblin Okay, something worked: |
|
@tgamblin @davydden @alalazo Figured out the issue. The patch has been tested and it works; the problem I encountered was that spack won't distinguish between versions of compilers it builds and system compilers. If I build gcc 6.1.0 with spack and gcc 6.1.0 is installed on my machine, then Is there any way to distinguish between spack-installed and system-installed compiler versions? Maybe add a |
[Why me? :-] OK, my opinion... I think this is a non-issue. I never use Spack's On Tue, Sep 20, 2016 at 8:50 PM, Geoffrey Oxberry notifications@github.com
|
|
I regularly use a Spack-installed compiler. I configure Spack manually to do this, appending |
@citibeth You're among the most active commenters on the spack repo and also take the lead on a lot of the documentation issues. If the response is mainly, "a user should just manually configure spack to use the installed compiler in case of version collision," then I believe this advice should be added to the documentation. |
|
I bring up autoconfiguring Spack partly because it would be convenient -- a Spack-installed compiler should be known to Spack -- and partly because debugging this issue was difficult. The incorrect assumption on my part was that installing gcc 6 via Spack meant that when I typed |
|
Clearly, the right automagic is to modify On Tue, Sep 20, 2016 at 10:36 PM, Geoffrey Oxberry <notifications@github.com
|
|
|
||
| # Fix a standard header file for OS X Yosemite & earlier that | ||
| # is GCC incompatible by replacing non-GCC compliant macros | ||
| if sys.platform == 'darwin': |
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.
you write Yosemite and earlier, why don't you check for that (looking at versions)? I am not sure this patch will work on elcapitan or sierra. Generally what Apple ships with next versions is unknown. So i would adjust the if so that it is in accordance with the comment.
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.
see #1814
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.
you write Yosemite and earlier, why don't you check for that (looking at versions)?
@davydden The API for OS versions isn't documented at all, AFAICT. I had some downtime so I spent a couple hours looking at the Spack code and couldn't make much heads or tails of it, so I only added a check for Yosemite. As I'm sure you're aware, the bare string comparison of OS versions will fail (e.g., spec.architecture.platform_os.version < 10.10 is False when the OS version is 10.9), so as you note in #1814, the OS version should really be a Version object so that it can use the comparison operators inherited from that class.
I am not sure this patch will work on elcapitan or sierra.
Also, AFAICT from Google searches, the string replacement doesn't affect other versions of Mac OS prior to Sierra, excluding Yosemite, of course. That said, I haven't tested this code against any versions of OS X other than Yosemite. If someone has access to OS X systems running Sierra/El Capitan/Mavericks/etc. and would be willing to test this patch, I'd appreciate it.
So i would adjust the if so that it is in accordance with the comment.
I have made a change that I think complies with your request.
Fixes spack#1203. Apple ships headers in Yosemite (and possibly earlier) that are gcc-incompatible, but compile fine with clang. The fix is to copy the offending system header from /usr/include/${REST_OF_HEADER_PATH} to ${GCC_PREFIX}/include/${REST_OF_HEADER_PATH} and replace the non-gcc- compatible features with gcc-compatible equivalents. See https://github.com/hashdist/hashstack/pull/771/files for inspiration, and http://hamelot.io/programming/osx-gcc-dispatch_block_t-has-not-been-declared-invalid-typedef/ for a description of the header issue.
cf7f298 to
25c7213
Compare
davydden
left a comment
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.
the way you wright it now is even more readable than what i have thought. Thanks for changing the if.
@tgamblin ping.
|
There's probably one caveat, which is that if you're cross-compiling I like the design of |
|
I was wrong; this patch should work because the conditional will not be satisfied in a cross-compile. This patch is ready to merge, @tgamblin. |
|
@goxberry: I'm hoping to get specs documented eventually. They're currently kind of extensible, although I'd really like to get the constraint login in there to make it so the concretization code is essentially declarative. Allowing plugins or something for different fields on Specs would be a start to that but I think the more important thing at the moment is making the concretization better... it basically needs a better solver. Thanks for the contributions and sorry this took a while to merge! |
Fixes #1203. Apple ships headers in Yosemite (and possibly earlier) that
are gcc-incompatible, but compile fine with clang. The fix is to copy
the offending system header from /usr/include/${REST_OF_HEADER_PATH} to
${GCC_PREFIX}/include/${REST_OF_HEADER_PATH} and replace the non-gcc-
compatible features with gcc-compatible equivalents.
See https://github.com/hashdist/hashstack/pull/771/files for
inspiration, and
http://hamelot.io/programming/osx-gcc-dispatch_block_t-has-not-been-declared-invalid-typedef/
for a description of the header issue.