Issue 5697065: code review 5697065: crypto/tls: use dlsym to determine symbol availability ... (Closed)
|
Created:
12 years, 2 months ago by mkrautz Modified:
12 years, 2 months ago CC:
golang-dev, mjhollins_gmail.com Visibility:
Public. |
Descriptioncrypto/tls: use dlsym to determine symbol availability in OS X root fetcher
Fixes issue 3131.
Patch Set 1 #Patch Set 2 : diff -r 0fa62412dbdd https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 0fa62412dbdd https://go.googlecode.com/hg/ #MessagesTotal messages: 14
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com, mjhollins@gmail.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
LGTM, although I know very little about OS X's system APIs. Will leave for someone who, at least, runs a Mac to glace at.
Sign in to reply to this message.
On 2012/02/26 21:20:41, agl1 wrote: > LGTM, although I know very little about OS X's system APIs. Will leave for > someone who, at least, runs a Mac to glace at. LGTM. Tested on Mac OS X 10.6.8. BTW, I think we probably should fix the original weak import approach after Go 1.
Sign in to reply to this message.
Far from thrilled about this. We have had no problems running all.bash on any known Mac systems. The problems we have are building on one system and taking the binaries elsewhere. I believe there is an environment variable that Xcode respects called something like MAC_OS_X_VERSION_FOR_BUILD that will make us generate the right binaries for these other systems. However, to test we need to identify the problematic (system-we-built-on, system-the-binaries-don't-run-on) combinations. Russ
Sign in to reply to this message.
On 2012/02/27 16:09:09, rsc wrote: > Far from thrilled about this. We have had no problems > running all.bash on any known Mac systems. The problems > we have are building on one system and taking the binaries > elsewhere. I believe there is an environment variable that > Xcode respects called something like MAC_OS_X_VERSION_FOR_BUILD > that will make us generate the right binaries for these other systems. > > However, to test we need to identify the problematic > (system-we-built-on, system-the-binaries-don't-run-on) > combinations. > > Russ Hi Russ, The real problem is simply that the linker (I assume) is discarding the weak import annotations for the Security.framework symbols. This causes binaries built using a 10.7 SDK not to load at all on 10.6, since we're using symbols aren't available there (SecItemExport). Here's the relevant technical note from Apple: https://developer.apple.com/library/mac/#technotes/tn2064/_index.html I suppose we could just set MAC_OS_X_VERSION_MAX_ALLOWED to 10.6 and use the deprecated symbol, if you wish. Note that it's only deprecated, not removed. I was just trying to be future proof when I wrote the code.
Sign in to reply to this message.
On 2012/02/27 16:09:09, rsc wrote: > Far from thrilled about this. We have had no problems > running all.bash on any known Mac systems. The problems > we have are building on one system and taking the binaries > elsewhere. I believe there is an environment variable that > Xcode respects called something like MAC_OS_X_VERSION_FOR_BUILD > that will make us generate the right binaries for these other systems. How about add this (w/o this CL): #cgo LDFLAGS: --with-macos-sdk=/Developer/SDKs/MacOSX10.6.sdk I've tested this on 10.6, but I know it won't break Mac OS X 10.6.
Sign in to reply to this message.
On 2012/02/27 16:32:54, minux wrote: > On 2012/02/27 16:09:09, rsc wrote: > > Far from thrilled about this. We have had no problems > > running all.bash on any known Mac systems. The problems > > we have are building on one system and taking the binaries > > elsewhere. I believe there is an environment variable that > > Xcode respects called something like MAC_OS_X_VERSION_FOR_BUILD > > that will make us generate the right binaries for these other systems. > How about add this (w/o this CL): > #cgo LDFLAGS: --with-macos-sdk=/Developer/SDKs/MacOSX10.6.sdk > > I've tested this on 10.6, but I know it won't break Mac OS X 10.6. That might work, but with Apple's move to a single-app Xcode with 4.2, SDKs now live inside the app bundle. It would be hard to handle the divide between pre and post Xcode 4.2. The other problem is that Apple typically removes old SDKs from Xcode once a new version's out. So come Mountain Lion, the 10.6 SDK probably won't be bundled with Xcode anymore. And I don't think the SDK version matters - it's fine to build using a newer SDK so long as you only use symbols that are available on the systems you're targetting (use use the MACOSX_VERSION_MAX_ALLOWED, etc. macros to do that).
Sign in to reply to this message.
I think we should make sure our binary distribution scripts use MACOSX_VERSION_MAX_ALLOWED (thanks for tracking that down) and not worry about working around it in each package that uses cgo. Russ
Sign in to reply to this message.
On 2012/02/27 16:51:01, rsc wrote: > I think we should make sure our binary distribution > scripts use MACOSX_VERSION_MAX_ALLOWED > (thanks for tracking that down) and not worry about > working around it in each package that uses cgo. > > Russ Note that its a preprocessor define, not an env var. Can we force that through the scripts?
Sign in to reply to this message.
On Tue, Feb 28, 2012 at 12:57 AM, <krautz@gmail.com> wrote: > On 2012/02/27 16:51:01, rsc wrote: > >> I think we should make sure our binary distribution >> scripts use MACOSX_VERSION_MAX_ALLOWED >> (thanks for tracking that down) and not worry about >> working around it in each package that uses cgo. >> > > Russ >> > > Note that its a preprocessor define, not an env var. Can we force that > through the scripts? > I think currently we can't. There is a pending TODO in cmd/go/build.go line 1168. I've made a CL for this, http://codereview.appspot.com/5699091 With that CL, you can define TARGET_CC="gcc -D MACOSX_VERSION_MAX_ALLOWED=xxx" in misc/dist/darwin/dist.bash. But I don't know whether this approach is OK.
Sign in to reply to this message.
Hmm. Maybe the best thing in this case is the #cgo CFLAGS line that was suggested. It's per-package, which I'm not wild about, but it is significantly less intrusive. I am not planning to add any new environment variables to the go command. Russ
Sign in to reply to this message.
On 2012/02/27 17:27:27, rsc wrote: > Hmm. > > Maybe the best thing in this case is the #cgo CFLAGS line that was suggested. > It's per-package, which I'm not wild about, but it is significantly > less intrusive. > I am not planning to add any new environment variables to the go command. > > Russ OK. But as I mentioned, SDK paths vary widly depending on which Xcode version you have. This is (hopefully) only an issue until we, as suggested by Minux, honor the weak import annotations. Until then I think we should just strive to use the availability macros in all OS X stdlib cgo files that use frameworks (currently only crypto/tls) to force our API usage to be 10.6-compatible. (That's our lower bound, right?) See this CL: http://codereview.appspot.com/5700083/ It complains if I use APIs introduced in Lion, and does not throw deprecation warnings at me for using the (in Lion), deprecated SecKeychainExportItem function. I still need to figure out why it only works with the two underscores before the MAX_ALLOWED define, though. What do you think?
Sign in to reply to this message.
5700083 looks good to me.
Sign in to reply to this message.
|
