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

Update toolchains #10

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Update toolchains #10

wants to merge 3 commits into from

Conversation

def-
Copy link

@def- def- commented Dec 11, 2024

Incremental compile time for bin/mzcompose improves from 6min28s to 13s on my system by using lld.

I'm also not sure how to upload the artifacts for this to homebrew. @benesch I guess you have access to do so.

Moved this PR from #9 because it refers to the main branch via tap directory.

@def- def- force-pushed the main branch 2 times, most recently from eaa208c to f1376c1 Compare December 11, 2024 01:43
@def- def- requested review from benesch and ParkMyCar December 11, 2024 01:44
@def- def- force-pushed the main branch 3 times, most recently from dfb34f4 to 22d715f Compare December 11, 2024 02:28
Copy link
Member

@benesch benesch left a comment

Choose a reason for hiding this comment

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

Unless there's something in particular we need from newer versions of glibc/Linux, I'd prefer to keep those at their existing versions. The versions of those were intentionally chosen to be the oldest versions of glibc/Linux supported for each architecture's toolchain, to maximize the compatibility of the resulting binary. (A binary that's linked against a certain version of glibc/Linux can't run on a system that uses an older version of glibc/Linux.)

Upgrading gcc, binutils, et al. seems fine although fair warning that newer versions of gcc/binutils do not always improve compilation speed.

Anyway, if this really does bring down link times as much as you say, this is really exciting!

lib/toolchain.rb Outdated Show resolved Hide resolved
lib/toolchain.rb Outdated
Comment on lines 193 to 195
brew_prefix = Pathname.new(ENV["HOMEBREW_PREFIX"] || "/usr/local/homebrew")
ln_s brew_prefix/"bin/lld", "mnt/install/bin/ld.lld"
Copy link
Member

Choose a reason for hiding this comment

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

This looks sketchy. I think the standard pattern looks more like this: https://stackoverflow.com/a/73408649

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

@def- def- Dec 11, 2024

Choose a reason for hiding this comment

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

I don't think the stackoverflow thread's approach works to get lld working. I did find another approach though, so this PR might not be necessary to get the lld speedup: MaterializeInc/materialize#30797

I still think we should bring the crosstools-ng in macOS to parity with ci-builder.

@@ -0,0 +1,140 @@
From: Francois-Xavier Coudert <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

Why not use gcc 13.3? It seems like this patch is included in 13.3.

Copy link
Author

Choose a reason for hiding this comment

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

I wanted to make sure we have the same versions as in ci-builder (plus these fixes for the build)

@benesch
Copy link
Member

benesch commented Dec 11, 2024

In theory, the bottles will be automatically added by https://github.com/MaterializeInc/homebrew-crosstools/blob/main/.github/workflows/publish.yml after merge. Who knows if that's still working after all these years, though.

@benesch
Copy link
Member

benesch commented Dec 11, 2024

@def- here's a patch that will get the builds going: e472af9

(I couldn't push at this PR since it's based on your main branch.)

@def-
Copy link
Author

def- commented Dec 11, 2024

I did not want to upgrade toolchains, but it's my understanding that in homebrew you can't pick an older version as a dependency, so I had to upgrade crosstools-ng to get a new build. I picked the same versions we use in ci-builder. We do a similar thing to get lld support there: https://github.com/MaterializeInc/materialize/blob/92acf2387bb0ff7331d7d452857d399801fb314a/ci/builder/Dockerfile#L213-L214

@def- def- force-pushed the main branch 3 times, most recently from 05bd2f0 to 27fc073 Compare December 11, 2024 13:38
@def- def- changed the title Support lld + update toolchains Update toolchains Dec 11, 2024
@def-
Copy link
Author

def- commented Dec 11, 2024

Something is still wrong here. I can build this successfully with brew untap def-/homebrew-crosstools; brew tap def-/homebrew-crosstools && brew install --formula Formula/aarch64-unknown-linux-gnu.rb --build-from-source, but in the end the binaries are missing.

I also don't understand why brew doctor is failing: https://github.com/MaterializeInc/homebrew-crosstools/actions/runs/12278420051/job/34260051710?pr=10

@def- def- force-pushed the main branch 4 times, most recently from 36c27c2 to 8a28f56 Compare December 11, 2024 15:48
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