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

Makefile: Make all builds explicitly fully static (disable CGO) #3466

Merged
merged 6 commits into from
Sep 19, 2024

Conversation

JoeKar
Copy link
Collaborator

@JoeKar JoeKar commented Sep 11, 2024

Due to this the "Linux 64 fully static" has been marked deprecated and is only kept for compatibility with:
https://github.com/benweissmann/getmic.ro/blob/f90870e948afab8be9ec40884050044b59ed5b7c/index.sh#L197-L204

Fixes #3463

@dmaluka
Copy link
Collaborator

dmaluka commented Sep 11, 2024

Shouldn't we just set CGO_ENABLED=0 in Makefile instead?
We don't want unnecessary divergence between released binaries and binaries built manually by the user?

@JoeKar
Copy link
Collaborator Author

JoeKar commented Sep 12, 2024

Hm, exactly this was my intention in the first place to keep the capability to link it dynamically.
But since the Makefile is indeed the smarter place to do this change we can add a new build target too keep both use cases.

@JoeKar JoeKar changed the title tools/cross-compile: Make all builds explicitly fully static (disable CGO) Makefile: Make all builds explicitly fully static (disable CGO) Sep 12, 2024
echo "Linux 64 fully static"
CGO_ENABLED=0 GOOS=linux GOARCH=amd64 make build
echo "Linux 64 fully static (deprecated)"
# Only kept for https://github.com/benweissmann/getmic.ro/blob/f90870e948afab8be9ec40884050044b59ed5b7c/index.sh#L197-L204
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we rather ask @benweissmann to update the script?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I directly provided the PR: benweissmann/getmic.ro#40
But we've to keep in mind, that this will take effect earliest with the upcoming release.

Copy link
Collaborator

@dmaluka dmaluka Sep 12, 2024

Choose a reason for hiding this comment

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

But we've to keep in mind, that this will take effect earliest with the upcoming release.

Sh*t... so this means that if we remove linux64-static and merge benweissmann/getmic.ro#40, getmic.ro will be broken for Musl until the next release?

Choose a reason for hiding this comment

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

I would suggest doing one release where you publish statically-linked binaries using both the old name and new name (i.e., have the same statically linked binary published both with and without the -static suffix). Then, I can update getmic.ro after that release is published to cut over to the new name seamlessly, and future releases can drop the -static suffix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

SGTM.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated
CGO_ENABLED=0 go build -trimpath -ldflags "-s -w $(GOVARS) $(ADDITIONAL_GO_LINKER_FLAGS)" ./cmd/micro

build-dynamic: generate
CGO_ENABLED=1 go build -trimpath -ldflags "-s -w $(GOVARS) $(ADDITIONAL_GO_LINKER_FLAGS)" ./cmd/micro
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems a bit messy, and "asymmetric" (no quick version of build-dynamic, no dynamic version of build-dbg). Maybe instead of adding a target, just add a variable, e.g. DYNAMIC? I.e. just:

build-quick:
	CGO_ENABLED=$(DYNAMIC) go build ...

build-dbg:
	CGO_ENABLED=$(DYNAMIC) go build ...

And then we can run DYNAMIC=1 make build or whatever.

Also need to update (i.e. rewrite) the Fully static binary section in README.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Damn, this "hidden" darwin dependency tools/info-plist.go 🤦‍♂️
So we've to exclude darwin somehow or can't generalize it in the Makefile.

Maybe define ADDITIONAL_GO_LINKER_FLAGS to be empty and extend it only in case the GOOS is darwin and forcing CGO_ENABLED to be 1 afterwards. 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, my first thought was: what the hell is this plist and can we just get rid of it or its -linkmode external. But it is not obvious if it is possible and how, and I have no wish to figure that out.

Maybe define ADDITIONAL_GO_LINKER_FLAGS to be empty and extend it only in case the GOOS is darwin and forcing CGO_ENABLED to be 1 afterwards. 🤔

Seems to make sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW found its roots: #513

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe define ADDITIONAL_GO_LINKER_FLAGS to be empty and extend it only in case the GOOS is darwin and forcing CGO_ENABLED to be 1 afterwards. 🤔

Should do the trick (for) now.
But to be honest, this kind of meta info is only included in the moment micro is natively build on macOS. It's completely missing, at least since the builds are cross compiled via GitHub actions, in our prebuilt binaries. 🤷‍♂️

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I've just realized that, after looking at GOHOSTOS in your patch.

Which makes this feature from #513 even more... interesting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although before 6945aa3 this feature was used for cross-compiled binaries as well... but I doubt it was really working correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now, should we document this (the fact that MacOS builds are an exception, but only native ones) in README as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, better to document before someone is complaining.

Although before 6945aa3 this feature was used for cross-compiled binaries as well... but I doubt it was really working correctly.

Me too, since building micro with darwin AND CGO_ENABLED=1 requires clang.
We could try it with GOOS instead of GOHOSTOS as switch, but then we most probably run into trouble cross compiling for darwin where clang is not available. This might hit the GitHub actions etc.pp

@JoeKar JoeKar force-pushed the fix/linux-dynamic2static branch 2 times, most recently from 58033b8 to 8257bde Compare September 17, 2024 19:23
@JoeKar JoeKar merged commit 71da59f into zyedidia:master Sep 19, 2024
6 checks passed
@JoeKar JoeKar deleted the fix/linux-dynamic2static branch September 19, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GLIBC Errors in 2.0.14-rc1 and 2.0.14
3 participants