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

GCCBootstrap: Bump libtapi version #1625

Merged
merged 2 commits into from
Sep 1, 2020
Merged

GCCBootstrap: Bump libtapi version #1625

merged 2 commits into from
Sep 1, 2020

Conversation

Keno
Copy link
Contributor

@Keno Keno commented Aug 31, 2020

Upgrades libtapi to the latest version in
https://github.com/tpoechtrager/apple-libtapi, which in particular
picks up support for the v4 format for TAPI, which is used in
the macOS 11 SDK.

Upgrades libtapi to the latest version in
https://github.com/tpoechtrager/apple-libtapi, which in particular
picks up support for the v4 format for TAPI, which is used in
the macOS 11 SDK.
@Keno Keno marked this pull request as draft September 1, 2020 00:12
@Keno
Copy link
Contributor Author

Keno commented Sep 1, 2020

@Keno
Copy link
Contributor Author

Keno commented Sep 1, 2020

That said, maybe we shouldn't be building LLVM tools for this copy of llvm in the first place. Or maybe we should see if we can just build libtapi with our regular LLVM build and avoid building LLVM again.

@Keno
Copy link
Contributor Author

Keno commented Sep 1, 2020

We're also building all the backends, which is entirely unnecessary. Also, we're building one copy of this for each GCC version, which we don't need either. Not necessarily something we need to fix now, but worth revisiting in the future.

@@ -315,7 +315,6 @@ function gcc_script(compiler_target::Platform)
# If we're on MacOS, we need to install cctools first, separately.
if [[ ${COMPILER_TARGET} == *-darwin* ]]; then
cd ${WORKSPACE}/srcdir/apple-libtapi
atomic_patch -p1 "${WORKSPACE}/srcdir/patches/libtapi_fullyaml.patch"
Copy link
Member

Choose a reason for hiding this comment

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

Is this patch no longer necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no

@staticfloat
Copy link
Member

When I originally glued this all together, the LLVM build that we make was not compatible with libtapi, I don't think. Also, we don't build just one LLVM version; we build multiple versions and swap them out, independently of the GCC version. Somehow I don't think libtapi is going to be happy with having LLVM 6 at one moment and LLVM 10 the next.

Since libtapi isn't changing from GCC version to GCC version, I think perhaps what we should do is extract this out into the PlatformSupport shard, and build it only once for each Darwin target, then see if we can link against it nicely in GCCBoostrap.

@Keno
Copy link
Contributor Author

Keno commented Sep 1, 2020

Putting it in PlatformSupport seems fine. I understand why it's in GCCBootstrap at the moment, since it basically replaces binutils, but those do vary with GCC version, while on Darwin, we only support one version of the linker (and libtapi).

@staticfloat
Copy link
Member

I understand why it's in GCCBootstrap at the moment, since it basically replaces binutils, but those do vary with GCC version, while on Darwin, we only support one version of the linker (and libtapi).

Yeah..... perhaps the truly right way to do this is to go back to a separate Binutils_jll that we grab the appropriate version of for each GCC shard.

@Keno Keno marked this pull request as ready for review September 1, 2020 20:16
@Keno Keno requested a review from staticfloat September 1, 2020 20:16
@Keno Keno merged commit 483c5df into master Sep 1, 2020
@Keno Keno deleted the kf/bumptapi branch September 1, 2020 22:06
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.

2 participants