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

add github CI #125

Closed
wants to merge 5 commits into from
Closed

add github CI #125

wants to merge 5 commits into from

Conversation

neheb
Copy link
Contributor

@neheb neheb commented May 11, 2023

barebones CI.

The MSYS2 failures are the ones to look at.

the printf issue needs a fix like https://github.com/Exiv2/exiv2/blob/main/src/image_int.hpp#L15

@neheb
Copy link
Contributor Author

neheb commented May 11, 2023

actions needs enabling looks like

@gstrauss

@glensc
Copy link
Contributor

glensc commented May 12, 2023

actions are visible in your repo until it's merged:

@gstrauss
Copy link
Member

Thank you for putting this all together!

Looks like there may be some items that need fixing to be able to pass on all platforms.

  • FreeBSD and OpenBSD CI images do not have /bin/bash, so scripts should probably use /bin/sh instead.
  • I'll have to experiment more on MacOS for why the image is looking for/not finding .so modules vs .dylib.
    I know that lighttpd builds and runs on MacOS (e.g. lighttpd: Update to 1.4.70 macports/macports-ports#18620)
  • On Windows, pcre2 needs to be installed or else lighttpd needs to be built --without-pcre2.
    Also, I have not built or tested 32-bit lighttpd on Windows. Besides, I think that testing 32-bit builds on Windows in the CI would be a waste of resources. On the other hand, building 32-bit on a unix-like platform is much more useful (e.g. openwrt).
  • ...

I am very interested in ironing things out to get this committed, though will not have some solid, uninterrupted time to spend on this for another week or two.

@gstrauss
Copy link
Member

FYI, lighttpd src/first.h already contains a mess for MINGW

#if defined(__MINGW32__) || defined(__MINGW64__)
#define _POSIX
#define __USE_MINGW_ALARM 1
/* https://sourceforge.net/p/mingw-w64/wiki2/gnu%20printf/ */
#define __USE_MINGW_ANSI_STDIO 1
/*#include <stdio.h>*/
#undef __printf__
/*#define __printf__ __MINGW_PRINTF_FORMAT*/
#define __printf__ __gnu_printf__
/* override pid_t before <sys/types.h> is included; modified from:
 * /usr/x86_64-w64-mingw32/sys-root/mingw/include/sys/types.h */
#ifndef _PID_T_
#define _PID_T_
#ifndef _WIN64
typedef int _pid_t;
#else
typedef long long _pid_t;
#endif
typedef int pid_t;
#endif
#endif /* __MINGW32__ || __MINGW64__ */

and the above is apparently what leads to
../src/log.h:24:1: warning: '__format__' attribute argument not supported: gnu_printf [-Wignored-attributes]

I had experimented with a few things (back in 2020ish(?))
#define __printf__ __gnu_printf__ was what I settled on.
Ref: https://sourceforge.net/p/mingw-w64/wiki2/gnu%20printf/
Ref: commit d091557 and commit message

[multiple] _WIN32 misc compat

Use #define __USE_MINGW_ANSI_STDIO 1
to use __mingw_printf() to support %lld %zu and other C11 stdio support
msvcrt.dll only supports C89, and has not been linked into dlls by MS
since VC++ 6

I do not know what the "right" answer is, but the above is context around what is currently in the code.

@neheb
Copy link
Contributor Author

neheb commented May 12, 2023

__USE_MINGW_ANSI_STDIO should only be defined for non UCRT.

@gstrauss
Copy link
Member

Is #include <stdio.h> necessary in src/first.h?

@gstrauss
Copy link
Member

gstrauss commented May 12, 2023

Should we exclude 'x86' and 'MINGW32' and 'CLANG32' ?

  VisualStudio:
    runs-on: windows-latest
    name: MSVC-${{matrix.deps}}-${{matrix.platform}}
    strategy:
      matrix:
        deps: ['forcefallback', 'default']
        platform: ['x64', 'x86']
...
  MSYS2:
    runs-on: windows-latest
    name: MSYS2-${{matrix.platform}}-deps=${{matrix.deps}}
    strategy:
      matrix:
        deps: ['enabled', 'disabled']
        platform: ['MINGW32', 'MINGW64', 'UCRT64', 'CLANG32', 'CLANG64']

Some untested patches to address some 32-bit warnings on Windows.
There are a couple more to do but this should address most of the warnings.

--- a/src/fdevent_win32.c
+++ b/src/fdevent_win32.c
@@ -645,7 +645,7 @@ pid_t fdevent_createprocess (char *argv[], char *envp[], intptr_t fdin, intptr_t
      * Programmatically controlling which handles are inherited by new processes
      * in Win32: https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873
      */
-    size_t sz = 0;
+    SIZE_T sz = 0;
     InitializeProcThreadAttributeList(NULL, 1, 0, &sz);
     /* GetLastError() == ERROR_INSUFFICIENT_BUFFER */
     LPPROC_THREAD_ATTRIBUTE_LIST attrlist = info.lpAttributeList = malloc(sz);
--- a/src/plugin.c
+++ b/src/plugin.c
@@ -152,7 +152,11 @@ int plugins_load(server *srv) {
                       srv->srvconf.modules->used, sizeof(plugin *));
 
        buffer * const tb = srv->tmp_buf;
+  #ifdef _WIN32
+       int (WINAPI *init)(plugin *pl);
+  #else
        int (*init)(plugin *pl);
+  #endif
 
        for (uint32_t i = 0; i < srv->srvconf.modules->used; ++i) {
                const buffer * const module = &((data_string *)srv->srvconf.modules->data[i])->value;
--- a/src/server_win32.c
+++ b/src/server_win32.c
@@ -239,6 +239,7 @@ static void lighttpd_ServiceMain (DWORD dwNumServicesArgs, LPSTR *lpServiceArgVe
 {
     /* service thread; not main(); params are not program startup argc, argv */
     hStatus = RegisterServiceCtrlHandlerA("lighttpd",
+                                          (LPHANDLER_FUNCTION)
                                           lighttpd_ServiceCtrlHandler);
     if (!hStatus) return; /*(unexpected; can not continue)*/
     lighttpd_ServiceStatus(SERVICE_START_PENDING, NO_ERROR, 1000);
@@ -281,7 +282,7 @@ static void lighttpd_ServiceCtrlDispatcher (int argc, char ** argv)
     svc_main_argv = argv;
 
     static const SERVICE_TABLE_ENTRYA lighttpd_ServiceDispatchTable[] = {
-      { "lighttpd", lighttpd_ServiceMain },
+      { "lighttpd", (LPSERVICE_MAIN_FUNCTIONA)lighttpd_ServiceMain },
       { NULL, NULL }
     };
     UINT rc = StartServiceCtrlDispatcherA(lighttpd_ServiceDispatchTable)
--- a/src/sys-time.h
+++ b/src/sys-time.h
@@ -125,8 +125,7 @@ gmtime_y2038_kludge32 (unix_time64_t t, struct tm *result)
          * Thu, 01 Jan 1970 00:00:00 GMT)
          *(returning NULL not expected by lighttpd and might crash)*/
         tt = 0;
-        gmtime_r(&tt, result);
-        return result;
+        return gmtime_r(&tt, result);
     }
 }
 
@@ -161,8 +160,7 @@ localtime_y2038_kludge32 (unix_time64_t t, struct tm *result)
          * Thu, 01 Jan 1970 00:00:00 GMT)
          *(returning NULL not expected by lighttpd and might crash)*/
         tt = 0;
-        localtime_r(&tt, result);
-        return result;
+        return localtime_r(&tt, result);
     }
 }
 

@neheb
Copy link
Contributor Author

neheb commented May 13, 2023

What’s wrong with 32 bit?

@gstrauss
Copy link
Member

gstrauss commented May 13, 2023

What’s wrong with 32 bit?

My question more is "what is useful about 32-bit software on Windows?" (edit: ... in 2023)

@neheb
Copy link
Contributor Author

neheb commented May 13, 2023

That's not the issue. The issue is that CI offers no good way to test 32-bit platforms besides windows.

Could try cross compiling but that's more work than just testing a 32-bit platform.

Anyway, I'll update this PR with fixes and whatnot when I get some time. I'm out of the country right now.

@gstrauss
Copy link
Member

gstrauss commented May 13, 2023

You will likely run into loads of _WIN32-$@#%@#^ Windows-specific "compatibility" issues if you attempt building lighttpd 32-bit on Windows. I will not prioritize fixing any Windows-specific 32-bit build issues.

(64-bit lighttpd on Windows is separate and is desirable to be part of CI.)

That's not the issue. The issue is that CI offers no good way to test 32-bit platforms besides windows.

This seems like a much better issue to spend time addressing.

Could try cross compiling but that's more work than just testing a 32-bit platform.

In one of the Ubuntu containers, can you add packages for gcc 32-bit compiler toolchain and/or clang 32-bit compiler toolchain? Prebuilt packages should be available for the compiler toolchains. Once the toolchain is installed, a slight modification of the build setup invocation (to specify the target arch) should be all that is needed to build a minimal lighttpd executable, though admittedly without any dependencies unless packages for 32-bit libs are also available to be installed.

@neheb
Copy link
Contributor Author

neheb commented May 13, 2023

AFAIK a cross file is required to do so. It’s much less work to just test 32-bit Windows.

That being said, 64-bit Windows doesn’t build currently either.

@gstrauss
Copy link
Member

gstrauss commented May 13, 2023

That being said, 64-bit Windows doesn’t build currently either.

lighttpd on Windows builds for me (manually) under cygwin mingw using autoconf, and under MSVC using cmake.
I have not tested meson on Windows.

FYI, I pushed some fixes to lighttpd master branch to clean up some warnings I saw in the CI logs for 32-bit Windows builds. I hope that helps.

I won't be able to accept this PR with platforms on which CI does not pass, so we might consider putting the CI in place for the platforms that currently work, and add additional platforms in new PRs.

@neheb
Copy link
Contributor Author

neheb commented May 13, 2023

Hmm I thought I made this a draft. It's nowhere near ready. I mostly copied it from exiv2's CI.

I'll rebased on master.

@stbuehler stbuehler marked this pull request as draft May 13, 2023 12:53
@neheb neheb force-pushed the ci branch 3 times, most recently from bff423d to 39dbee2 Compare May 13, 2023 13:18
@gstrauss
Copy link
Member

Hmm I thought I made this a draft. It's nowhere near ready.

Nice. Ok. I had overlooked that. No rush.

I generally try to encourage contributors by making some forward progress and setting expectations when I'll have time to review, troubleshoot, and improve.

lighttpd-git pushed a commit that referenced this pull request Jan 8, 2024
(thx neheb)

initial take by neheb
reduced and updated to a mostly-working set of OS in this commit

x-ref:
  "add github CI"
  #125
@gstrauss
Copy link
Member

gstrauss commented Jan 8, 2024

@neheb thank you for the effort in getting this started.

I made some minor changes for portability and updated the macOS meson build to use .so instead of .dylib, and now the unix-like platforms build and most platforms run and pass all the tests. (TODO: For some reason, NetBSD and OpenBSD lighttpd test run fails to find the test lighttpd.conf, even though FreeBSD and DragonflyBSD run the exact same commands in the .yaml and succeed in running the tests.)

Those changes have been pushed to lighttpd git master. If you're interested in working further on this PR, it will need to be rebased and merge conflicts resolved, due to some of the updates that I made.

When I find more time and am feeling masochistic, I may continue trying to get the github CI to work for native Windows and MSYS builds, though I will probably do so using autoconf or CMake instead of meson, since I know that autoconf works under cygwin to cross-compile for native Windows, and CMake works under Visual Studio GUI. To get that working with meson would probably require looking at lighttpd src/CMakeLists.txt and copying some of the properties on the lighttpd executable to export all symbols on Windows for lighttpd shared modules.

@gstrauss
Copy link
Member

gstrauss commented Jan 8, 2024

@neheb: curious: why did you name the matrix 'deps' when it is used for meson -Dauto_features={enabled,disabled,auto}. Would it be better-named 'auto_features'?

@neheb
Copy link
Contributor Author

neheb commented Jan 8, 2024

Forgot about this PR. Features are typically paired with extra dependencies. I’ll rebase when I get time.

neheb added 3 commits January 7, 2024 20:55
Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
Signed-off-by: Rosen Penev <[email protected]>
@neheb neheb force-pushed the ci branch 3 times, most recently from deda7e7 to 7e43fde Compare January 8, 2024 05:18
@neheb neheb force-pushed the ci branch 2 times, most recently from d6190f5 to 1d50e3c Compare January 8, 2024 05:20
neheb added 2 commits January 7, 2024 21:24
latest compilers are a lot more strict. Simplifies workflow file.

Signed-off-by: Rosen Penev <[email protected]>
Remove pointless and redundant Name entries.

Remove auto features. They require a value setting to be absent in
meson_options.txt.

Signed-off-by: Rosen Penev <[email protected]>
@neheb
Copy link
Contributor Author

neheb commented Jan 8, 2024

@gstrauss last two commits can go in.

@gstrauss
Copy link
Member

gstrauss commented Jan 9, 2024

@gstrauss last two commits can go in.

Thank you. I cherry-picked those two changes.

I made some additional changes, added some more platforms to the build, and renamed meson.yml to pr.yml.

The CI is now passing on all platforms, including 32-bit and 64-bit platforms.

On some platforms, and for different reasons, I had to use cmake instead of meson, which is why I renamed meson.yml to pr.yml.

While I do not have further time at the moment to work on this, future work to expand build coverage might install additional packages and enable additional lighttpd modules which rely on additional underlying libraries, e.g. TLS libs, database access libs, and more.

@gstrauss
Copy link
Member

gstrauss commented Jan 12, 2024

@neheb: turns out (as I expected) that there are good options to run 32-bit VMs for CI.
https://github.com/marketplace/actions/setup-alpine-linux-environment

I have added the Alpine VMs to lighttpd github CI to test lighttpd on various 32-bit architectures, and I have removed the MSYS2 32-bit builds. https://github.com/lighttpd/lighttpd1.4/blob/master/.github/workflows/pr.yml

@neheb
Copy link
Contributor Author

neheb commented Jan 12, 2024

Well, for 32-bit you could also cross compile.

@gstrauss
Copy link
Member

Closing this as the CI is building 32-bit and 64-bit on Linux, and is building 64-bit on Cygwin, MSYS, and other platforms.

Thanks for the initial inspiration!

@gstrauss gstrauss closed this Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants