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

cask/quarantine: avoid xcrun when executing Swift #16796

Merged
merged 1 commit into from
Mar 4, 2024
Merged

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Mar 2, 2024

Fixes things breaking when people put broken stuff into /usr/local/include:

We were using /usr/bin/swift which is the same as /usr/bin/xcrun swift. This has a weird documented behaviour:

ENVIRONMENT
       CPATH
          This environment variable is modified by xcrun to include
          /usr/local/include when an explicit SDK is not requested via
          environment variable nor command line argument and neither -nostdinc
          nor -nostdsysteminc are present.

We can avoid this by using xcrun -find to find the underlying Swift executable and invoking that directly.

Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. Thanks for tracking this down. Do you have any idea why they added this unusual behavior to xcurn?

Comment on lines 20 to 32
def self.swift
@swift ||= DevelopmentTools.locate("swift")
@swift ||= begin
# /usr/bin/swift (which runs via xcrun) adds `/usr/local/include` to the top of the include path,
# which allows really broken local setups to break our Swift usage here. Using the underlying
# Swift executable directly however (returned by `xcrun -find`) avoids this CPATH mess.
xcrun_swift = ::Utils.popen_read("/usr/bin/xcrun", "-find", "swift", err: :close).chomp
if $CHILD_STATUS.success? && File.executable?(xcrun_swift)
xcrun_swift
else
DevelopmentTools.locate("swift")
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks the old method returns a Pathname here but all the usages seem to just get coerced to strings so it's probably fine. On that note, it might be good to add types to this method.

Before:

brew(main):001:0> Cask::Quarantine.send(:swift)
=> #<Pathname:/usr/bin/swift>

After:

brew(main):001:0> Cask::Quarantine.send(:swift)
=> "/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/swift"

Copy link
Member Author

@Bo98 Bo98 Mar 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah maybe will keep it as a string because it's very slightly cheaper and is a private_class_method rather than API.

@Bo98
Copy link
Member Author

Bo98 commented Mar 4, 2024

Do you have any idea why they added this unusual behavior to xcurn?

My guess would be the transition from /usr/include to SDKs. Before it was very simple in that we had /usr/include and /usr/local/include. SDKs uses the sysroot system so that all these paths were prefixed with an SDK now, e.g. <sdk>/usr/include and <sdk>/usr/local/include. For compatiblity reasons it could make sense they made it so /usr/bin/clang worked like it did before with /usr/local/include.

That's just a theory though.

In any case, it hasn't scaled well with modules being introduced and generally with Swift. It makes some sense when working with Clang etc directly, but does raise a point that maybe this is something we want to avoid in the superenv, particularly on /opt/homebrew installs.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks again @Bo98!

@MikeMcQuaid MikeMcQuaid merged commit a1d1ee4 into master Mar 4, 2024
26 checks passed
@MikeMcQuaid MikeMcQuaid deleted the swift-direct branch March 4, 2024 09:36
@github-actions github-actions bot added the outdated PR was locked due to age label Apr 3, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants