Repository navigation
Conversation
derSteFfi
changed the title
|
one ToDo I forgot in notes: tar can exclude files from extraction. The current shell-script does delete a few folders in a few repositories - we could modify the tar call to not extract them in the first place. |
1f4928c to
ffbdf87
Compare
|
the remaining CI failures are not specific to the Makefile but actual problems, so marking this ready. |
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the build_bench_env.sh script with a Makefile-based build system to improve consistency and maintainability. The change switches from git cloning to downloading versioned archives from GitHub and centralizes version management.
Key changes:
- Replace shell script with comprehensive Makefile for building allocators and benchmarks
- Introduce VERSIONS file to centralize version management across all components
- Move package installation documentation from script to README with per-distribution instructions
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| build-bench-env.sh | Add python-six dependency for ArchLinux package installation |
| bench.sh | Update library paths and remove hardcoded version variables |
| VERSIONS | New file defining versions and sources for all allocators and benchmarks |
| README.md | Replace build script documentation with Makefile usage and detailed dependency lists |
| Makefile | New comprehensive build system with targets for all allocators and benchmarks |
| Dockerfile | Update to use Makefile instead of build script and add package installations |
| .github/workflows/all.yml | Remove Fedora exclusion from CI matrix |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
mjp41
left a comment
mjp41
left a comment
There was a problem hiding this comment.
This is looking pretty tidy. Thanks for working on this.
mjp41
left a comment
mjp41
left a comment
There was a problem hiding this comment.
LGTM, thanks for the changes.
@jvoisin, @derSteFfi I don't want to hold these changes holding off on fixing issues for individual allocators/platforms. But I really like to only have a Green CI. Thoughts?
|
Absolutely agree. I still have at least scudo and lp on my list and will re-check all comments here afterwards. With that fixed, I would also be pro-merging to make it easier to hunt individual bugs. The branch-switching sometimes takes time, and we would have a clear stand - fixing individual things is much easier than to always have to model the whole process. |
0266c64 to
2d0082e
Compare
|
@jvoisin the MR is up-to-date with master - what do you think about merging? |
|
~All the tests are failing :/ |
|
sorry, missed a reference to |
|
There are still a bunch of failures it seems |
|
these are the same we have on master, except that fedora is deactivated in the CI on master (don't remember why). alpine base container and ubuntu/tcg fail with master, too. |
|
Then it does look good to me. I'll let @daanx hit the merge button as it's his project :) |
d0028ed to
90a3c8a
Compare
|
re-synced with master. Also added rmalloc and fixed sc. Would love to not have to resolve merge conflicts like these every few months... |
derSteFfi
changed the title
|
I missed a few bumps in the merge, should be synchronized now. |
bc691e3 to
c976d1a
Compare
|
rebased onto master, added support for versioned archives. |
|
@daanx friendly ping on this :) |
9b1b4a3 to
924f37b
Compare
|
re-based again. |
924f37b to
8e6c358
Compare
|
I've rebased the MR again and included the first fix (8e6c358): ruby was missing on alpine in both the README as well as the Dockerfile. In the README, it was missing for ArchLinux, too. Problem is: I don't want to fix things in the old shell script, but also do not want to pollute this MR. Fixes that do not touch the building of the suite are trivial, but everything that touches build-bench-env.sh is complicated. Shall I:
Let me know :) |
cbc53bc to
e97e0ef
Compare
The build was a ~970-line bash script that re-did work on every run and had to be kept in sync with bench.sh by hand. Replace it with a Makefile plus a VERSIONS file (one line per project: name, version, commit, url) that builds each allocator and benchmark as an unpack/configure/build stamp chain: incremental, parallel via -j, dependency-aware, and usable for a single allocator (`make je`) as well as everything (`make all`). Adapt bench.sh to the new layout: allocator and benchmark directories lose their version suffixes, the working-directory check no longer references the deleted script, and the rpmalloc, diehard, and mesh library paths follow the new build outputs. Version pins move from bench.sh into VERSIONS.
Install the dependencies the new build needs: rust/cargo for smalloc, bazel via the official package repository (the way build-bench-env.sh did) for the mesh/nomesh/tcg targets, and the rocksdb/lua/redis build deps. Pin base images (fedora:44, alpine:3.24, ubuntu:24.04) so builds reproduce. Adjust the platform workflow's known-failure notes accordingly (tcg's bazel target is gone upstream; alpine no longer gets a special bazel case).
Replace the build-bench-env.sh usage sections with make targets (allocs, benchmarks, individual allocators, -j behaviour), move the per-distribution package lists to match the Dockerfile, and mention VERSIONS as the single place to bump allocator versions.
Download each project into archives/<name>-<label>.tar.gz, where the label is the version tag or a short sha, and unpack from there into extern/<name>. The version is now visible on disk, and each unpack stamp records "<label> <sha>": the pin the build tree was made from. Version bumps were invisible to make before: with unversioned archive names and empty stamps, an existing extern/<name> tree was never refreshed after VERSIONS moved. Freshness is now keyed by the pin: the archive target is named after the pinned version, so editing a pin gives the project a new prerequisite and make re-fetches, re-unpacks, and rebuilds exactly that project, while everything else stays put. The archive targets are declared explicitly (static pattern rules) to keep make from treating them as intermediate files it may silently skip or delete. lp and scudo unpack by sparse git clone instead of using an archive. Pins are full shas now, and git clone -b only accepts branch names, so fetch the sha explicitly and check it out detached; a marker file named after their pin, archives/<name>-<label>.clone, triggers their re-clone and nothing else. Add a clean-archives target that sweeps stale archives and markers but keeps the pinned ones, and in the README point at VERSIONS instead of build-bench-env.sh for the versions.
The bazel tcmalloc target is a static library, so bench.sh finds no shared object to preload and runs the job with an empty allocator list. Add a linkshared cc_binary for it.
hardened_malloc maps guard pages per allocation and sh6bench keeps tens of thousands of them alive, so it needs over 140000 mappings; at the default 65530 mprotect fails and malloc returns null although memory is free.
libhoard.cpp does not define them but Heap-Layers' wrapper.cpp calls them, so libhoard.so has undefined symbols and every c++ test dies at its first sized delete. Define them as weak defaults, as the mac wrapper does.
The exclusion was for a bazel target error that is gone at the pinned version. Fedora has no bazel installed and alpine does not compile tcmalloc with clang.
c0e2d53 to
6f1ed81
Compare
pushing this draft as a starter to get your opinions - I'll be on vacation the next weeks and won't be working on it anyway. There are a few open tasks, but the general vibe should already come across. Feedback very welcome.
Decisions made:
TODO
put archives in extra storage after versionizing them (see here)docker caches these alreadyopen Questions: