magiblot / turbo

An experimental text editor based on Scintilla and Turbo Vision.
Other
461 stars 35 forks source link

Compilation error under MinGW and clang/macOS #14

Closed electroly closed 3 years ago

electroly commented 3 years ago

I got the following error building turbo for x86_64-w64-mingw32. I'm cross-compiling from Arch Linux using a fresh install.

In file included from /tmp/turbo/src/tpath.cc:2,
                 from /tmp/turbo/build/CMakeFiles/turbo-ui.dir/Unity/unity_1_cxx.cxx:7:
/usr/x86_64-w64-mingw32/include/tvision/compat/io.h:20:21: error: no include path in which to search for io.h
   20 | #include_next <io.h>
      |                     ^

If I comment out that line in compat/io.h, it builds fine.

electroly commented 3 years ago

I had to apply the following patch to app.cc in order to build on macOS 10.13: https://github.com/electroly/tmbasic/blob/master/build/files/turbo-app.cc.diff

/Users/user910831/Documents/tmbasic/mac/turbo/src/app.cc:444:23: error: call to member function 'assign' is
      ambiguous
        mostRecentDir.assign(TPath::dirname(w->file));
        ~~~~~~~~~~~~~~^~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string:988:19: note:
      candidate function
    basic_string& assign(__self_view __sv) { return assign(__sv.data(), __sv.size()); }
                  ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string:993:19: note:
      candidate function
    basic_string& assign(basic_string&& __str)
                  ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string:990:19: note:
      candidate function
    basic_string& assign(const basic_string& __str) { return *this = __str; }
                  ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string:1007:19: note:
      candidate function not viable: no known conversion from 'TStringView' to 'const
      std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::value_type *'
      (aka 'const char *') for 1st argument
    basic_string& assign(const value_type* __s);
                  ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string:1029:19: note:
      candidate function not viable: no known conversion from 'TStringView' to
      'initializer_list<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>
      >::value_type>' (aka 'initializer_list<char>') for 1st argument
    basic_string& assign(initializer_list<value_type> __il) {return assign(__il.begin(), __il.size());}
                  ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string:1005:19: note:
      candidate function template not viable: requires at least 2 arguments, but 1 was provided
                  assign(const _Tp & __t, size_type __pos, size_type __n=npos);
                  ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string:1017:9: note:
      candidate function template not viable: requires 2 arguments, but 1 was provided
        assign(_InputIterator __first, _InputIterator __last);
        ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string:1026:9: note:
      candidate function template not viable: requires 2 arguments, but 1 was provided
        assign(_ForwardIterator __first, _ForwardIterator __last);
        ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string:1006:19: note:
      candidate function not viable: requires 2 arguments, but 1 was provided
    basic_string& assign(const value_type* __s, size_type __n);
                  ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string:1008:19: note:
      candidate function not viable: requires 2 arguments, but 1 was provided
    basic_string& assign(size_type __n, value_type __c);
                  ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/string:997:19: note:
      candidate function not viable: requires at least 2 arguments, but 1 was provided
    basic_string& assign(const basic_string& __str, size_type __pos, size_type __n=npos);
                  ^
magiblot commented 3 years ago

Regarding io.h: unlike other old-fashioned headers used by Turbo Vision (iostream.h, fstream.h...), io.h is still available in modern Windows build environments because it is part of Microsoft's C Runtime. In my Arch Linux system, it is located in /usr/x86_64-w64-mingw32/include/io.h and is owned by the mingw-w64-headers package. So I suspect this is some sort of configuration issue, or there's a bug in include_next, or I'm overlooking something. I'll have to investigate further.

Regarding string assignment: the basic_string& assign(__self_view __sv) overload provided by your toolchain is not part of the standard, so it can be considered their fault. Maybe that compiler has an option to disable non-standard features, or this issue was fixed in a more recent version, if you are not using the latest one already. Your fix is good anyway. Although I find it curious that operator= does not suffer from the same issue.

electroly commented 3 years ago

Re: io.h. I have an io.h in that location, too.

The build environment is a fresh Arch Linux container pulled and updated today. Here are the MinGW versions and cmake commands I'm using:

mingw-w64-binutils 2.35.1-1
mingw-w64-crt 8.0.0-1
mingw-w64-gcc 10.2.0-2
mingw-w64-headers 8.0.0-1
mingw-w64-winpthreads 8.0.0-1
cmake .. \
    -DCMAKE_PREFIX_PATH=/usr/x86_64-w64-mingw32 \
    -DCMAKE_INSTALL_PREFIX=/usr/x86_64-w64-mingw32 \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_TOOLCHAIN_FILE=/tmp/toolchain-cross-mingw64-linux-x86_64.cmake

That toolchain file is https://github.com/electroly/tmbasic/blob/master/build/files/toolchain-cross-mingw64-linux-x86_64.cmake. Here's a link to the relevant lines in the Dockerfile that sets up the build environment: https://github.com/electroly/tmbasic/blob/master/build/files/Dockerfile.build-win#L234-L278. tvision and tmbasic both build fine without this io.h patch; only turbo has a problem. I use the same cmake command when building tvision. I'm not sure what to make of it.

Re: assign(). The Mac build machine has Xcode 10.1 and clang is Apple LLVM version 10.0.0 (clang-1000.11.45.5). This version of Xcode appears to be three years old. I bet you're right about it being fixed in a later version but I have no way to test. I don't have admin access on the Macincloud build machines, so I'm stuck with what they provide. One of these days I will buy a Mac so I can use the latest Xcode, but it's a tough pill to swallow when Macincloud is $1/hour and all I need is the occasional build. The trials and tribulations of cross-platform development :)

electroly commented 3 years ago

I found this elsewhere in the same Dockerfile specifically relating to io.h, reinforcing the idea that something is wrong with the build environment configuration.

# gtest - why are these declarations from <io.h> needed? gtest header fails when included in tmbasic otherwise
RUN echo "static int _isatty(int) { return 0; }" >> /usr/$ARCH-w64-mingw32/include/gtest/internal/custom/gtest-port.h && \
    echo "static int read(int,void*,unsigned int) { return 0; }" >> /usr/$ARCH-w64-mingw32/include/gtest/internal/custom/gtest-port.h && \
    echo "static int write(int,const void*,unsigned int) { return 0; }" >> /usr/$ARCH-w64-mingw32/include/gtest/internal/custom/gtest-port.h && \
    echo "static int close(int) { return 0; }" >> /usr/$ARCH-w64-mingw32/include/gtest/internal/custom/gtest-port.h

I still haven't isolated the issue but there is surely something wrong on my end here. I tried making a minimal Dockerfile that includes only what is needed to build turbo, and it builds successfully without the io.h patch. I will keep digging but I don't think there's anything for you to fix here; this is for me to figure out.

Let me know if I should leave the issue open for the assign() issue, but I'm happy to keep the patch in my tree to support this outdated toolchain if you'd prefer not to take the patch. I'll close the issue otherwise.

magiblot commented 3 years ago

You can leave it open for the assign() issue, it is in my interest to support older compiler versions when possible.

electroly commented 3 years ago

I narrowed down the io.h issue a bit further. It happens when I install my own build of tvision and remove add_subdirectory(deps/tvision) from turbo's CMakeLists.txt to try to get it to use the system-installed library. I'd like to provide my own builds of fmt, libclipboard, and tvision -- is there a better way than patching CMakeLists.txt?

magiblot commented 3 years ago

This is an important feature that needs to be added: the ability to use installed libraries instead of the submodules. Will look into it.

magiblot commented 3 years ago

Turbo Vision now supports being imported as a system-wide CMake package. Full example:

pushd tvision # Build tvision
cmake . -B build -DCMAKE_INSTALL_PREFIX="$HOME/.local" # Adjust install prefix to your needs.
cmake --build build
cmake --install build
popd
pushd turbo # Build turbo
cmake -B build -DTURBO_USE_SYSTEM_TVISION=ON # Should find the previously installed tvision.
cmake --build build
popd

This works because Turbo Vision now generates a CMake config-file package which Turbo can detect with find_package(tvision CONFIG REQUIRED). The advantage of this over just linking libtvision.a is that Turbo automatically gets the right compilation flags required by Turbo Vision (-lncursesw, etc.).

I see that in your patch you also avoided recompiling fmt and libclipboard. I guess I'll have to allow finding these in the system too.

Cheers.

electroly commented 3 years ago

I've updated my tvision and turbo deps and no longer need the app.cc patch on Mac -- thanks! Looks like I still need the io.h patch on mingw. I haven't had a chance to look deeper into that issue. The ability to use the system fmt and libclipboard would be appreciated, as I'm still patching for that.

magiblot commented 3 years ago

The ability to use the system fmt and libclipboard would be appreciated, as I'm still patching for that.

Implemented in a637236c04b5c4b21387473b7dae1afecb08bd22. Add -DTURBO_USE_SYSTEM_DEPS=ON next to -DTURBO_USE_SYSTEM_TVISION=ON.

magiblot commented 3 years ago

With the changes above, cross-compilation works for me after modifying the toolchain file like this:

+set(CMAKE_FIND_ROOT_PATH /usr/${COMPILER_PREFIX})
 set(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM NEVER)
 set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY ONLY)
 set(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE ONLY)
+set(CMAKE_FIND_ROOT_PATH_MODE_PACKAGE ONLY)

If I don't set CMAKE_FIND_ROOT_PATH, CMake will find things outside /usr/x86_64-w64-mingw32.

electroly commented 3 years ago

I'm still looking into the io.h issue. I've updated to the latest tvision/turbo and have applied -DTURBO_USE_SYSTEM_DEPS=ON and your suggested toolchain file updates. It works great, but the io.h patch still seems to be required. Under the assumption that my build environment is wrong somehow, I built mingw-w64 from source with an Ubuntu base instead of using an Arch base + prebuilt Arch mingw-w64 package, and I get exactly the same io.h issue despite the totally different environment. I'm working on preparing a minimal example that only includes mingw-w64 and the minimum deps required to build turbo.

magiblot commented 3 years ago

I found why #include_next fails. It is caused by tvision/compat/io.h being located in the same path as io.h. So this is not a configuration issue but a mistake of mine. This should be fixed by (40edc00d8b9ddb4c12d100f22831d78615920a6d / https://github.com/magiblot/tvision/commit/ded9eba5f873f61976be737951a74d68da8dc942). Sorry for the inconvenience!

electroly commented 3 years ago

Nice! I've picked up the latest commit and removed my io.h patch. Working great. Closing this issue as you've addressed everything -- thank you!

magiblot commented 3 years ago

You are welcome. Thanks for reporting!