wasix-org / wasix-libc

wasix libc implementation for WebAssembly
https://wasi.dev
Other
114 stars 19 forks source link

Github Actions Workflow for building a C/C++ sysroot with CI tests. #19

Closed SanderVocke closed 1 year ago

SanderVocke commented 1 year ago

This workflow script could be used to build a sysroot which can then be released on this repository.

A few notes:

SanderVocke commented 1 year ago

The memcpy patch I used on the wasix-sysroot repo:

From f85c1e80be293dd3946f36ddfcd404d2634896f2 Mon Sep 17 00:00:00 2001
From: wasix-sysroot builder <wasix_sysroot@notreal.com>
Date: Wed, 6 Sep 2023 10:17:42 +0000
Subject: [PATCH 2/2] Fix duplicate memcpy

---
 libc-top-half/musl/include/sched.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/libc-top-half/musl/include/sched.h b/libc-top-half/musl/include/sched.h
index 3d3fbe8..c5f25b1 100644
--- a/libc-top-half/musl/include/sched.h
+++ b/libc-top-half/musl/include/sched.h
@@ -5,6 +5,7 @@ extern "C" {
 #endif

 #include <features.h>
+#include <__functions_memcpy.h>

 #define __NEED_struct_timespec
 #define __NEED_pid_t
@@ -81,9 +82,7 @@ int clone (int (*)(void *), void *, int, void *, ...);
 int unshare(int);
 int setns(int, int);

-void *memcpy(void *__restrict, const void *__restrict, size_t);
 int memcmp(const void *, const void *, size_t);
-void *memset (void *, int, size_t);
 void *calloc(size_t, size_t);
 void free(void *);

-- 
2.42.0

I am not familiar with the repo structure and I am guessing I am probably breaking some rules or conventions here. The issue is that __functions_memcpy.h has compiler attributes on memcpy which musl's memcpy does not. Maybe the fix should be to somehow not encounter two declarations in the first place.

SanderVocke commented 1 year ago

Good follow-up steps (IMO, can be done after merging this) would be:

Arshia001 commented 1 year ago

Hi @SanderVocke, thank you for this contribution!

I want to see the output from the pipeline, but I'm not sure how we can get it to run?

SanderVocke commented 1 year ago

@Arshia001: it should run automatically. But for that to work it needs to be enabled in the repository settings.

But on the source of this pull request (SanderVocke/wasix-libc) it does run because I enabled GitHub actions scripts on that fork.

You will see though that the build fails there because of the memcpy issue I mentioned above.

SanderVocke commented 1 year ago

The easiest way to review and tinker with the script is probably to install nektos/act, a tool to run actions scripts locally. Then it should be as easy as just running act in the repo folder with this pr checked out.

Let me know if that works for you!

SanderVocke commented 1 year ago

Or another suggestion: you can just make a pull request back to SanderVocke/wasix-libc. For example with the memcpy patch above, or an alternative fix.

Since actions are enabled there, the script will automatically run on any PR to that fork, and you can see the output directly on GitHub.

If the pipeline succeeds, it also produces a zip artifact with the resulting sysroot for download.

SanderVocke commented 1 year ago

To make things simpler, I created another PR on my fork SanderVocke/wasix-libc, which has Actions enabled, where I applied the memcpy fix above.

The script ran there and you can review the resulting sysroot by navigating to the passed check, then Summary. The sysroot is automatically attached as an artifact there.

https://github.com/SanderVocke/wasix-libc/pull/2

Arshia001 commented 1 year ago

@SanderVocke thanks for the explanation. Build output looks good to me. @theduke I don't seem to have admin access to this repo, we may need to take a look at the repo settings to make sure the action can run here before merging it in.

SanderVocke commented 1 year ago

@Arshia001 I believe that it may not run automatically until a first version has been merged to the main branch, due to how GitHub Actions triggers work.

Arshia001 commented 1 year ago

@SanderVocke turns out the repo was configured to run CI on outside PRs only after approval. It's running now.

Arshia001 commented 1 year ago

... and we hit the memcpy problem as expected. Let me look into fixing that first before merging this PR.

Arshia001 commented 1 year ago

And that's it! Thank you, @SanderVocke. Merging and closing.

theduke commented 1 year ago

@SanderVocke thanks for the contribution!

SanderVocke commented 1 year ago

No worries, thanks for merging!

SanderVocke commented 1 year ago

@theduke @Arshia001 IMO all that's missing now is a release of the built sysroot on the project Releases page. That would make it easier to find. It could be marked as a "pre-release" for now, but just to have something there.

I think this PR changed the README to mention such a release exists, although it doesn't yet.