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

Fixes build issues which occur when building on Apple Silicon #77

Merged
merged 1 commit into from
Dec 16, 2023

Conversation

mcbarton
Copy link
Contributor

@mcbarton mcbarton commented Nov 20, 2023

Link to xeus-zmq
Add option to define where cxxopts folder is (Macbook unable to locate folder without)
Change header file xeus/xserver_zmq.hpp to xeus-zmq/xserver_zmq.hpp
Add #include to include/xeus-clang-repl/xmanager.hpp otherwise errors due to use of std::cerr and others

Once fixes were applied I found I could build on Apple Silicon, but not everything in the xeus-clang-repl notebooks worked.
Fixes #74
Fixes #75

@mcbarton mcbarton marked this pull request as draft November 25, 2023 19:44
@p4vook
Copy link

p4vook commented Nov 27, 2023

yeah cmake in this project as a whole needs some work

@mcbarton mcbarton marked this pull request as ready for review December 5, 2023 21:15
@mcbarton
Copy link
Contributor Author

mcbarton commented Dec 5, 2023

@vgvassilev Any suggestions on this pull request? There is almost certainly a better way to do what I have done with cxxopts, but its the only solution I could get to work on my Macbook. After this pull request is approved, I need to make a change to CppInterOp and I should be able to create a build script for those who want to test xeus-clang-repl on their Apple Silicon Macbooks.

@mcbarton
Copy link
Contributor Author

mcbarton commented Dec 5, 2023

After Github CI workflow failed I realised I need to update Github workflow CI to install xeus-zmq due to my changes, and in the Dockerfile also (and probably a few other places which I haven't realised yet I suspect). Will do this sometime in the next few days. Any other issues raised will be fixed with the commit which fixes this issue.

@mcbarton
Copy link
Contributor Author

mcbarton commented Dec 13, 2023

I have just pushed a commit to that updates the Dockerfile so that it builds on arm. It should be considered a draft, which I plan to refine, now that I have it building. Other known issues left to do are

  1. Test amended CI
  2. Update README

@mcbarton
Copy link
Contributor Author

mcbarton commented Dec 14, 2023

@vgvassilev This pull request is ready for review.

@@ -45,6 +45,7 @@ RUN \
###libomp-dev \
# Other "our" apt installs (development and testing)
build-essential \
clang-15 \
Copy link
Collaborator

Choose a reason for hiding this comment

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

In ci.yaml matrix we use clang-runtime==17. Can you use the newer version (17) of the clang runtime here as well? It should be able to since the CI test passes with version 17.

Copy link
Contributor Author

@mcbarton mcbarton Dec 15, 2023

Choose a reason for hiding this comment

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

I used clang-15 as that is the latest version of clang I could find in the apt repository which is available for Ubuntu 22.04 . Also when building llvm-project I had to build version 16 as building version 17 and using this with CppInterOp would build but fail with this command on an arm architechture (I cannot remember off the top of my head what message it failed with). It is not currently possible to switch this build of llvm-project to version 17, as although it passes on x86, it doesn't pass on arm (from tests on my local machine).

python -c "import cppyy" && \

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, yes. This is a good one. I am misled that conda-forge also has the 17th version, but this is APT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could put a fixme next the part of the dockerfile which builds llvm-project version 16.x , until the issue using 17.x on arm is resolved (I'd raise it as an issue, so that there is a log of what the error is), and put a fixme next to using Clang-15 from apt (when the next LTS version of Ubuntu comes out April next year this would have the latest version of Clang since its in apt repository for version 23.10 of Ubuntu, so would be a quick fix).

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Comment on lines +258 to +263
export CC=/usr/bin/clang-15 && \
export CXX=/usr/bin/clang++-15 && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

clang-17?

then \
export CC=/usr/bin/clang-15 && \
export CXX=/usr/bin/clang++-15 && \
git clone --depth=1 --branch release/16.x https://github.com/llvm/llvm-project.git && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

release/17.x

cd ./llvm-project/ && \
mkdir build && \
export LLVM_DIR=$(pwd) && \
compgen -G "../CppInterOp/patches/llvm/clang16-*.patch" > /dev/null && find ../CppInterOp/patches/llvm/clang16-*.patch -printf "%f\n" && git apply ../CppInterOp/patches/llvm/clang16-*.patch && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

clang17-*.patch?

Copy link
Collaborator

@alexander-penev alexander-penev left a comment

Choose a reason for hiding this comment

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

In different places we use different versions of clang. Maybe it's a good idea to standardize and replace them with the latest supported version (17?).
Otherwise, everything else looks good.

@p4vook
Copy link

p4vook commented Dec 15, 2023

There is also an issue where xeus-clang-repl links to its older library instead of the just built one. It isn't specifically related to this PR, but it would be nice to fix.

Link to xeus-zmq
Add option to define where cxxopts folder is (Macbook unable to locate folder without)
Change header file xeus/xserver_zmq.hpp to xeus-zmq/xserver_zmq.hpp
Add #include <iostream> to include/xeus-clang-repl/xmanager.hpp otherwise errors due to use of std::cerr and others
Fixes dockerfile so builds on arm based machines
Changes CI to include extra dependencies
Added fixme messages to Dockerfile due to issues with arm based build and clang-17
@mcbarton
Copy link
Contributor Author

Added fixme messages to Dockerfile due to clang-17 issues and build on arm

@@ -20,6 +20,7 @@

#include "nlohmann/json.hpp"

#include <iostream>
Copy link

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mentioned in the PR description. When building natively on MacOS it would not build without this include due to the use of std::cerr and others

@alexander-penev alexander-penev merged commit 9a00131 into compiler-research:main Dec 16, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants