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.

Sign up for GitHub

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

Jump to bottom

[WIP] Use xcode-select -p (weak link approach) #172

Closed
wants to merge 4 commits into from
Closed

[WIP] Use xcode-select -p (weak link approach) #172

wants to merge 4 commits into from

Conversation

norio-nomura
Copy link
Collaborator

#167

Overview:

  • libclang.dylib and sourcekitd.framework are weakly linked to SourceKittenFramework.
  • toolchain pathes has been removed from LC_RPATH on SourceKittenFramework.
  • sourcekitten loads libclang.dylib and sourcekitd.framework from active developer directory by calling checkDepndenciesOfSourceKittenFramework() in main.swift
  • checkDepndenciesOfSourceKittenFramework() calls execv for re-launching main executable for loading libclang.dylib and sourcekitd.framework. So, it might not be used on some dependent client of SourceKittenFramework

checkDepndenciesOfSourceKittenFramework() does following:

  • Check whether libclang.dylib and sourcekitd.framework are loaded or not
  • If not, coordinates DYLD_* environment variables and calls execv
  • New sourcekitten process launched by execv loads libclang.dylib and sourcekitd.framework using coordinated DYLD_* environment.

DYLD_* environment variables are coordinated to searching them in following order:

  1. $TOOLCHAIN_DIR/usr/lib # for debugging on Xcode
  2. xcode-select -p /Toolchains/XcodeDefault.xctoolchain/usr/lib
  3. /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib
  4. /Applications/Xcode-beta.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib
  5. $HOME/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib
  6. $HOME/Applications/Xcode-beta.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib

3~6 are fallbacks on xcode-select -p pointing Command Line Tools OS X for Xcode, because that does not contains sourcekitd.framework.

Problems

  • SPM does not support weak link yet
  • execv needed. I don't know how large this affects to other clients.

@norio-nomura
Change libclang.dylib and sourcekitd.framework to be weakly linked
eddf9f6
@norio-nomura
sourcekitten loads libclang.dylib and sourcekitd.framework from…
5a0b64b 
… active developer directory

`checkDepndenciesOfSourceKittenFramework()` called in main.swift does following:
- Check whether `libclang.dylib` and `sourcekitd.framework` are loaded or not
- If not, coordinates `DYLD_*` environment variables and calls `execv`
- New `sourcekitten` process launched by `execv` loads `libclang.dylib` and `sourcekitd.framework` using coordinated `DYLD_*` environment.

`DYLD_*` environment variables are coordinated to searching them in following order:
1. `$TOOLCHAIN_DIR/usr/lib` # for debugging on Xcode
2. `\`xcode-select -p\`/Toolchains/XcodeDefault.xctoolchain/usr/lib`
3. `/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib`
4. `/Applications/Xcode-beta.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib`
5. `$HOME/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib`
6. `$HOME/Applications/Xcode-beta.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib`
@norio-nomura
Delete rpathes from SourceKittenFramework on installables
d8a5eac
@norio-nomura
Change using DEVELOPER_DIR to TOOLCHAIN_DIR in `LD_RUNPATH_SEARCH…
aba5ecc 
…_PATHS`

`TOOLCHAIN_DIR` reflects overrided toolchain on Xcode 7.3 beta
@norio-nomura norio-nomura changed the title [WIP] Use xcode-select -p [WIP] Use xcode-select -p (weak link approach) Feb 12, 2016
@norio-nomura norio-nomura mentioned this pull request Feb 12, 2016
Closed
@segiddins
Copy link
Collaborator

This should also consider DEVELOPER_DIR

@norio-nomura
Copy link
Collaborator Author

This should also consider DEVELOPER_DIR

As far as I tested, DEVELOPER_DIR is reflected to TOOLCHAIN_DIR on Xcode.
Ah, but when we run DEVELOPER_DIR="/Applications/Xcode-beta.app" make and swiftlint is directly launched by make, we should consider that.
I'll add it.

@norio-nomura
Copy link
Collaborator Author

This should also consider DEVELOPER_DIR

DEVELOPER_DIR environment variable affects to result of xcode-select -p.

% xcode-select -p
/Applications/Xcode.app/Contents/Developer
% DEVELOPER_DIR="/Applications/Xcode-beta.app" xcode-select -p
/Applications/Xcode-beta.app/Contents/Developer

So, using result of xcode-select -p means considering DEVELOPER_DIR.
I won't add checking DEVELOPER_DIR individually.

zwaldowski
zwaldowski reviewed Feb 18, 2016
View reviewed changes
Source/SourceKittenFramework/DyldHelper.swift
let argv = CStringArray(arguments)
execv(path, argv.pointers) // never returns on normal flow
let error = String(UTF8String: strerror(errno))! // swiftlint:disable:this force_unwrapping
fatalError("execv failed: \(error)")
Copy link

Choose a reason for hiding this comment

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

Why not the stdlib's Process?

Copy link

Choose a reason for hiding this comment

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

If not, it might be better to follow the way the stdlib does instead of CStringArray.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I didn't know Process.unsafeArgv.
Thanks!

@zwaldowski
Copy link

And also consider the XCODE_DEFAULT_TOOLCHAIN_OVERRIDE env variable.

I don't think relaunching automatically is the right long-term solution for clients that link or embed SourceKittenFramework directly — for instance, that approach doesn't work right for a Cocoa app embedding SourceKittenFramework. We should dlsym the symbols we're using after finding the toolchain; this would also allow us to sandbox toolchain lookup into a single type, and would force us to harden the leaking of any stuff from the SourceKit or libclang headers. I've had success with this on a branch.

@norio-nomura
Copy link
Collaborator Author

I don't think relaunching automatically is the right long-term solution

I agree.

@norio-nomura
Copy link
Collaborator Author

And also consider the XCODE_DEFAULT_TOOLCHAIN_OVERRIDE env variable.

It seems XCODE_DEFAULT_TOOLCHAIN_OVERRIDE does not affect to TOOLCHAIN_DIR.
We need that.
Thanks!

@norio-nomura norio-nomura mentioned this pull request Feb 20, 2016
Closed
2 tasks
@norio-nomura
Copy link
Collaborator Author

This was replaced by #176 that using dlsym() approach.

@norio-nomura norio-nomura mentioned this pull request Mar 17, 2016
Closed
@norio-nomura norio-nomura deleted the nn-use-xcode-select-p branch October 18, 2016 00:59
@norio-nomura norio-nomura mentioned this pull request Jun 9, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@norio-nomura @segiddins @zwaldowski