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

cmd/go: explicitly specify macOS base SDK version when compiling go binary #35459

Closed
andybons opened this issue Nov 8, 2019 · 30 comments
Closed
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin
Milestone

Comments

@andybons
Copy link
Member

andybons commented Nov 8, 2019

This is needed to ensure that Apple’s notarization service can check that the go binary is using (at minimum) the 10.9 SDK.

$ codesign -dvv /usr/local/go/bin/go
Executable=/usr/local/go/bin/go
Identifier=go
Format=Mach-O thin (x86_64)
CodeDirectory v=20200 size=116482 flags=0x0(none) hashes=3636+2 location=embedded
Library validation warning=OS X SDK version before 10.9 does not support Library Validation
Signature size=9042
Authority=Developer ID Application: Google, Inc. (EQHXZ8M8AV)
Authority=Developer ID Certification Authority
Authority=Apple Root CA
Timestamp=Oct 31, 2019 at 7:23:46 PM
Info.plist=not bound
TeamIdentifier=EQHXZ8M8AV
Sealed Resources=none
Internal requirements count=1 size=164

Notice Library validation warning=OS X SDK version before 10.9 does not support Library Validation

The SDK is not properly specified in the go binary:

$ otool -l /usr/local/go/bin/go | grep -B1 -A3 MIN_MACOS
Load command 5
      cmd LC_VERSION_MIN_MACOSX
  cmdsize 16
  version 10.10
      sdk n/a

All other binaries built with the toolchain have the correct values:

$ otool -l /usr/local/go/pkg/tool/darwin_amd64/vet | grep -B1 -A3 MIN_MACOS
Load command 5
      cmd LC_VERSION_MIN_MACOSX
  cmdsize 16
  version 10.9
      sdk 10.9

The fix for this will likely need to be backported. Hopefully it’s simple. 🤞

Related issues:

@andybons andybons added OS-Darwin NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Nov 8, 2019
@andybons andybons added this to the Go1.14 milestone Nov 8, 2019
@andybons andybons self-assigned this Nov 8, 2019
@andybons
Copy link
Member Author

It seems that we want to specify the -mmacosx-version-min when compiling the go binary.

#18400 is related.

@andybons
Copy link
Member Author

The fact that the SDK isn’t specified in the Mach-O headers makes me wonder if the xcode toolchain we’re using to compile cgo binaries at release time may need to be updated. When I build the toolchain on a darwin-amd64-10_11 machine, I get the same results, while when I build the toolchain locally, I get:

$ otool -l ../bin/go | grep -B1 -A3 MIN_MAC
Load command 5
      cmd LC_VERSION_MIN_MACOSX
  cmdsize 16
  version 10.10
      sdk 10.15

When running xcode-select -p on the gomote builder I get /Library/Developer/CommandLineTools while when I run it locally, I get /Applications/Xcode.app/Contents/Developer. It may be that without the full Xcode install, we won’t get the SDK version set because we don’t have the SDK installed. Yay.

@cagedmantis @toothrot @dmitshur.

@networkimprov
Copy link

cc @eliasnaur @odeke-em

@eliasnaur
Copy link
Contributor

If the Go linker detects a load command in an external .o file, it will use that. We can't just pass -mmacosx-version-min to CC, because the load commands for macOS, iOS etc. are mutually exclusive.

In internal linking mode, the Go linker sets a default LC_VERSION_MIN_MACOSX version. The version was bumped to 10.9 in https://go-review.googlesource.com/c/go/+/175918/.

Perhaps it's enough to update/fix the toolchain used to build Go release. However, we could also expand the linker check above ("machoPlatform == 0") to trigger when the version and sdk are less than 10.9 in external linking mode. If you do that, please keep my https://go-review.googlesource.com/c/go/+/206337 in mind to only use the macOS load command for macOS binaries.

@andybons
Copy link
Member Author

@ianlancetaylor for cgo.

It may be OK to specify the MACOSX_DEPLOYMENT_TARGET env var when building the toolchain only and not for every cgo binary.

@networkimprov
Copy link

cc @thanm

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/208268 mentions this issue: cmd/release: use macOS 10.15 (Catalina) for releases

@andybons andybons changed the title cmd/go: go binary not specifying min macOS SDK version (needs 10.9) cmd/go: go binary not specifying macOS base SDK version causing warning in codesign validation Nov 22, 2019
@andybons
Copy link
Member Author

andybons commented Nov 22, 2019

Confirmed that a toolchain built using the new 10.15 builder with Xcode installed and no changes to the min deployment target gets rid of the warning from codesign when validating the go command.

$ codesign -dvv go/bin/go
Executable=/private/tmp/go/bin/go
Identifier=go
Format=Mach-O thin (x86_64)
CodeDirectory v=20500 size=116526 flags=0x10000(runtime) hashes=3636+2 location=embedded
Signature size=9063
Authority=Developer ID Application: Andrew Bonventre (USS93J4HN5)
Authority=Developer ID Certification Authority
Authority=Apple Root CA
Timestamp=Nov 21, 2019 at 7:35:42 PM
Info.plist=not bound
TeamIdentifier=USS93J4HN5
Runtime Version=10.15.0
Sealed Resources=none
Internal requirements count=1 size=164

The issue may or may not affect notarization, since the min SDK version is still being set to 10.10 for the go command no matter what builder we’re using (I was misreading the Mach-O header values above) and the minimum for notarization is 10.9. Also, the notarization service is currently not reporting any errors or warnings with the go binary if it’s signed, has a secure timestamp, and has the hardened runtime enabled. Whether the notarization service would throw an error with the old SDK values sometime in the future is up for speculation, but it’s better to be safe than sorry.

It may be fine to leave the target SDK version at the default (10.10) and not set it explicitly, but I worry about defaults changing down the road.

gopherbot pushed a commit to golang/build that referenced this issue Nov 22, 2019
This builder has Xcode installed instead of just the command-line
tools. Because the SDKs are absent when just the command-line tools
are installed, no base SDK version is being set in the go binary
(it uses external linking) and codesign was printing a warning about
library validation. Updating to use this builder gets rid of that
warning.

Updates golang/go#35459

Change-Id: Ib548c998042f499c2d57324e9a8746594ea7060d
Reviewed-on: https://go-review.googlesource.com/c/build/+/208268
Run-TryBot: Andrew Bonventre <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Carlos Amedee <[email protected]>
@andybons andybons changed the title cmd/go: go binary not specifying macOS base SDK version causing warning in codesign validation cmd/go: explicitly specify macOS base SDK version when compiling go binary Dec 4, 2019
@andybons
Copy link
Member Author

andybons commented Dec 4, 2019

Removing release-blocker label since the default value for the base SDK is 10.10 using the 10_15 Catalina builder. Keeping open to track setting this explicitly.

@toothrot
Copy link
Contributor

/cc @dmitshur @cagedmantis

@cagedmantis cagedmantis self-assigned this Dec 13, 2019
@luckymateo
Copy link

Is that the only way to solve the problem by updating the Mac system ? @andybons

@cagedmantis
Copy link
Contributor

@huoxingyun We've isolated what the fix would be and will be implementing it soon.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/215721 mentions this issue: cmd/go: add -d flag to mod_get_test

gopherbot pushed a commit that referenced this issue Jan 22, 2020
'go get all' was run in this test without -d. This caused some std
packages to be reinstalled if the test is run in a slightly different
configuration than make.bash was run. run.bash would fail in some
situations because of this. Nothing in the cmd/go tests should modify
installed std or cmd packages.

Updates #35459

Change-Id: Idd259a27d55502923b7fc54f361a77f0ac11eea2
Reviewed-on: https://go-review.googlesource.com/c/go/+/215721
Run-TryBot: Jay Conrod <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Michael Matloob <[email protected]>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/215957 mentions this issue: cmd/link: ensure cgo cflags do not leak into tvOS test

gopherbot pushed a commit that referenced this issue Jan 23, 2020
Running the 'TestBuildForTvOS' test with CGO_CFLAGS set
with certain values would cause the test to fail. all.bash
would fail when CGO_CFLAGS was set to '-mmacosx-version-min=10.10'
because the --macosx-version-min flag is incompatible with tvOS.
The change guards against using an unintended flag in the unit test.

Updates #35459

Change-Id: Ifc43f3ebfb23d37aabeaac2ea9efae5b877991bf
Reviewed-on: https://go-review.googlesource.com/c/go/+/215957
Run-TryBot: Carlos Amedee <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/216177 mentions this issue: cmd/link: ensure cgo cflags do not leak into dwarf tests

@belloyang
Copy link

Is there a solution to this issue?
-- Feb. 18, 2020

@toothrot
Copy link
Contributor

@cagedmantis What else is needed on this issue? Should the milestone be changed to 1.15?

@toothrot toothrot modified the milestones: Go1.14, Go1.15 Feb 25, 2020
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/218598 mentions this issue: [release-branch.go1.13] cmd/link: ensure cgo cflags do not leak into tvOS test

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/217057 mentions this issue: [release-branch.go1.13] cmd/link: ensure cgo cflags do not leak into dwarf tests

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/217059 mentions this issue: [release-branch.go1.13] cmd/go: fix cgo test when min macOS version is set

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/218597 mentions this issue: [release-branch.go1.13] cmd/go: add -d flag to mod_get_test

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/221100 mentions this issue: cmd/release: set the minimum macOS version supported by go 1.13

gopherbot pushed a commit that referenced this issue Feb 26, 2020
…tvOS test

Running the 'TestBuildForTvOS' test with CGO_CFLAGS set
with certain values would cause the test to fail. all.bash
would fail when CGO_CFLAGS was set to '-mmacosx-version-min=10.10'
because the --macosx-version-min flag is incompatible with tvOS.
The change guards against using an unintended flag in the unit test.

Updates #36846
Updated #35459

Change-Id: Ifc43f3ebfb23d37aabeaac2ea9efae5b877991bf
Reviewed-on: https://go-review.googlesource.com/c/go/+/215957
Run-TryBot: Carlos Amedee <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
(cherry picked from commit ace25f8)
Reviewed-on: https://go-review.googlesource.com/c/go/+/218598
gopherbot pushed a commit that referenced this issue Feb 26, 2020
…dwarf tests

Running the dwarf tests with CGO_CFLAGS set
with certain values would cause the test to fail. all.bash
would fail when CGO_CFLAGS was set to '-mmacosx-version-min=10.10'
because the --macosx-version-min flag is incompatible with some dwarf
tests. The change guards against using an unintended flag in the unit test.

Updates #36846
Updates #35459

Change-Id: Idc9b354aba44fdab424cb0081a4b3ea7a6d0f8e3
Reviewed-on: https://go-review.googlesource.com/c/go/+/216177
Run-TryBot: Carlos Amedee <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Jay Conrod <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
(cherry picked from commit e948d2b)
Reviewed-on: https://go-review.googlesource.com/c/go/+/217057
gopherbot pushed a commit that referenced this issue Feb 26, 2020
'go get all' was run in this test without -d. This caused some std
packages to be reinstalled if the test is run in a slightly different
configuration than make.bash was run. run.bash would fail in some
situations because of this. Nothing in the cmd/go tests should modify
installed std or cmd packages.

Updates #36846
Updates #35459

Change-Id: Idd259a27d55502923b7fc54f361a77f0ac11eea2
Reviewed-on: https://go-review.googlesource.com/c/go/+/215721
Run-TryBot: Jay Conrod <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Michael Matloob <[email protected]>
(cherry picked from commit 71bbffb)
Reviewed-on: https://go-review.googlesource.com/c/go/+/218597
Run-TryBot: Carlos Amedee <[email protected]>
Reviewed-by: Jay Conrod <[email protected]>
gopherbot pushed a commit that referenced this issue Feb 26, 2020
…s set

Regression tests for #24161 use a macro to conditionally compile some
stub definitions. The macro tests that the minimum macOS version is
less than 10.12.

We get duplicate definitions when building this test with
CGO_CFLAGS=-mmacosx-version-min=10.x where 10.x < 10.12. With this
change, we use a different macro, __MAC_OS_X_VERSION_MAX_ALLOWED__,
which tests the SDK version instead of the minimum macOS version. This
checks whether these definitions are present in headers.

After this change, 'go tool dist test cgo_test' should pass with
CGO_FLAGS=-mmacosx-version-min=10.10.

Updates #36846
Updates #35459

Change-Id: I88d63601c94b0369c73c38d216a2d41ba7d4e579
Reviewed-on: https://go-review.googlesource.com/c/go/+/216243
Run-TryBot: Jay Conrod <[email protected]>
Reviewed-by: Keith Randall <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
(cherry picked from commit 1f9f88b)
Reviewed-on: https://go-review.googlesource.com/c/go/+/217059
Run-TryBot: Carlos Amedee <[email protected]>
Reviewed-by: Jay Conrod <[email protected]>
gopherbot pushed a commit to golang/build that referenced this issue Feb 26, 2020
Set the minmacos version depending on the version of go being packaged
for macOS releases. Removed support for Go 1.12 since it is no longer
supported.

Fixes golang/go#36846
Updates golang/go#35459

Change-Id: Ifb9a0296fde5ed87da72d7719a714744484cc7db
Reviewed-on: https://go-review.googlesource.com/c/build/+/221100
Run-TryBot: Carlos Amedee <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Alexander Rakoczy <[email protected]>
@ianlancetaylor
Copy link
Member

What else needs to be done for this issue?

@jayconrod
Copy link
Contributor

I believe this is resolved now, but if anyone knows differently, please reopen.

@cagedmantis
Copy link
Contributor

Indeed. The work for this has been done. Thanks for closing the issue.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/278435 mentions this issue: cmd/release: set macOS deployment target for darwin/arm64 too

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/322629 mentions this issue: cmd/go,cmd/link: do not check for staleness in most tests

gopherbot pushed a commit that referenced this issue May 27, 2021
Instead, check that stale packages in the standard library
are not rebuilt when already present in the build cache,
and are not installed implicitly when rebuilt.

We retain the staleness checks for the runtime package in tests
involving '-i', because those are guaranteed to fail anyway if the
package is stale and the "stale" failure message is arguably clearer.
They can be removed if/when we remove the '-i' flag, but the runtime
package is less likely to become stale because it does not have cgo
dependencies.

Fixes #46347
Updates #33598
Updates #35459
Updates #41696

Change-Id: I7b0a808addd930f9f4911ff53ded62272af75a40
Reviewed-on: https://go-review.googlesource.com/c/go/+/322629
Trust: Bryan C. Mills <[email protected]>
Run-TryBot: Bryan C. Mills <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
Reviewed-by: Jay Conrod <[email protected]>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/381854 mentions this issue: cmd/go: rewrite TestScript/cgo_stale_precompiled to be agnostic to staleness

gopherbot pushed a commit that referenced this issue Jan 31, 2022
…aleness

The configuration set by x/build/cmd/releasebot causes runtime/cgo to
be stale in the darwin/amd64 release (see #36025, #35459).
That staleness is mostly benign because we can reasonably assume that
users on macOS will either disable CGO entirely or have a C compiler
installed to rebuild (and cache) the stale packages if needed.

Fixes #50892
Fixes #50893
Updates #46347

Change-Id: Ib9ce6b5014de436264238f680f7ca4ae02c9a220
Reviewed-on: https://go-review.googlesource.com/c/go/+/381854
Trust: Bryan Mills <[email protected]>
Run-TryBot: Bryan Mills <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Russ Cox <[email protected]>
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin
Projects
None yet
Development

No branches or pull requests

10 participants