Adjust CI to handle build executables for supported platforms #176

elken · 2025-01-21T13:05:19Z

Marked as draft as this has conflicts with a recent change, and I am offering these changes as a proposal. Aware I also have to re-enable tests as well. Figured I'd get your input early 😄

If you want me to drop certain bits I can, and I think there's probably a cleaner way we can handle the various on/off states we have across runs.

Once this is merged, I have changes needed ready to go for homebrew-jank to pick this up.

jeaye

This is great to see, Ellis! I think it's excellent to build for macOS, but let's keep Ubuntu using apt and use bash scripts which we can lint. I've left notes for how we can fit this into the existing build matrix, too.

jeaye · 2025-01-21T18:04:38Z

.github/actions/build/action.yml

+      run: |
+        if [[ "$OSTYPE" == "linux"* ]]; then
+          eval "$(/home/linuxbrew/.linuxbrew/bin/brew shellenv)"
+          sudo apt-get install -y libzip-dev libbz2-dev libgc-dev
+        fi


I think it's good to have Linux brew in the matrix, but it should not replace apt, simply due to the fact that the vast majority of Ubuntu/Debian users use apt and not Linux brew. It's more impactful for us to test what people will be using.

I would assume (correct me if I'm wrong) that a Linux binary built with Linux brew deps will not work on a Linux machine without Linux brew installed, due to dynamic lib dependency hell. If I'm right about that, using CI to make our Ubuntu builds would then be forcing Linux brew onto everyone, right?

Note: If we do add Linux brew separately, that would be our 8th build job. Nothing wrong with that, just clarifying.

Won't necesserily break at all no, you would just still need the deps installed which will be taken care of when we ship .deb and .rpms. Linux can load the objects from anywhere, they don't have to be installed via brew and in fact if they weren't then the LD paths will already be correctly loaded. I'll prove this next time I'm on my desktop.

EDIT: Running in a fresh ubuntu-24.10 distrobox and all I did was install clang-19, libgc-dev and libzip-dev (which also conveniently gives me the minimum dependencies I need for later ha) and it fails purely because clojure.core is missing since I just copied the exe to /tmp so the linker can't find the brew deps.

The only difference in package managers is brew has a good enough version of clang which prevents us having to spend several hours building it. Once ubuntu-24.10 images start shipping we can use those as it ships with clang-19.

I did try every variation of Clang I could but for some reason it always failed for weird errors.

Think I have a way around this to have the best of both worlds here, but my point above still stands about the package manager in use.

Ok, gotcha. I'm confident that you know more about packaging that I do. 🙃 Just wanted to make sure we considered this. If it works and it's simpler, great!

No worries! You're right to question it as it does add some potential confusion 😄

jeaye · 2025-01-21T19:19:47Z

.github/actions/build/action.yml

+        if [[ "$OSTYPE" == "darwin"* ]]; then
+            export SDKROOT=$(xcrun --sdk macosx --show-sdk-path)
+        fi
+
+        if [[ "$OSTYPE" == "linux"* ]]; then
+          eval "$(/home/linuxbrew/.linuxbrew/bin/brew shellenv)"
+        fi
+
+        export PATH="$(brew --prefix)/opt/llvm/bin:${PATH}"
+        export LDFLAGS="-Wl,-rpath,$(brew --prefix)/opt/llvm/lib ${LDFLAGS}"
+        export CPPFLAGS="-I$(brew --prefix)/opt/llvm/include ${CPPFLAGS}"
+
+        ${{ github.workspace }}/compiler+runtime/bin/configure -GNinja \
+                                                               -DCMAKE_BUILD_TYPE=${{ matrix.build_type }} #\
+                                                               #-Djank_tests=on \
+                                                               # -Djank_analysis="${ANALYSIS:-off}" \
+                                                               # -Djank_coverage="${COVERAGE:-off}"
+
+        ${{ github.workspace }}/compiler+runtime/bin/compile
+        # ${{ github.workspace }}/compiler+runtime/bin/test
+        # LLVM_PROFILE_FILE=build/test.profraw ./bin/test
+        # llvm-profdata merge --sparse build/test.profraw -o build/test.profdata
+        # llvm-cov show ./build/jank-test --instr-profile build/test.profdata > coverage.txt


This was in bin/ci/test so that we can easily benefit from linting it (we have a CI job for this). In general, I'm opposed to writing large amounts of bash in yaml files, so I tend to pull those into scripts.

I did debate it as it was getting quite large, no worries then will amend

jeaye · 2025-01-21T19:20:50Z

.github/actions/build/action.yml

-        export DESTDIR=${{ github.workspace }}/jank-${{ matrix.os }}-$(date +'%Y-%m-%d').$(git rev-parse --short $GITHUB_SHA)
+        export OS_NAME=$(uname | tr "[:upper:]" "[:lower:]")
+        export FOLDER=jank-${OS_NAME}-$(uname -m)
+        export DESTDIR=${{ github.workspace }}/${FOLDER}
        ${{ github.workspace }}/compiler+runtime/bin/install
-        tar czf $DESTDIR.tar.gz $DESTDIR/
-        echo "archive=$DESTDIR.tar.gz" >> $GITHUB_OUTPUT
+
+        cp -R ${DESTDIR} .
+
+        tar czf ${DESTDIR}.tar.gz ${FOLDER}/
+        echo "archive=${DESTDIR}.tar.gz" >> $GITHUB_OUTPUT


This would be good in a bin/ci/install script which we can lint, too.

jeaye · 2025-01-21T19:21:20Z

.gitmodules

 [submodule "third-party/vcpkg"]
-	path = compiler+runtime/third-party/vcpkg
-	url = https://github.com/jank-lang/vcpkg.git
+        path = compiler+runtime/third-party/vcpkg
+        url = https://github.com/jank-lang/vcpkg.git
 [submodule "compiler+runtime/third-party/folly"]


Very odd to see changes in here.

Tabs to spaces. My editor converts them automatically on save.

jeaye · 2025-01-21T19:22:27Z

compiler+runtime/CMakeLists.txt

@@ -257,6 +257,8 @@ include(cmake/dependency/fmt.cmake)
 include(cmake/dependency/libzippp.cmake)

 find_package(OpenSSL REQUIRED COMPONENTS Crypto)
+set(Boost_USE_STATIC_LIBS ON)
+set(Boost_USE_STATIC_RUNTIME OFF)


What's the static runtime? Will you please put it as a comment so I can also know in 6 months when I ask myself the same thing? 😂

I can actually remove that, thanks for spotting

jeaye · 2025-01-21T19:23:57Z

compiler+runtime/bin/ci/validate-formatting

+if [[ -e "./build/llvm-install" ]]; then
+    clang_format="./build/llvm-install/usr/local/bin/clang-format"
+else
+    clang_format="$(command -v clang-format)"
+fi


Please drop the then down, Allman style. Also, this file has 2 space indentation.

Worth adding an editorconfig file, I can add also if you want

Up to you if it's worth it.

jeaye · 2025-01-21T19:26:03Z

.github/workflows/build.yml

-        os: [ubuntu-24.04]
+        os: [ macos-15]


For macOS, given the new matrix on main, we should probably just include a Debug + Release job. All of the analysis, codecov, and sanitization can happen on Ubuntu. We have 5 build jobs (1 lint) right now, so this should put us at 7 build jobs.

I want to have a re-think on adjusting the pipeline to better streamline the steps while still handling the Debug/Release steps.

Are you sure there's no value in having macOS also do those?

I don't think there's no value in macOS doing sanitization, but I'm also wary of exploding our CI jobs. If you'd like to try with all the jobs first, we can do that.

TODO Fill this in when done

jeaye requested changes Jan 21, 2025

View reviewed changes

elken force-pushed the main branch 14 times, most recently from 6eeb701 to 3002be6 Compare January 23, 2025 07:45

elken added 2 commits January 23, 2025 07:57

Adjust CI to build executables for supported platforms

ce48c4b

Build Boost static

8f02bdc

elken force-pushed the main branch from 3002be6 to 6601216 Compare January 23, 2025 07:58

Response to review

642f3b5

TODO Fill this in when done

elken force-pushed the main branch from 6601216 to 642f3b5 Compare January 23, 2025 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust CI to handle build executables for supported platforms #176

Adjust CI to handle build executables for supported platforms #176

elken commented Jan 21, 2025 •

edited

Loading

jeaye left a comment

jeaye Jan 21, 2025

jeaye Jan 21, 2025

elken Jan 22, 2025 •

edited

Loading

elken Jan 22, 2025

jeaye Jan 22, 2025

elken Jan 23, 2025

jeaye Jan 21, 2025

elken Jan 22, 2025

jeaye Jan 21, 2025

jeaye Jan 21, 2025

elken Jan 22, 2025

jeaye Jan 21, 2025

elken Jan 22, 2025

jeaye Jan 21, 2025

elken Jan 22, 2025

jeaye Jan 22, 2025

jeaye Jan 21, 2025

elken Jan 22, 2025

jeaye Jan 22, 2025

Adjust CI to handle build executables for supported platforms #176

Are you sure you want to change the base?

Adjust CI to handle build executables for supported platforms #176

Conversation

elken commented Jan 21, 2025 • edited Loading

jeaye left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elken Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elken commented Jan 21, 2025 •

edited

Loading

elken Jan 22, 2025 •

edited

Loading