Skip to content
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

OS X: do not use -ld_classic #38931

Merged
merged 2 commits into from
Nov 16, 2024
Merged

Conversation

jhpalmieri
Copy link
Member

@jhpalmieri jhpalmieri commented Nov 5, 2024

Recent versions of Xcode have deprecated -ld_classic, so (a) we stop using it, (b) we filter warnings about it when doctesting, and (c) we filter some other warnings related to ld in Xcode.

Without these changes, when building with the latest Xcode, I get many doctest failures because of the new warning

ld: warning: -ld_classic is deprecated and will be removed in a future release

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • 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

Copy link

github-actions bot commented Nov 5, 2024

Documentation preview for this PR (built with commit e79ccb9; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@culler
Copy link
Contributor

culler commented Nov 5, 2024

@jhpalmieri: The code which this PR removes was supposed to check whether the old linker is present in the toolchain and not add the -ld_classic option if the old linker is not present. This would use the old linker in older versions of XCode but would not use the flag in newer versions of XCode which do not have the old linker. It tested the existence by just checking if ld accepts that option. I don't see why that test should be removed in favor of never requesting the old linker, even in old versions of XCode where it was buggy.

Since you have a new XCode, you should not have been using the option. Somehow the test failed. I am wondering if you might have some mixed up setup, such as a new XCode and an old command line tools installation, which made the test fail.

@jhpalmieri
Copy link
Member Author

Since you have a new XCode, you should not have been using the option. Somehow the test failed. I am wondering if you might have some mixed up setup, such as a new XCode and an old command line tools installation, which made the test fail.

I'm confused: I think that the newer versions of Xcode are precisely the ones that contain an ld-classic executable. For example I have Xcode 16.1 and a file /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ld-classic which is detected by that code in sage-env. Therefore the -ld_classic flag is added to LDFLAGS, and that leads to the many deprecation warnings.

So I think the question is, are there versions of Xcode that are recent enough to be distributed with an ld-classic executable and are old enough that Sage requires us to use the -ld_classic flag to actually build?

@culler
Copy link
Contributor

culler commented Nov 6, 2024

Sorry. I did not understand the situation -- I thought that the ld_classic linker had already been removed in XCode 16.1. What actually did happen is that the -ld_classic option was deprecated in XCode 16.0, according to the release notes.

I don't think there is any need to use the old linker with XCode 16. I think the bugs have been fixed by now. But if you are using XCode 15 then you probably do want to use ld_classic. So maybe the correct thing to do is to make the test be more sophisticated. Maybe it should run ld with the -ld_classic flag and check if a deprecation warning is issued.

@jhpalmieri
Copy link
Member Author

This seems to work: "ld -ld_classic" prints a deprecation warning with the new Xcode, presumably not with Xcode 15.

@culler
Copy link
Contributor

culler commented Nov 6, 2024

This looks good to me.

I extracted the block of code that adds the ld_classic flag (if needed) as a separate script which I then ran on three systems: one with the XCode 14 command line tools, one with the XCode 15 command line tools and one with XCode 16.1. Only the XCode 15 system got the extra flag.

@jhpalmieri
Copy link
Member Author

This looks good to me.

I extracted the block of code that adds the ld_classic flag (if needed) as a separate script which I then ran on three systems: one with the XCode 14 command line tools, one with the XCode 15 command line tools and one with XCode 16.1. Only the XCode 15 system got the extra flag.

Great, thank you for checking that! I don't have access to machines running anything except Xcode 16.

vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 7, 2024
    
Recent versions of Xcode have deprecated `-ld_classic`, so (a) we stop
using it, (b) we filter warnings about it when doctesting, and (c) we
filter some other warnings related to ld in Xcode.

Without these changes, when building with the latest Xcode, I get many
doctest failures because of the new warning
```
ld: warning: -ld_classic is deprecated and will be removed in a future
release
```

### 📝 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.
- [ ] 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#38931
Reported by: John H. Palmieri
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 8, 2024
    
Recent versions of Xcode have deprecated `-ld_classic`, so (a) we stop
using it, (b) we filter warnings about it when doctesting, and (c) we
filter some other warnings related to ld in Xcode.

Without these changes, when building with the latest Xcode, I get many
doctest failures because of the new warning
```
ld: warning: -ld_classic is deprecated and will be removed in a future
release
```

### 📝 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.
- [ ] 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#38931
Reported by: John H. Palmieri
Reviewer(s): Marc Culler
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 9, 2024
    
Recent versions of Xcode have deprecated `-ld_classic`, so (a) we stop
using it, (b) we filter warnings about it when doctesting, and (c) we
filter some other warnings related to ld in Xcode.

Without these changes, when building with the latest Xcode, I get many
doctest failures because of the new warning
```
ld: warning: -ld_classic is deprecated and will be removed in a future
release
```

### 📝 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.
- [ ] 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#38931
Reported by: John H. Palmieri
Reviewer(s): Marc Culler
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 13, 2024
    
Recent versions of Xcode have deprecated `-ld_classic`, so (a) we stop
using it, (b) we filter warnings about it when doctesting, and (c) we
filter some other warnings related to ld in Xcode.

Without these changes, when building with the latest Xcode, I get many
doctest failures because of the new warning
```
ld: warning: -ld_classic is deprecated and will be removed in a future
release
```

### 📝 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.
- [ ] 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#38931
Reported by: John H. Palmieri
Reviewer(s): Marc Culler
@vbraun vbraun merged commit 0df1f22 into sagemath:develop Nov 16, 2024
19 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants