ScintillaOrg / lexilla

A library of language lexers for use with Scintilla
https://www.scintilla.org/Lexilla.html
Other
185 stars 65 forks source link

`ifdef windir` doesn't work with MSYS2 make #6

Closed lifenjoiner closed 3 years ago

lifenjoiner commented 3 years ago

Testing makefile:

# Work with MinGW make, but doesn't work with MSYS2 make.
ifdef windir
    Value1 = 1
else
    Value1 = 0
endif

# Work with both.
ifneq ("$windir", "")
    Value2 = 1
else
    Value2 = 0
endif

all:
    echo $(Value1)
    echo $(Value2)

Patch: Improve-windir-variable-testing-for-makefile.zip

BTW: Have you considered adding GitHub Action workflows of CI file and then reviewing/merging PRs? GitHub action can automatically build and test when push and/or PR. That will make the whole process easier.

nyamatongwe commented 3 years ago

The second technique doesn't appear to be testing windir as it produces the same results for a non-existent variable with MinGW and CMD:

ifneq ("$notthere", "")
    Value3 = 1
else
    Value3 = 0
endif

all:
    echo [$(Value3)]
    echo [$(notthere)]
>mingw32-make -f test.mak
echo [1]
[1]
echo []
[]
>Exit code: 0

In regards to GitHub Action workflows, I am currently still trying to find ways to improve more basic manual workflows to be as smooth as Hg. Its the overly automatic nature of GitHub that is more of a problem for me as I often want to update the patches to include changes to test cases, history, warning suppressions and references in the commit message.

lifenjoiner commented 3 years ago

I got the reasons: MinGW takes the environment variables the same as CMD, while MSYS2 convert the name (most) to upper case. Tricky. You can observe it by:

ifeq ($(OS),Windows_NT)
    Value1 = 1
else
    Value1 = 0
endif

all:
    echo $(Value1)
    echo [$(windir)]
    echo [$(WINDIR)]

Here is the common solution: Use-variable-OS-to-detect-Windows-for-makefile.zip

Using Hg with GitHub seems rare. Maybe you can try Git, which is more powerful with less conflicts while merging :p

lifenjoiner commented 3 years ago

This issue also exists in Scintilla and SciTE.

nyamatongwe commented 3 years ago

Here is the common solution: Use-variable-OS-to-detect-Windows-for-makefile.zip

This is doing different tests in different places. Its testing whether OS is non-empty in, for example

EXE = $(if $(OS),TestLexers.exe,TestLexers)

and its testing if OS == Windows_NT in

ifeq ($(OS),Windows_NT)

I don't really trust that $(OS) will never be set on Unix. It's too short and attractive.

lifenjoiner commented 3 years ago

I don't really trust that $(OS) will never be set on Unix. It's too short and attractive.

I understand your concern. $(OS) is still the most used, by searching on the net. What about changing all to match Windows_NT? It would be more reliable. And it would be meaningful to be used to cover cross-compiling.

Update: Use-variable-OS-to-detect-Windows-for-makefile_v2.zip

nyamatongwe commented 3 years ago

The if function is different to the if conditional in GNU make as it doesn't allow a comparison.

W = $(if ($(OS),Windows_NT),Windows,notWindows)
M = $(if ($(OS),macOS),macOS,notMacOS)

all:
    @echo [$(OS)]
    @echo [$(W)]
    @echo [$(M)]
>make -f test.mak
[Windows_NT]
[Windows]
[macOS]

It may be possible to use other functions to make the if function work.

The base problem here is that MSYS2 capitalizes some environment variables, probably because very old versions of Windows had inconsistent capitalization. To me, its a bug in MSYS2.

It would be possible to detect MSYS2 and set windir, but that is more difficult to understand:

ifdef MSYSTEM
windir=$(WINDIR)
endif

It would be more obvious to define an extra variable:

ifeq ($(OS),Windows_NT)
    WIN32 = 1
endif

ifdef WIN32
    EXT = .dll
else
    EXT = .so
endif

PYTHON = $(if $(WIN32),pyw,python3)

all:
    @echo [$(OS)]
    @echo [$(windir):$(WINDIR)]
    @echo [$(WIN32)]
    @echo [$(EXT)]
    @echo [$(PYTHON)]

CMD:

C:\u\hg>make -f test.mak
[Windows_NT]
[C:\WINDOWS:]
[1]
[.dll]
[pyw]

MSYS2:

$ make -f test.mak
[Windows_NT]
[:C:WINDOWS]
[1]
[.dll]
[pyw]

Linux:

[neil@fedora merc]$ make -f test.mak
[]
[:]
[]
[.so]
[python3]
lifenjoiner commented 3 years ago

I just corrected my patch by merging them into existing (or split into new) ifeq ($(OS),Windows_NT) or ifneq ($(OS),Windows_NT). Or you can choose the one you like.

Update: Use-variable-OS-to-detect-Windows-for-makefile_v3.zip

nyamatongwe commented 3 years ago

MSYS2 tries to provide a Unix-like environment on Windows. The current makefiles work in that mode, producing a liblexilla.so instead of lexilla.dll. Then TestLexers works:

Neil@Arrakis MSYS /c/u/hg/lexilla/scripts
$ sh RunTest.sh
g++ -DDEBUG -I ../include -I ../../scintilla/include -I ../src -I ../lexlib -fvisibility=hidden --std=c++17 -fPIC -g -Wpedantic -Wall -Wextra   -c Lexilla.cxx -o Lexilla.o
g++ -DDEBUG -I ../include -I ../../scintilla/include -I ../src -I ../lexlib -fvisibility=hidden --std=c++17 -fPIC -g -Wpedantic -Wall -Wextra   -c ../lexlib/Accessor.cxx -o Accessor.o
[Omitted]
g++ -DDEBUG -I ../include -I ../../scintilla/include -I ../src -I ../lexlib -fvisibility=hidden --std=c++17 -fPIC -g -Wpedantic -Wall -Wextra   -c ../lexers/LexYAML.cxx -o LexYAML.o
g++  -shared Lexilla.o Accessor.o CharacterCategory.o CharacterSet.o DefaultLexer.o LexerBase.o LexerModule.o LexerSimple.o PropSetSimple.o StyleContext.o WordList.o LexA68k.o LexAPDL.o LexASY.o LexAU3.o LexAVE.o LexAVS.o LexAbaqus.o LexAda.o LexAsm.o LexAsn1.o LexBaan.o LexBash.o LexBasic.o LexBatch.o LexBibTeX.o LexBullant.o LexCIL.o LexCLW.o LexCOBOL.o LexCPP.o LexCSS.o LexCaml.o LexCmake.o LexCoffeeScript.o LexConf.o LexCrontab.o LexCsound.o LexD.o LexDMAP.o LexDMIS.o LexDataflex.o LexDiff.o LexECL.o LexEDIFACT.o LexEScript.o LexEiffel.o LexErlang.o LexErrorList.o LexFSharp.o LexFlagship.o LexForth.o LexFortran.o LexGAP.o LexGui4Cli.o LexHTML.o LexHaskell.o LexHex.o LexHollywood.o LexIndent.o LexInno.o LexJSON.o LexJulia.o LexKVIrc.o LexKix.o LexLaTeX.o LexLisp.o LexLout.o LexLua.o LexMMIXAL.o LexMPT.o LexMSSQL.o LexMagik.o LexMake.o LexMarkdown.o LexMatlab.o LexMaxima.o LexMetapost.o LexModula.o LexMySQL.o LexNim.o LexNimrod.o LexNsis.o LexNull.o LexOScript.o LexOpal.o LexPB.o LexPLM.o LexPO.o LexPOV.o LexPS.o LexPascal.o LexPerl.o LexPowerPro.o LexPowerShell.o LexProgress.o LexProps.o LexPython.o LexR.o LexRaku.o LexRebol.o LexRegistry.o LexRuby.o LexRust.o LexSAS.o LexSML.o LexSQL.o LexSTTXT.o LexScriptol.o LexSmalltalk.o LexSorcus.o LexSpecman.o LexSpice.o LexStata.o LexTACL.o LexTADS3.o LexTAL.o LexTCL.o LexTCMD.o LexTeX.o LexTxt2tags.o LexVB.o LexVHDL.o LexVerilog.o LexVisualProlog.o LexX12.o LexYAML.o -o ../bin/liblexilla.so
ar rc ../bin/liblexilla.a Lexilla.o Accessor.o CharacterCategory.o CharacterSet.o DefaultLexer.o LexerBase.o LexerModule.o LexerSimple.o PropSetSimple.o StyleContext.o WordList.o LexA68k.o LexAPDL.o LexASY.o LexAU3.o LexAVE.o LexAVS.o LexAbaqus.o LexAda.o LexAsm.o LexAsn1.o LexBaan.o LexBash.o LexBasic.o LexBatch.o LexBibTeX.o LexBullant.o LexCIL.o LexCLW.o LexCOBOL.o LexCPP.o LexCSS.o LexCaml.o LexCmake.o LexCoffeeScript.o LexConf.o LexCrontab.o LexCsound.o LexD.o LexDMAP.o LexDMIS.o LexDataflex.o LexDiff.o LexECL.o LexEDIFACT.o LexEScript.o LexEiffel.o LexErlang.o LexErrorList.o LexFSharp.o LexFlagship.o LexForth.o LexFortran.o LexGAP.o LexGui4Cli.o LexHTML.o LexHaskell.o LexHex.o LexHollywood.o LexIndent.o LexInno.o LexJSON.o LexJulia.o LexKVIrc.o LexKix.o LexLaTeX.o LexLisp.o LexLout.o LexLua.o LexMMIXAL.o LexMPT.o LexMSSQL.o LexMagik.o LexMake.o LexMarkdown.o LexMatlab.o LexMaxima.o LexMetapost.o LexModula.o LexMySQL.o LexNim.o LexNimrod.o LexNsis.o LexNull.o LexOScript.o LexOpal.o LexPB.o LexPLM.o LexPO.o LexPOV.o LexPS.o LexPascal.o LexPerl.o LexPowerPro.o LexPowerShell.o LexProgress.o LexProps.o LexPython.o LexR.o LexRaku.o LexRebol.o LexRegistry.o LexRuby.o LexRust.o LexSAS.o LexSML.o LexSQL.o LexSTTXT.o LexScriptol.o LexSmalltalk.o LexSorcus.o LexSpecman.o LexSpice.o LexStata.o LexTACL.o LexTADS3.o LexTAL.o LexTCL.o LexTCMD.o LexTeX.o LexTxt2tags.o LexVB.o LexVHDL.o LexVerilog.o LexVisualProlog.o LexX12.o LexYAML.o
ranlib ../bin/liblexilla.a
g++ -DDEBUG -I ../../scintilla/include -I ../include -I ../access --std=c++2a -g -Wpedantic -Wall -Wextra   -c TestLexers.cxx -o TestLexers.o
g++ -DDEBUG -I ../../scintilla/include -I ../include -I ../access --std=c++2a -g -Wpedantic -Wall -Wextra   -c TestDocument.cxx -o TestDocument.o
g++ -DDEBUG -I ../../scintilla/include -I ../include -I ../access --std=c++2a -g -Wpedantic -Wall -Wextra   -c ../access/LexillaAccess.cxx -o LexillaAccess.o
g++ --std=c++2a -g -Wpedantic -Wall -Wextra   TestLexers.o TestDocument.o LexillaAccess.o -ldl  -o TestLexers
./TestLexers
Found Lexilla at /c/u/hg/lexilla
Lexing batch/x.bat
[Omitted]
Lexing vb/x.vb

This may be preferable for those people using MSYS as it is more like Unix. Those who want to build lexilla.dll to use with Win32 applications can use Mingw-w64 from a CMD prompt.

lifenjoiner commented 3 years ago

... People used to use MSYS2 to install MinGW-w64 (MSYS/MinGW are outdated), and GNU utils themselves, to migrate *nix programs to be Windows native.

nyamatongwe commented 3 years ago

Inside MSYS2, compiles do not see _WIN32 or WIN32 definitions but there are __MSYS__ and __CYGWIN__. When I saw this I first thought that the #if _WIN32 tests inside Lexilla code should be updated to something like #if _WIN32 || __MSYS__ to produce behaviour that is more Windows-compatible.

However, that seems to be fighting MSYS2 to make it more Windows and less Unix. MSYS2 provides Unix headers like dlfcn.h and the runtime contains dlopen and dlsym so things just work if you approach it as Unix. It will even access .dll files using dlopen to the extent of fixing dlopen("liblexilla.so") to open "liblexilla.dll" if it is present instead.

It may be better to leave building on MSYS2 alone or even make it explicitly choose Unix settings.

#ifdef WIN32
#warning WIN32
#endif
#ifdef _WIN32
#warning _WIN32
#endif
#ifdef __CYGWIN__
#warning __CYGWIN__
#endif
#ifdef __MSYS__
#warning __MSYS__
#endif

CMD:

TestLexers.cxx:306:2: warning: #warning is a GCC extension
  306 | #warning _WIN32
      |  ^~~~~~~
TestLexers.cxx:306:2: warning: #warning _WIN32 [-Wcpp]

MSYS2:

TestLexers.cxx:309:2: warning: #warning is a GCC extension
  309 | #warning __CYGWIN__
      |  ^~~~~~~
TestLexers.cxx:309:2: warning: #warning __CYGWIN__ [-Wcpp]
TestLexers.cxx:312:2: warning: #warning is a GCC extension
  312 | #warning __MSYS__
      |  ^~~~~~~
TestLexers.cxx:312:2: warning: #warning __MSYS__ [-Wcpp]
lifenjoiner commented 3 years ago

Not that complicated. Cygwin => MSYS -> MSYS2 are all targeting Windows. Their shared library extension names are all .dll normally.

Cygwin is aiming to be a middle layer, like Wine on *nix. MSYS/MSYS2 are aiming to be an assistant of MinGW/MinGW-w64. Even libraries built for MSYS/MSYS2, they still have the extension name .dll. The difference of the library is targeting MSYS/MSYS2 or not, is decided by which MinGW/MinGW-w64 that is used. And it is achieved by changing the paths order while MSYS/MSYS2 initializing.

The less Cygwin/MSYS/MSYS2 (cygwin1.dll/msys-1.0.dll/msys-2.0.dll) depended the better.

nyamatongwe commented 3 years ago

I am happy to leave this unchanged since builds work with MSYS2.

nyamatongwe commented 3 years ago

Closing as 'not-a-problem'. We can revisit this later if there are stronger reasons to change.