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

macOS: Conditionally add -Wl,-ld_classic for Xcode CLT >= 15 #972

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

DilumAluthge
Copy link
Member

@DilumAluthge DilumAluthge commented Oct 16, 2024

Replaces #935 (closes #935)
Reference #738


This is based on #935. All credit goes to @PhilReinhold for providing the fix and @jayscook for opening the original PR (#935).

This PR differs slightly from #935. #935 used a try-catch. This PR uses ignorestatus() to ignore non-zero exit codes from running the pkgutil ... command, and uses isnothing() to handle the case where the regex does not match. Also, this PR factors out the "detect CLT major version" functionality out into a separate helper function.

Also, this PR now addresses #935 (comment)

We do not add these flags for CLT versions <= 14.

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Oct 16, 2024

This PR doesn't yet address #935 (comment), but I'll look into it further.

Okay, #935 (comment) is now addressed by the combination of c9a4b52 and adf5737.

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 48.64865% with 19 lines in your changes missing coverage. Please review.

Project coverage is 83.00%. Comparing base (85cdb03) to head (90931fe).

Files with missing lines Patch % Lines
src/xcode.jl 43.75% 18 Missing ⚠️
src/PackageCompiler.jl 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #972      +/-   ##
==========================================
- Coverage   84.56%   83.00%   -1.57%     
==========================================
  Files           3        4       +1     
  Lines         823      859      +36     
==========================================
+ Hits          696      713      +17     
- Misses        127      146      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DilumAluthge
Copy link
Member Author

Okay, #935 (comment) is now addressed by the combination of c9a4b52 and adf5737.

@DilumAluthge
Copy link
Member Author

Specifically, we now run $cc --version, and we use that output to determine whether or not the compiler is the Clang from Xcode.

@DilumAluthge
Copy link
Member Author

It's not entirely clear to me if we still need this PR, because #930 seems to have fixed #738, at least in the macOS CI on this repo.

src/xcode.jl Outdated Show resolved Hide resolved
src/xcode.jl Outdated Show resolved Hide resolved
src/xcode.jl Outdated Show resolved Hide resolved
@jayscook
Copy link

Thanks for putting this together @DilumAluthge!

@DilumAluthge
Copy link
Member Author

@jayscook Thank you for putting the original PR together (and thank you to @PhilReinhold for writing the original code).

@DilumAluthge
Copy link
Member Author

@topolarity Could you take a look at this PR, and let me know if it addresses your concerns in #935 (comment) and #935 (comment)?

@topolarity
Copy link
Member

Sure thing! Looks like the right direction, but I won't be able to take a close look until Monday

@DilumAluthge
Copy link
Member Author

Sounds good, thanks man!

@DilumAluthge
Copy link
Member Author

@sjkelly I see you also reviewed the original PR (#935), so I've requested your review on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants