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

V2 #14

Merged
merged 70 commits into from
May 25, 2021
Merged

V2 #14

merged 70 commits into from
May 25, 2021

Conversation

tonistiigi
Copy link
Owner

No description provided.

base/test-go.bats Outdated Show resolved Hide resolved
@crazy-max
Copy link
Collaborator

Looking at xx-info env I think you can close #13. That's a neat addition thanks!

env Print XX_* variables defining target environment
is-cross Exit cleanly if target is not native architecture
libc Print used libc (musl or gnu)
march Print target machine architecture, uname -m
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is inconsistent with the other *-arch commands and not as obviously coming from uname. Would prefer uname-arch. And also wonder if we shouldn't reverse it to arch-* instead so it's arch-alpine, arch-debian, arch-uname, arch-pkg.

@crazy-max
Copy link
Collaborator

crazy-max commented Jan 5, 2021

Would be nice to add support for MIPS systems: GOOS=linux GOARCH=mips GOMIPS=softfloat go build.

if case $TARGETARCH in "mips"*) true;; *) false;; esac; then
  if [ -n "$TARGETVARIANT" ]; then
    export GOMIPS="$TARGETVARIANT"
  else
    export GOMIPS="hardfloat"
  fi
fi

@tonistiigi
Copy link
Owner Author

tonistiigi commented Jan 5, 2021

@crazy-max I did add some mips support to verify script. I've no specific need to provide support for it but as I noticed that both alpine and debian provide packages(might be different variant though) I don't mind PRs to add it to other scripts as well(wait until I move this out of draft). It's much less common in container-world though. The official images do not support mips and to support GOMIPS the variant component needs to be defined in https://github.com/containerd/containerd/blob/master/platforms/platforms.go#L98 first.

@crazy-max
Copy link
Collaborator

@tonistiigi

It's much less common in container-world though.

Yes indeed I'm using this snippet only to create artifacts through bake actually.

@tonistiigi
Copy link
Owner Author

@crazy-max Regarding mips, https://github.com/moby/buildkit/tree/master/util/archutil would probably need support as well for consistency.

@tonistiigi
Copy link
Owner Author

Pushed some new Dockerfiles for building tools to v2...wip-toolchains-v2.

I wonder if for https://github.com/tonistiigi/xx/compare/v2...wip-toolchains-v2?expand=1#diff-b093d34eec92adf6e0cc6e445edb7147ea45d25fb0c98f2f84a60f5aae6f34f7R177 case it would make sense to add xx-clang --print-cmake-defs that would just print value of echo -DPKG_CONFIG_EXECUTABLE=$(xx-clang --print-prog-name=pkg-config) -DCMAKE_C_COMPILER_TARGET=$(xx-clang --print-target-triple) -DCMAKE_CXX_COMPILER_TARGET=$(xx-clang++ --print-target-triple) -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ to avoid the typing.

base/xx-verify Outdated
fi

if [ -n "$static" ]; then
if ! echo $out | grep "statically linked" >/dev/null; then
Copy link
Collaborator

@tiborvass tiborvass Jan 20, 2021

Choose a reason for hiding this comment

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

I'd prefer using readelf -d $out | grep -q NEEDED (cf https://twitter.com/tiborvass/status/1277154999689076737). Readelf is in binutils package.

Copy link
Collaborator

@tiborvass tiborvass Jan 20, 2021

Choose a reason for hiding this comment

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

Ah... it wouldn't work for non-linux.

Copy link
Owner Author

Choose a reason for hiding this comment

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

it's another dependency and only works on elf

@tonistiigi
Copy link
Owner Author

@crazy-max @tiborvass I wonder what is the best pattern to use for cross-os builds and static toolchains. I have toolchains for binutils, cctools, compiler-rt etc. I wonder how to publish them so they can be easily used.

For the tools that target a specific platform need to make an image that has target in the name: eg binutils-v2.36-linux-arm64-alpine.

Then in the Dockerfile you would use it with

FROM --platform=$BUILDPLATFORM tonistiigi/xx:binutils-v2.36-$TARGETOS-$TARGETARCH$TARGETVARIANT-alpine AS binutils

FROM alpine
COPY --from=binutils / /

For things like compiler-rt, macos sdk, windows libraries that are always for a specific platform already I think it makes more sense to leave the image to their target platform. Eg. windows arm libraries would run with pull --platform=windows/arm tonistiigi/xx:sdk. Otherwise, if we would use the previous pattern all of them would need a special image, a more complicated way to include them, and all the images would have the same layers for all platforms.

That brings us to the case where you need both. Eg. to build for macos, you need an sdk with the headers for the target but you also need support binaries targeting macos like ld64 and signer for your current build platform. I'd like to avoid the case where even to do a simple build you need to include 2 extra images, with a different include model. I'm wondering if maybe we could instead release the static binaries of the tooling as github binary releases in this repo. Then scripts like xx-clang/xx-apt/xx-apk will automatically download the release in their --setup-target-triple loader. We can still do the image releases as well. WDYT?

@tiborvass
Copy link
Collaborator

@tonistiigi sounds good to me.

We can still do the image releases as well.

What would the names look like?

@tonistiigi
Copy link
Owner Author

tonistiigi commented Feb 17, 2021

@tiborvass ld64-static ld-$os-$arch . Don't think we want to release whole binutils as static binaries. Actually, the only big problem with ld is s390x that is not supported by lld . Could also do a musl based release of whole binutils (debian already has packages for glibc based ones). Not sure how much of the rest of the cctools (except ld64) is that useful to require a release. I think github releases for only things that the xx-* scripts will load in automatically is enough.

@crazy-max
Copy link
Collaborator

@tonistiigi

Sounds good to me too

I'm wondering if maybe we could instead release the static binaries of the tooling as github binary releases in this repo. Then scripts like xx-clang/xx-apt/xx-apk will automatically download the release in their --setup-target-triple loader.

I'm worried about this one because we have to handle proxy support and other exotic behavior for some users. I'm ok to release an image even if it's a huge one.

@tiborvass
Copy link
Collaborator

@tonistiigi Sorry I don't understand the consistency between ld64-static and $os-$arch-ld. I'm fine with --setup-target-triple downloading ld64 from github releases but the extra images are in case someone wants to/needs to do the extra work in the Dockerfile (such as behind a proxy, although would be better to require a more seamless experience that doesn't require changing the Dockerfile). Anyway, so how would the user use these ld64 images?

@tonistiigi
Copy link
Owner Author

@tiborvass

FROM --platform=$BUILDPLATFORM tonistiigi/xx:binutils-v2.36-$TARGETOS-$TARGETARCH$TARGETVARIANT-alpine AS binutils

FROM alpine
COPY --from=binutils / /
FROM --platform=$BUILDPLATFORM tonistiigi/xx:ld64-static AS ld64

FROM alpine
COPY --from=ld64 / /

the ld example was for the binaries release. For image release, we can just use musl based binutils.

@tonistiigi
Copy link
Owner Author

I'm ok to release an image even if it's a huge one.

The issue isn't that images are big. It is that a lot of manual work is needed by user in their Dockerfile to use it. With binary release we can put this logic in the xx- script but we can't pull image layers in the script. Image cache would still apply if --setup command is put on a separate instruction.

@tiborvass
Copy link
Collaborator

@tonistiigi what about $os-$arch-ld ? I guess it seems inconsistent to me compared to ld64-static or much more likely: i'm missing something.

@tonistiigi
Copy link
Owner Author

@tiborvass This was an example of the binary release. The difference is that ld64 is a mac specific linker that can target both amd64 and arm64. While ld can only target one specific architecture. So separate ones need to be built for each.

@tiborvass
Copy link
Collaborator

@tonistiigi I see you edited the image names in #14 (comment) to make it consistent, so I'm fine with ld64-static and ld-$os-$arch.

@tiborvass
Copy link
Collaborator

@crazy-max

I'm worried about this one because we have to handle proxy support and other exotic behavior for some users. I'm ok to release an image even if it's a huge one.

I guess what is being suggested here at least as a first step, is to have a simpler Dockerfile UX for those not behind a proxy but those behind a proxy can use the specific images, just a bit longer to write in a Dockerfile.

@crazy-max
Copy link
Collaborator

@tiborvass

I guess what is being suggested here at least as a first step, is to have a simpler Dockerfile UX for those not behind a proxy but those behind a proxy can use the specific images, just a bit longer to write in a Dockerfile.

Yes sorry I misinterpreted the initial objective and I understand the issue of having a Dockerfile as simple as possible. I guess --setup-target-triple is the best way to handle that.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi tonistiigi marked this pull request as ready for review May 25, 2021 00:32
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
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