the ECM patch breaks some macOS installations #39040
Conversation
|
Documentation preview for this PR (built with commit 7ce1877; changes) is ready! 🎉 |
So the patch is for building sage with gcc 14 (installed by homebrew) on macOS, but apparently it conflicts with building sage with system clang on macOS with an intel chip. If we drop it, we should warn users not to attempt the configuration (homebrew gcc 14 + macOS with an intel chip). Does this configuration imply that homebrew gcc overrides xcode clang? If so, that is really unusual. |
|
@culler Do you agree? |
|
My inclination would be to try to understand what is going on before
deciding what to do about it.
The patch Is very tiny. It only involves choosing whether a handful of
autogenerated assembly language files should have extension .s, .S or .sx.
Here
<https://community.nxp.com/t5/CodeWarrior-Development-Tools/What-is-the-difference-between-s-and-sx-assembly-file/m-p/1412545>
is a clear explanation of the situation with these filename extensions. It
is clear to me that ECM was making an error. They were using extension .s
for autogenerated assembly language files containing preprocessor
directives. The comments indicated that they were choosing to do that on
macOS because the filesystem is case-insensitive, but that is not the issue
and what they did only worked for the wrong reasons. I think that gcc has
been consistent about using .S or .sx for assembly language files which
need preprocessing, but I think that clang has been inconsistent and the
ECM was relying on clang's quirky behavior. I think that clang's behavior
became consistent with gcc in XCode 16.0 or 16.1, and that is the real
cause of this problem.
To know for sure, one would have to do some testing with various XCode
versions. Once that was done, correcting the patch should be fairly easy
to do.
- Marc
…On Tue, Nov 26, 2024 at 8:05 PM Kwankyu Lee ***@***.***> wrote:
As it was added in order to support building ECM with gcc 14, which is a
rather hypothetical situation, it's OK to drop it.
So the patch is for building sage with gcc 14 (installed by homebrew) on
macOS, and it conflicts with building sage with system clang on macOS with
an intel chip.
If we drop it, we should warn users not to attempt the configuration
(homebrew gcc 14 + macOS with an intel chip). Does this configuration imply
that homebrew gcc overrides xcode clang? If so, that is really unusual.
—
Reply to this email directly, view it on GitHub
<#39040 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ6CPZIVUF6B4KQK6XRLML2CUSHHAVCNFSM6AAAAABSQZQTXGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMBSGUYDAMZUGI>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Since when is it even possible (again) to build Sage with "real" gcc on macOS? This has been broken for years. Do you know any user who does this? |
|
We are in release candidate stage. I think there is not enough time left to investigate the issue and prepare a proper fix that works for all platforms. Let's get this in. |
|
Possibly it is too late. But I am not convinced that we know which
situations fail with the patch and which fail without it. My current
impression is that if you use the patch then the build will fail when Sage
is built with XCode versions less than 16.0 and if you do not use the
patch then the build will fail if Sage is built with XCode versions 16.0
and up. (It only affects Intel because the assembly language files are
specific to Intel CPUs.) Even though this issue first appeared in the
course of changing the gcc package, I am not sure that it is related to gcc.
I think we know for a fact that the build can fail if the patch is used
with an older Intel XCode compiler, and I know for a fact that I have built
ECM successfully with the patch installed using the latest Intel XCode
compiler. But do you know for sure that the build succeeds with a new
Intel XCode compiler if the patch is removed? I don't.
- Marc
…On Tue, Nov 26, 2024 at 8:54 PM Kwankyu Lee ***@***.***> wrote:
We are in release candidate stage. I think there is not enough time left
to investigate the issue and prepare a proper fix that works for all
platforms.
Let's get this in.
—
Reply to this email directly, view it on GitHub
<#39040 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ6CP2IZQKVZEGEMULMHU32CUX67AVCNFSM6AAAAABSQZQTXGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMBSGU4DAMZWGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Marc, it's not possible to build Sage on macOS with "real" gcc. Maybe it is possible to build parts of Sage with "real" gcc and other parts with clang, but we don't support this. |
|
If the patch in question was needed for XCode's clang then putting it in the PR upgrading gcc was a rather inappropriate place. |
Marc, your #38905 claims that this patch isn't needed for XCode (namely, you wrote Besides, XCode 16.1 was released less that a month ago. We are not obliged to support it in the coming release, it's way too late. |
|
I don't think you read my message. I don't think this has anything to do
with gcc, and I was definitely not using gcc when I successfully built ECM
with the patch installed. I think Apple clang changed how they interpret
the file extensions. I could be wrong. I hope so. But I think that with
the patch ECM will build with newer XCode versions and without the patch
ECM will build with older XCode versions. Please prove me wrong. I would
be much happier being wrong about that.
- Marc
…On Tue, Nov 26, 2024 at 9:31 PM Dima Pasechnik ***@***.***> wrote:
Marc, it's not possible to build Sage on macOS with "real" gcc. Maybe it
is possible to build parts of Sage with "real" gcc and other parts with
clang, but we don't support this.
So I don't know what this patch is about in the 1st place. Let's not try
to solve nonexisting in Sage problems here.
—
Reply to this email directly, view it on GitHub
<#39040 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ6CP5KOHNJNHPGDUM5OEL2CU4JPAVCNFSM6AAAAABSQZQTXGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMBSGYZTAOBWGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Well, you put a patch which has nothing to do with gcc (as you say now) to a PR with the title Update the gcc spkg to version 14.2.0 using iains/gcc-14-branch. There is nothing about the ECM patch in the PR description. The PR description talks at length about "real" gcc for Darwin. All this leads one to believe that the patch is needed for gcc (C compiler!) support for Darwin. However, as we don't use gcc's C compiler on Darwin, it appears to make no sense. By right, there should have been a totally separate PR dealing with the ECM issue. |
My mac has Intel Xeon E5 processor, and has Building ecm fails with the patch but succeeds without the patch. With the patch, I get My machine is an old mac pro, but I managed to install the latest macOS. So my system does not have an official configuration. |
is the compiler Apple clang version 16.0.0? |
|
is it xcode 16.0? 16.1? one way or another, an alternative fix would be, on macOS, to try first to build without the patch, and if it fails, build with the patch. |
|
I also have a macbook pro with an intel chip and official macOS, with which I failed to build ecm with the patch but removing the patch, it was a breeze to build ecm. |
On my mac pro, it is xcode 16.1. |
The machine has Intel Core i9 chip, and xcode 16.1 |
There was a problem hiding this comment.
As far as I understand, either keeping or removing the patch is not a complete solution. I think we may remove the patch now to cover the reported cases. Later if we get failure reports from the other side, then we may conceive a better solution based on more data. So I approve the PR, but I leave the decision to Marc.
|
I am OK with removing the patch. If it turns out that some people need it and some don't, that means that the implementation is wrong and it should be redone, adding code which decides when the Makefile.in should be changed. For the record, I would like to post the comment from the ECM Makefile which is clearly wrong: The statement that "Nothing here needs the C preprocessor" is being applied to files which contain preprocessor directives and therefore do need the C preprocessor. The mystery is why the C preprocessor sometimes gets used in cases (namely when the file has a .s extension) where the gcc rules say it should not be used. Of course the compiler being used is clang, not gcc, but clang claims to be providing a gcc emulator. I expect that this issue will arise again. But at least we now know something (not much, but something) about it and can keep an eye out for it. Maybe in the meantime ECM will address it. |
|
I remembered something which might explain the mysterious changes in XCode behavior. When I encountered my failure building ECM, which the patch fixed, I was using XCode 16.1beta. I am guessing that Apple decided that making clang consistent with gcc in this way was causing more problems than it was solving, so they reverted that change. |
|
I did some testing with XCode which convinced me that Dima is correct - this patch was created to allow building ECM with gcc 14.2.0 and was based on my false assumption that XCode's gcc emulator followed the gcc convention regarding assembly language file extensions. This is what my testing shows:
I tested by writing a "hello world" in C; compiling it to assembly language with gcc -S; So, given that nobody can and nobody should try building Sage with gcc on macOS, I am now convinced that removing the patch is the correct thing to do. My apologies for assuming that a gcc emulator would emulate gcc without checking if that were true. I also apologize for forgetting the exact chain of events which, fortunately, I had written down in the PR where Dima could read them. |
|
Marc, thanks - would you propose your patch to the ECM upstream? |
The ECM patch added in sagemath#38905 breaks some macOS installations. As it was added in order to support building ECM with gcc 14, which is a rather hypothetical situation, it's OK to drop it. This will fix sagemath#39039 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#39040 Reported by: Dima Pasechnik Reviewer(s): Kwankyu Lee
The ECM patch added in #38905 breaks some macOS installations.
As it was added in order to support building ECM with gcc 14, which is a rather hypothetical situation, it's OK to drop it.
This will fix #39039
📝 Checklist
⌛ Dependencies