Fix potentially broken support for macOS

[aryan9600]

Make changes to Makefile, so that libgit2.1.1.dylib can be found at runtime when running on a Mac M1. Closes #506.

[hiddeco]

@stefanprodan can you confirm this solves all your miseries?

[stefanprodan]

It doesn't

$ sudo rm /usr/local/lib/libgit2.1.1.dylib

$ gh pr checkout 515

$ LIBGIT2_FORCE=1 make run
PKG_CONFIG_PATH=/Users/stefanprodan/go/src/github.com/fluxcd/source-controller/hack/libgit2/lib/pkgconfig \
CGO_LDFLAGS="-Wl,-rpath,/Users/stefanprodan/go/src/github.com/fluxcd/source-controller/hack/libgit2/lib" \
go vet ./...
# pkg-config --cflags  -- libgit2
Package openssl was not found in the pkg-config search path.
Perhaps you should add the directory containing `openssl.pc'
to the PKG_CONFIG_PATH environment variable
Package 'openssl', required by 'libgit2', not found
pkg-config: exit status 1
make: *** [Makefile:131: vet] Error 2

[aryan9600]

Could you try updating your PKG_CONFIG_PATH to include the openssl stuff. Something like: PKG_CONFIG_PATH="/opt/homebrew/opt/[email protected]/lib/pkgconfig:/Users/stefanprodan/go/src/github.com/fluxcd/source-controller/hack/libgit2/lib/pkgconfig". I'll try updating the Makefile to respect the user's PKG_CONFIG_PATH as well.

[stefanprodan]

@aryan9600 as I posted in the issue, my env is set to:

$ cat ~/.bash_profile
export PATH="/opt/homebrew/opt/make/libexec/gnubin:$PATH"
export PATH="/opt/homebrew/opt/[email protected]/bin:$PATH"
export LDFLAGS="-L/opt/homebrew/opt/[email protected]/lib"
export CPPFLAGS="-I/opt/homebrew/opt/[email protected]/include"
export PKG_CONFIG_PATH="/opt/homebrew/opt/[email protected]/lib/pkgconfig"

[stefanprodan]

This works:

vet: $(LIBGIT2)	## Run go vet against code
ifdef MAC_M1
	PKG_CONFIG_PATH="$(LIBGIT2_LIB_PATH)/pkgconfig:$$PKG_CONFIG_PATH" \
	CGO_LDFLAGS="-Wl,-rpath,$(LIBGIT2_LIB_PATH)" \
	go vet ./...
	cd api; go vet ./...
else
	PKG_CONFIG_PATH=$(LIBGIT2_LIB_PATH)/pkgconfig \
	go vet ./...
	cd api; go vet ./...
endif

[stefanprodan]

Is this breaking on macOS Intel? If not I would replace MAC_M1 with Darwin.

[aryan9600]

Don't have an Intel mac to test it on, I'm afraid. This seemed the safer way to go, since the current Makefile works on Intel Macs.

[stefanprodan]

@squaremo can you please test this on your mac?

[squaremo]

My mac may not be the best for testing this, since it's not a newcomer to the libgit2 battlefront. I've got libraries, pkg-config files, and env entries littered everywhere. Indeed, make run works with the changes, without the changes, and when I remove everything except for go run ./main.go.

I would like to understand why -rpath works where LD_LIBRARY_PATH doesn't, since these do pretty similar things. That explanation could usefully go in the commit message :-)

[aryan9600]

I'm not an expert on dynamic linking, but I'll try to explain what I think is going on.

> otool -L bin/manager
bin/manager:
        /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1853.0.0)
        /System/Library/Frameworks/Security.framework/Versions/A/Security (compatibility version 1.0.0, current version 60157.30.13)
        @rpath/libgit2.1.1.dylib (compatibility version 1.1.0, current version 1.1.1)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1311.0.0)

The thing of our interest is @rpath/libgit2.1.1.dylib. From this article about @rpath:

Starting in 10.5, Apple provides @rpath, which is a solution to this. When placed at the front of an install name, this asks the dynamic linker to search a list of locations for the library.

So, at runtime when our binary tries to load libgit2.1.1.dylib, it tries to search for it in the dirs that fall under @rpath. Presumably, the linker ignores LD_LIBRARY_PATH because of the usage of @rpath.

---

Why this is happening only on M1 and not on Intel, is beyond my understanding I'm afraid.

[aryan9600]

I spun up my very old Intel Mac running macOS 10.13. The old Makefile errored out with the same error whereas the new Makefile works just the same as it does on macOS arm64. I'm starting to think that it was working on Intel Macs accidentally rather than the way it was intended to. It might be something to do with the go version like you said (i ran go 1.17 on both laptops). Let me try running it using go 1.16 on my Intel Mac and get back to you.

[squaremo]

I'm starting to think that it was working on Intel Macs accidentally

I think it is less likely to be to do with the CPU, and more to do with the individual machines on which it gets run; or failing that explanation, the OS version.

[aryan9600]

Same error on macOS 10.13 running go 1.16 :)

[stefanprodan]

I think it used to work for us because we've installed libgit2 with brew, back when the version matched what Flux wants.

[squaremo]

I think it used to work for us because we've installed libgit2 with brew, back when the version matched what Flux wants.

Yeah I think you're right -- we both did this, and that's pretty much the pool of test machines accounted for :-)

(By the way, I verified that it was working by coincidence by moving /usr/local/lib/libgit2.* out of the way -- the executable then fails to start.)

@aryan9600 I think you can reframe this PR as "Work by design rather than by coincidence, on MacOS" :-) (And you don't need to have a special case for M1 Macs, to answer the original question ...)

[stefanprodan]

LGTM

Thanks @aryan9600 🥇

PS. Please squash your commits. image-automation-controller needs the same changes, please open a PR there.

[squaremo]

Please squash your commits

.. and give the resulting commit a message that describes why this change is being made.

[cwyl02]

tried and this works for my intel MacOS 11.6.1

[stefanprodan]

Thanks @cwyl02 for testing this on Intel 🏅

[hiddeco]

With the last changes, it correctly detects my OpenSSL installation without requiring any changes to my shell environment (on Apple M1):

I would like to make the additional suggestion to introduce a TEST_ARGS as git2go has with the same default, to ensure Go does not look at cached linker configurations.

[hiddeco]

Discussed this further with @aryan9600, and the proposed changes in #515 (comment) can be combined with getting the changes in from fluxcd/helm-controller#369.