bakpakin / Fennel

Lua Lisp Language
https://fennel-lang.org
MIT License
2.48k stars 126 forks source link

Update Windows targets in the makefile. Resolves #487 #488

Closed noncom closed 2 months ago

noncom commented 2 months ago

A minor update to the build script for Windows. Now it defaults to making a 64bit build with one of the recent MinGW-w64 toolchains.

I added the explicit -I$(LUA_INCLUDE_DIR) arg to the compile args. It might work on Linux without it, but on Windows it doesn't because the notion of globally installed header files is more complicated on Windows, and is better to be considered non-existent in most cases for non-system includes. But since it anyway builds against a submodule Lua clone, maybe it's ok for Linux too?

I tested the build on Windows 10 64bit, and on Ubuntu 24.04, everything seems to be ok. For Linux the original instruction doesn't say that Lua must be installed in the system though it might seem like a somewhat obvious requirement.

But just as important, an additional user instruction could help quite a lot by hinting at some things. For example I would propose something along these lines:

Building on Windows

By default, Fennel for Windows builds with Lua support. If you need Fennel to support LuaJit, replace all LUA variables with their LUAJIT counterparts in the fennel.exe and $(BIN_LUA_DIR)/src/liblua-mingw.a targets in the makefile.

technomancy commented 2 months ago

For Linux the original instruction doesn't say that Lua must be installed in the system though it might seem like a somewhat obvious requirement.

No, it's not required that Lua be installed already. Under "Building Fennel from source" in the readme:

If you don't have Lua already installed on your system, you can run make fennel-bin LUA=lua/src/lua instead to build a standalone binary that has its own internal version of Lua.

However, this currently doesn't work unless the submodule has already been initialized, so I've updated the makefile to do that automatically.

I think the instructions for how to install gcc and make on Windows are probably a little too verbose to include in the readme, but maybe we could link to a wiki page for that or something? I'm not sure where the best place for it would be.

noncom commented 2 months ago

Got it! Yeah, there is some new info for me :D I'll make corrections.

noncom commented 2 months ago

Just tested the rule fennel-bin, it doesn't work on my system at least:

D:\work\Fennel>make fennel-bin
make -C D:/work/Fennel/lua
Guessing MSYS_NT-10.0-19045
make[3]: *** No rule to make target 'MSYS_NT-10.0-19045'.  Stop.
make[2]: *** [Makefile:99: guess] Error 2
make[1]: *** [Makefile:55: guess] Error 2
make: *** [Makefile:110: D:/work/Fennel/lua/src/lua] Error 2

I'm not a Make pro, but after some googling I've found that the situation with automatically determining the platform is quite pitiful, and there's no solution that works in 100% cases.

Maybe if this would be limited to GNU Make only, then threre's at least a known conglamerate of hacks that people say works: https://stackoverflow.com/questions/714100/os-detecting-makefile, but I think that maybe just manually selecting the rule is simpler.

As far as I understand, Guessing MSYS_NT-10.0-19045 comes from gcc probably? It's accurate, but it will be different for another Windows version and build since it includes these numbers.

noncom commented 2 months ago

This is already set as a CC opt by --compile-binary

Indeed, and this actually works. Then I'm not sure why it was failing to find the includes before. Running now the build with FENNEL_DEBUG=true shows that the option is correctly there. So removing that change now.

maybe WINDOWS_CC which could be overridden separately; like maybe it could default to gcc or $(CC) on Windows but use its current i686-w64-mingw32-gcc value on Linux?

Yeah, good idea. Then it's a variable now, and defaults to gcc as something Windows-user-oriented. By the way maybe you could also update the toolchain you're using to make x64 builds, the names are like this: https://stackoverflow.com/a/59805718 but idk what toolchain you're using. As one of the other answers there says, note that MinGW and MinGW-w64 are different projects.


For a shorter MinGW installation instruction -- I tried searching some, but they're all either very lengthy or obscure, or from a different flavor that have custom compiler naming conventions šŸ¤¦ā€ā™‚ļø. So maybe just omitting the trivial stuff, and simply pointing to their official page without additional words would be enough.

But let me ask you one thing first -- in your Windows-targeted setup are you using the toolchain from MSYS2 like desribed here? Because if so, then that's maybe where from the gcc naming convention is coming from in your setup, and we might just recommend its counterpart on Windows instead of MinGW-w64, though I rearely see MSYS2 recommended or used in comparison to MinGW-w64.

technomancy commented 2 months ago
D:\work\Fennel>make fennel-bin
make -C D:/work/Fennel/lua
Guessing MSYS_NT-10.0-19045
make[3]: *** No rule to make target 'MSYS_NT-10.0-19045'.  Stop.
make[2]: *** [Makefile:99: guess] Error 2
make[1]: *** [Makefile:55: guess] Error 2
make: *** [Makefile:110: D:/work/Fennel/lua/src/lua] Error 2

I'm not a Make pro, but after some googling I've found that the situation with automatically determining the platform is quite pitiful, and there's no solution that works in 100% cases.

I think it's just that Lua's own makefile isn't right; when running make fennel.exe we have Fennel's makefile run Lua's build with make mingw specifically, and when running make fennel-bin it just lets Lua's build pick the default target, which gets it wrong. (Hilariously wrong, even; it guesses at a target that doesn't actually exist!) So maybe we can't fix that from our side to do the right thing automatically since the root problem is in Lua; too bad!

Maybe if this would be limited to GNU Make only

Oh, well; that is the case. Fennel's makefile specifically does not attempt to be posix-compatible, because the posix standard for make leaves out a bunch of critically-important stuff, and the workarounds are really awful. So we specifically document a requirement of GNU Make and already depend on its extensions.

By the way maybe you could also update the toolchain you're using to make x64 builds

Yeah, that looks pretty straightforward. I'll switch to the 64-bit mingw gcc.

in your Windows-targeted setup are you using the toolchain from MSYS2 like desribed here? Because if so, then that's maybe where from the gcc naming convention is coming from in your setup

Did you link to the right section of that page? I am in fact using a setup similar to the section you linked to titled "Using Mingw-w64 to build Windows programs on other systems", that is, installing it using those apt packages (except for it currently being 32-bit) but that section doesn't say anything about MSYS2. I don't actually know what MSYS2 is. I just followed some advice I found on a mailing list many years ago for cross-compiling with mingw and a couple Windows users told me it worked. =)

technomancy commented 2 months ago

I've pushed out a change to use CC=x86_64-w64-mingw32-gcc so you may want to rebase on main.

noncom commented 2 months ago

I updated the branch. Keeping there the WINDOWS_CC constant for now, with some comments for comfort. Literally now the only change in the makefile is the gcc program name, and so now everything boils down to just setting a couple of environment variables :D

Now there's also one thing/problem: I'm actually looking into making this work somewhow: https://github.com/love2d/love/issues/2103#issuecomment-2366771496. Theoretically it might influence some considerations on compiling Fennel because maybe the fact that it doesn't work comes from me missing something about the build. Would be good to unerstand if that's the case or not. Let's see what comes.

Did you link to the right section of that page? I am in fact using a setup similar to the section you linked to titled "Using Mingw-w64 to build Windows programs on other systems", that is, installing it using those apt packages (except for it currently being 32-bit) but that section doesn't say anything about MSYS2. I don't actually know what MSYS2 is.

Yeah, it's the correct link :) It mentions MSYS2 above, but not in the particular section where the link leads to. But yeah, all this is cryptic enough, idk why so much stuff exists.

I just tried to clarify for myself, so, apparently, yes this clarifies why gcc in your setup has this naming pattern. I just rarely see MSYS2 recommended vs MinGW-w64, so haven't even seen it before, but probably this is a good Linux->Windows option that's supported by Microsoft. Ok!

technomancy commented 2 months ago

Sorry, but I don't think this really helps, because WINDOWS_CC in the patch above doesn't behave meaningfully different from CC to begin with.

This will break when it comes time to release because make upload will not be able to build fennel.exe when it comes time to make a release. It would need to default to a working WINDOWS_CC during release time for this to work.

technomancy commented 2 months ago

What about this instead?

--- a/Makefile
+++ b/Makefile
@@ -113,9 +113,8 @@ $(NATIVE_LUAJIT_LIB): $(LUAJIT_INCLUDE_DIR)
    $(MAKE) -C $(BIN_LUAJIT_DIR) BUILDMODE=static

 # TODO: update setup.md to point to new filenames on next release
-# it's unclear why this CC has "w32" in the name, but it builds 64-bit output
 fennel.exe: src/launcher.fnl fennel $(LUA_INCLUDE_DIR)/liblua-mingw.a
-   $(COMPILE_ARGS) CC=x86_64-w64-mingw32-gcc ./fennel --no-compiler-sandbox \
+   $(COMPILE_ARGS) ./fennel --no-compiler-sandbox \
        --compile-binary $< fennel-bin \
        $(LUA_INCLUDE_DIR)/liblua-mingw.a $(LUA_INCLUDE_DIR)
    mv fennel-bin.exe $@
@@ -170,7 +169,8 @@ test-builds: fennel test/faith.lua
    ./fennel --metadata --eval "(require :test.init)"
    $(MAKE) install PREFIX=/tmp/opt

-upload: fennel fennel.lua fennel-bin fennel.exe
+upload: fennel fennel.lua fennel-bin
+   $(MAKE) fennel.exe CC=x86_64-w64-mingw32-gcc
    mkdir -p downloads/
    mv fennel downloads/fennel-$(VERSION)
    mv fennel.lua downloads/fennel-$(VERSION).lua

This way we just use CC like normal and the mingw override is limited to just when the release is being prepared.

technomancy commented 2 months ago

I've pushed this out to main now; let me know how it works for you.

noncom commented 2 months ago

Ah yeah, the meaning of this PR has already fully evaporated. The only meaningful part that remains is the instruction about setting the PWD, I think.

My original confusion was from the fact that I was expecting the Windows build to work on Windows, and when it didn't, I thought that it was referring to some old 32bit toolchain, because many of them exist. But it turns out to be the actual config for Linux->Windows builds, which must be the default, so there's not much to do.

I guess that keeping the makefile simple, and just implying that the user must change the compiler to gcc of MinGW-w64, or mentioning this in an instruction, would be the best tbh. It's easier to explain to a human to replace a word than to make the C build system recognize the OS and choose the appropriate C compiler, welcome to the age of automation :D

Also, if you provide 64bit builds with latest Lua and LuaJIT, realistically I think that 99% of Windows users won't even ever try to build Fennel.


As for the current master, I just cloned and tested. It compiles Lua, but then:

FENNEL_PATH=src/?.fnl FENNEL_MACRO_PATH=src/?.fnl CC_OPTS=-static ./fennel --no-compiler-sandbox \
        --compile-binary src/launcher.fnl fennel-bin \
        D:/work/Fennel/lua/src/liblua-mingw.a D:/work/Fennel/lua/src
'cc' is not recognized as an internal or external command,
operable program or batch file.
Compiling with  cc -Os src/launcher.fnl_binary.c  D:/work/Fennel/lua/src/liblua-mingw.a -rdynamic -lm -ldl -o fennel-bin -I D:/work/Fennel/lua/src -static
'cc' is not recognized as an internal or external command,
operable program or batch file.
failed: cc -Os src/launcher.fnl_binary.c  D:/work/Fennel/lua/src/liblua-mingw.a -rdynamic -lm -ldl -o fennel-bin -I D:/work/Fennel/lua/src -static
make: *** [Makefile:118: fennel.exe] Error 1

Idk where it's taking cc from, but yeah, it's not something that exists. The closest is gcc which is the compiler name that's usually used in MinGW-w64 on Windows.

In the last change you removed CC=<compiler> from the fennel.exe target completely. Idk if this was intentional, but then this gives no info what C compiler to use. If I just add CC=gcc to line 118 then it compiles well and produces the exe.

technomancy commented 2 months ago

In the last change you removed CC= from the fennel.exe target completely. Idk if this was intentional, but then this gives no info what C compiler to use.

The intent was to remove it so that the Makefile doesn't interfere and the system-level defaults are used instead.

Idk where it's taking cc from, but yeah, it's not something that exists.

This is the standard way of referring to the system's C compiler; when you install GCC it should include cc as an alias for gcc. I guess mingw doesn't do that the way it's supposed to?

We could in theory set it to gcc instead when building fennel.exe. But is gcc really a good value for the default C compiler on Windows? Isn't it likely Windows users will have different compilers they'd want to use instead?

The only meaningful part that remains is the instruction about setting the PWD, I think.

Looking more closely at it, this may not be necessary; we could just use a relative path there.

noncom commented 2 months ago

This is the standard way of referring to the system's C compiler; when you install GCC it should include cc as an alias for gcc. I guess mingw doesn't do that the way it's supposed to?

Yeah, it doesn't setup cc as an alias. I don't think that setting aliases happens on Windows at all. There are some cases where installation managers like Chocolatey, NPM, etc do something like this when they link "the current version of ", but that's different, and the name never changes like gcc -> cc.

We could in theory set it to gcc instead when building fennel.exe. But is gcc really a good value for the default C compiler on Windows? Isn't it likely Windows users will have different compilers they'd want to use instead?

Well, it can be gcc, msvc (or cl) or clang. Not sure maybe there are some more exotic ones. But gcc is just the most common one:

Looking more closely at it, this may not be necessary; we could just use a relative path there.

Yeah maybe! I just tried to set it to ., and it worked fine.

technomancy commented 2 months ago

Defaulting to gcc within binary.fnl is definitely not the right choice. Defaulting CC ?= gcc in the top level of the makefile is also definitely wrong. But it might be viable to set the default just within the target for fennel.exe alone.

On the other hand, perhaps it would be best to adjust the error message within Fennel for when it fails to tell the user to set CC to whichever compiler they intended to use, because then it would benefit all uses of --compile-binary, not just when compiling fennel.exe.

noncom commented 2 months ago

Actually that's a very good idea -- to let Fennel handle it, and include the hint into the --compile-binary mode! šŸ˜„

Also tbh anything takes the world further from Make is good imho. Any decent scripting language, including Fennel itself, would do better. Well, at least xmake exists for Lua.

technomancy commented 2 months ago

Also tbh anything takes the world further from Make is good imho

As far as I can tell, none of the problems in this thread have been caused by Make; it's just from trying to compile C code in a portable way. All the make targets used to compile Fennel to Lua work seem to work perfectly fine. The tragic problem is that we don't have a way to get binaries without first going thru C, at least not yet. But we can dream of a better world. =)

I've pushed a fix for $PWD and the hint for setting $CC.

noncom commented 2 months ago

Tested it! Works like a charm, the warning is there, and setting CC works to specify the compiler. This is better than anything I could imagine initially!

Many thanks! šŸ˜„