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

docs: xcaddy build flags #385

Merged
merged 3 commits into from
Dec 14, 2023
Merged

docs: xcaddy build flags #385

merged 3 commits into from
Dec 14, 2023

Conversation

dunglas
Copy link
Owner

@dunglas dunglas commented Dec 14, 2023

@dunglas dunglas changed the title chore: improve automaxprocs logs docs: xcaddy build flags Dec 14, 2023
Copy link
Collaborator

@withinboredom withinboredom left a comment

Choose a reason for hiding this comment

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

LGTM, fwiw.

This got me thinking though: I wonder if we should set these env vars in the base images (dunglas/frankenphp:latest-builder*) so people don't need to even worry about it?

I suppose it still needs to be documented for native builds, but then that can just be a table of flags required per OS (or put in a Makefile).

> Otherwise, you may get errors like `PHP Fatal error: Maximum call stack size of 83360 bytes reached during compilation. Try splitting expression`
>
> To do so, change the `XCADDY_GO_BUILD_FLAGS` environment variable to something like
> `XCADDY_GO_BUILD_FLAGS=$'-ldflags "-w -s -extldflags \'-Wl,-z,stack-size=0x80000\'"'`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks weird to me: $'-ldflags "-w -s...'

Why is the $ there?

Copy link
Owner Author

Choose a reason for hiding this comment

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

To be able to escape the inner '.

Copy link

Choose a reason for hiding this comment

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

this syntax does not seem to work. I have in my Dockerfile:

ENV CGO_ENABLED=1 XCADDY_SETCAP=1
ENV XCADDY_GO_BUILD_FLAGS=$'-ldflags "-w -s -extldflags \'-Wl,-z,stack-size=0x80000\'"'
RUN xcaddy build \

And i get an error:

failed to solve: failed to process "$'-ldflags \"-w -s -extldflags \\'-Wl,-z,stack-size=0x80000\\'\"'": unexpected end of statement while looking for matching double-quote

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect it is supposed to be:

ENV XCADDY_GO_BUILD_FLAGS='-ldflags "-w -s -extldflags \'-Wl,-z,stack-size=0x80000\'"'

without the $ since we're not referencing a variable, though I haven't tested it.

Copy link

Choose a reason for hiding this comment

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

This doesn't work either:

failed to solve: failed to process "'-ldflags \"-w -s -extldflags \\'-Wl,-z,stack-size=0x80000\\'\"'": unexpected end of statement while looking for matching double-quote

This variant is ok in Dockerfile:

ENV XCADDY_GO_BUILD_FLAGS="-ldflags '-w -s -extldflags \'-Wl,-z,stack-size=0x80000\''"

But it fails while building the image:

[ERROR] Splitting arguments failed: -ldflags '-w -s -extldflags \'-Wl,-z,stack-size=0x80000\''  

@dunglas
Copy link
Owner Author

dunglas commented Dec 14, 2023

Good idea to put these flags in the base builder image (but no Makefile please, go has all the tooling we need). Do you want to work on a PR?

@dunglas
Copy link
Owner Author

dunglas commented Dec 14, 2023

By the way, I think that it would be better to use xcaddy to build the official images instead of our custom main.go.
We'll need caddyserver/caddy#5711 first.

@dunglas dunglas merged commit 1da3dbc into main Dec 14, 2023
38 of 39 checks passed
@dunglas dunglas deleted the docs/xcaddy-build-flags branch December 14, 2023 21:45
@withinboredom
Copy link
Collaborator

Good idea to put these flags in the base builder image

Yeah, I can take care of that and update the documentation.

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.

3 participants