-
Notifications
You must be signed in to change notification settings - Fork 373
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
Replace C++ build scripts with pixi
commands
#4097
Conversation
pixi
commands
This reverts commit 77e4ae7.
# TODO(emilk): make this work somehow. Right now this just results in | ||
# > Compiler: GNU 12.3.0 (/__w/rerun/rerun/.pixi/env/bin/x86_64-conda-linux-gnu-c++) | ||
# 😭 | ||
# - name: Build and run C++ tests with clang++ | ||
# shell: bash | ||
# run: | | ||
# pixi run cpp-clean | ||
# WARNINGS_AS_ERRORS=ON CXX=clang++ pixi run cpp-build-all | ||
# WARNINGS_AS_ERRORS=ON CXX=clang++ pixi run cpp-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because we have a dependency on the:
c-compiler = "1.6.0.*"
cxx-compiler = "1.6.0.*"
packages.
See: https://anaconda.org/conda-forge/cxx-compiler
This package is a generic way to obtain the C++ compiler for your system that conda-forge used to compile its ecosystem. This compiler is, therefore, guaranteed to be ABI compatible with the conda packages you have installed.
This causes CXX to be overridden inside the pixi environment.
I believe I added this when I was trying to get arrow-cpp
dependency to work correctly on windows. Ironically, windows is now that one platform where Andreas has now removed the c[++]-compiler
.
I'm 90% sure we can pretty safely drop this at this point and I've confirmed the CXX
flag will be respected once you have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting! Let's pull on that thread in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great.
With pixi managing the deps, in theory we shouldn't need to use the rerunio/ci_docker:0.10.0
image any more, but that should probably wait for another PR as well.
### What * Closes #4090 I tried getting these tests running on CI too, but after 2h I gave up. Andreas has volunteered to try a bit more, so let's hold off merging this PR until we can reproduce the problems on CI. See #4097 if you dare ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/4098) (if applicable) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG - [PR Build Summary](https://build.rerun.io/pr/4098) - [Docs preview](https://rerun.io/preview/a69d06b8a84b78469192f341bebdfc32dc7936cc/docs) <!--DOCS-PREVIEW--> - [Examples preview](https://rerun.io/preview/a69d06b8a84b78469192f341bebdfc32dc7936cc/examples) <!--EXAMPLES-PREVIEW--> - [Recent benchmark results](https://ref.rerun.io/dev/bench/) - [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
What
I also tried to get clang++ running on CI, but so far I've failed.
Checklist