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

CMake support for Qt5 and Qt6 #100

Closed
wants to merge 112 commits into from
Closed

Conversation

diegoiast
Copy link
Contributor

@diegoiast diegoiast commented Apr 16, 2022

This PR brings CMake support for the Qt5 build of NotepadNext.

To build use:

cmake -S . -B cbuild -G Ninja
cmake --build cbuild

Benefits:

  1. QMake is basically EOL
  2. CMake is supported on VS out of the box.
  3. CMake has ninja generators - which on my machine speed up compilation by almost a factor of 2.
  4. All upstreams are now handled using CPM
  5. This makes the build system more modern, and hopefully - easier to manage with the rest of the C++ community.

TODO:

  • Fix clang build on Linux (fails on my setup)
  • Fix libraries to have proper SO name
  • Add CI/CD setup to CMake on Windows/Linux/OSX
  • Fix Qt5/6 building on Windows
  • Add Qt6 support
  • Setup install CMake targets (needed for package building)
  • AppImage generation (is broken ... should be fixed again)
  • Windows installer generation (see error bellow)
  • App bundle generation
  • Convert the build.yml back to ninja
  • Add support for translations
  • Hooks to upload to Windows store(s?)
  • Hooks to upload to linux stores (flatpack and what not)
  • Multi arch support for OSX

Nice to have:

  • Start asking upstreams to accept patches, and remove my local forks. (WIP: see CMake support editorconfig/editorconfig-core-qt#2)
  • Try to fix as much of the TODOs and FIXME inside each module (lexzilla has generated files, which are a pain - I am unhappy about how this went - just an example) - this is not blocking for this PR.
  • Fix AppImages to have better naming (qt version, cmake, app version etc).
  • Instead of upstreaming build systems, we can use local build files, like we did on QMake, unless some code modifications are needed. Seems like LUA is a good target for this.

Status so far:
I am able to build a windows ZIP. From this - I should create the EXE using NSI. However - NSI dies, since it looks for the EXE version in order to create the version for the installer. I traced this down to the Windows RC compiler. QMake passes some arguments to it - and the versions are "preprocessor defines". I do not want to generate the RC on configure time, this seems like a hack to me, I want to do what QMake does. When this is done - IMHO, the installer for windows will be ready.

Installer for OSX? I have no idea whats borked there. I hope someone with a Mac can help me.

Linux should be ready.

@diegoiast diegoiast marked this pull request as draft April 16, 2022 19:52
@dail8859
Copy link
Owner

Thanks for the PR!

I am very apprehensive about switching to cmake at this time. I personally have never had any good experiences with cmake...again just my personal opinion.

I know its a widely-used, mature build system so I'm willing to keep an open mind. I know in the past couple of years QtCreator has had better cmake support (and Qt moving to cmake in general)

So I think the plan for now can be to keep this PR open and slowly work on maturing cmake support to see if it is an adequate replacement for everything qmake does today. I worry all upstreams will not be willing to support cmake (could this project implement its own cmake files outside of those streams? similar to what is done with qmake and Lua currently?) Even then I don't foresee moving to cmake really soon, just because of some of the macOS and flatpak distributions might need some tweaking. But if it matures nicely and proves it can take qmake's place then I may be willing to ditch qmake.

@diegoiast
Copy link
Contributor Author

I agree. I think the best way is to maintain both build systems - until you got comfortable with CMake. For you - you just need to open this branch and open VisualStudio. The rest will be similar to you - the workflow will be the same. I can find time to setup Qt6 on Windows - but it will take time. You can test Qt5/Windows now. (and Qt5/OSX, whoever tests this). It "should work".

Regarding upstreams: I have only 4 forks (you cannot add an out of source CMake, at least not trivially, yet):

  1. QSimpleUpdater - I will contact the original upstream and try to merge.
  2. editorconfig-core-qt - I will contact the original upstream and try to merge.
  3. scintilla-code - this is a huge pain, it will take several month until upstream merged this in (I need to fix 3 other platforms, not fun). They are HG only, and in source forge... so a github mirror will be necessary (until I get conan working).
  4. lexilla - this is relatively easy. But I am unsure if upstream wants this.

The first two are not a problem. The problem is scintilla's upstream. Even if they do not accept the new build system. as I have not modified their sources - I can easily merge from them back. IMHO - its a win-win scenario.

@dail8859
Copy link
Owner

you just need to open this branch and open VisualStudio

I use QtCreator, but think it would be similar.

(you cannot add an out of source CMake, at least not trivially, yet)

But doesn't it technically already have out of source cmake since your cmake file builds the entire application fine?

I would definitely want to avoid having to fork every repository that doesn't support cmake. Every external Qt-based repo that I've looked at using has qmake support so its at least a bit easier but not perfect. Plus out of source qmake is fine.

Regarding upstreams

I honestly regret using submodules. They are nice if you want to use the code as-is and never want to modify it. But then its just annoying because its something you always have to be aware of.

I definitely don't want Python to be a requirement for building Notepad Next since Scintilla needs it generate the two files that don't come by default with Scintilla. So I'm not sure how cmake would handle that situation.

@txtsd
Copy link

txtsd commented Apr 17, 2022

CMake is the recommended build method for Qt6+

@dail8859
Copy link
Owner

I started this project years ago and qmake was the build system of choice for Qt based projects. Qt Creator also has much better cmake support, plus as you mentioned Qt using cmake now. At some point Notepad Next will only support Qt6. All this is why I'm willing to see how well cmake will work for this project.

@diegoiast
Copy link
Contributor Author

While the CMake stuff is nice... I could have chosen to keep the old monorepo workflow. However, I decided to use a multirepo approach. What does this give me? See the latest commits in this rebased branch, see how easy it is for me to update version.

See this for example:
https://github.com/elcuco/NotepadNext/blob/2a370c2d5cdbf814f6f4c20a0c597f2e463cf9c6/CMakeLists.txt#L23

(still WIP, need to add the CI stuff)

@txtsd
Copy link

txtsd commented Apr 17, 2022

@elcuco Is it independent of git submodules?

EDIT: nvm I was confused for a sec. Wouldn't it be better to include the other repos as git submodules instead of using cmake as a dependency manager?

@diegoiast
Copy link
Contributor Author

I can easily convert this to using git submodules + add_subdirectory. Its just harder to use for developers - its easier to handle text files than git submodules.

One of the future enhancements I want to add, is migrating this to conan. Using submodules defeats this goal.

@txtsd
Copy link

txtsd commented Apr 17, 2022

Why is conan better?

@diegoiast
Copy link
Contributor Author

Why is conan better?

If you add a library by conan, you can get pre-compiled version (most of the artifacts are on VS and OSX). You can get a deep dependency chain - by asking the upper level package. For example here: https://github.com/elcuco/nana-ide/blob/master/conanfile.txt I asked for libpng, which in turn got me libz.

I can handle also dependencies which are not packaged using cmake - for example boost, which in turn depends on openssl (both have non-cmake build systems). They get installed into a sandbox - and conan does magic to include the correct path for libs and includes. I heard of it as a "docker environment experience" (since nothing on the real system is modified). You can also have several applications (sub projects, for example) each depending on a different version of the same library (each sub-project will need to have a dedicated conan file).

This is what npm/gradle/pods/pip/cargo does - but for C++. You expand the amount of libraries available to your app.

@txtsd
Copy link

txtsd commented Apr 19, 2022

Neat. I'll have to see it in action!

btw if this is compiling for you locally, can you also make changes to the github workflow so we know if the CI can handle it?

@txtsd
Copy link

txtsd commented Apr 19, 2022

Awesome!

@diegoiast
Copy link
Contributor Author

Awesome!

Almost. The windows build is broken (scintilla's cmake is bad).
I also need to add steps to build the packages for each platform.
Then - I need to port whats needed to Qt6.
Feel free to send a PR to my branch, if you can help. Otherwise, it will get there eventually.

PS: I don't own a mac, so building packages for it might become a trial an error with the CI. Help would be appreciated.

@txtsd
Copy link

txtsd commented Apr 20, 2022

I'm not good with CMake, but I have some experience with the CI. I'll take a look tomorrow and see if there's anything I can help with!

@diegoiast
Copy link
Contributor Author

I fixed the building of all libraries to be static, as cmake installs into /usr/lib/lib-x86_64/ but AppImage does like this. Also CMake install would also install headers and *.a files. This will be a nice blog.

I will give a try in the next week to fix the install targets, and after that (hopefully) AppImage would work.

@stevenhoving
Copy link

stevenhoving commented Apr 28, 2022

@elcuco

Notes how I got this to build with VS2022:

scintilla-code-src\CMakeLists.txt
I noticed that you added target_link_libraries(scintilla-qt-edit-base pthread). pthread is not native to Visual Studio and in this case not needed. Maybe you need to add a ifdef there.

scintilla-code-src\qt\ScintillaEditBase\ScintillaEditBase.h
Because you changed the library to be build as a static library, having EXPORT_IMPORT_API be dllimport or dllexport is incorrect. The easiest way to fix this is to use https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html . This supports the shared and static build requirement while being easy on qt moc.

I hope these notes help you resolve the windows build issues.

Update:
My windows CMake setup usually makes sure that all the required dependencies are copied to the install directory when I build the install target. This simplifies things for me and for whoever wants to try to. I would advice you to do the same. This also simplifies packaging.

@diegoiast
Copy link
Contributor Author

diegoiast commented May 1, 2022

Minor update:

  1. Ported all code to compile under Qt6/CMake. I will push on the next days.
  2. Been working last week on compilation on MinGW/Windows. This was a pain, since the code on Windows for lexizilla exports/imports on windows - and this does not work on static libraries. So another fix will be needed there. I have a working EXE, soon will publish code.
  3. VS will follow.
  4. Then I will work on installer for Windows. I would like to use MinGW/Qt6/CMake - instead of VS - anyone objects?

@stevenhoving - just saw your comments, window did not refersh. Yeap, VS should be trivial. Nice.

@dail8859
Copy link
Owner

dail8859 commented May 1, 2022

Ported all code to compile under Qt6/CMake

I think Qt6 is the right choice. I haven't decided yet when to move official distribution to Qt6 but it is definitely desired.

Been working last week on compilation on MinGW/Windows.

Is this required?

I would like to use MinGW/Qt6/CMake - instead of VS - anyone objects?

The only build system I will be using on Windows is VS and I want this to be the official build system. If MinGW works then great, but there's no way I can troubleshoot MinGW issues if they would ever arise.

@diegoiast
Copy link
Contributor Author

diegoiast commented May 6, 2022

I managed to get the static linking issues fixed on MinGW, the CI confirmed this works also on CL/MSVC.
I have enabled the CI for Linux/OS/Windows on Qt6 and Qt5. App images are being built. Soon MSI, and then AppBundles.

EDIT - fixed CI as well
EDIT2 - Linux App images (Qt6, Qt5) work. I should change the app image name "soon" (so I can download both zips, and unpack).

@diegoiast diegoiast changed the title CMake support for Qt5 CMake support for Qt5 and Qt6 May 6, 2022
@diegoiast
Copy link
Contributor Author

Just a small comment: the CI is currently generating EXEs built with MSVC. It worked as soon as I fixed the MinGW build on my laptop. I think about adding another win64 build when this gets merged. I like the idea of using MinGW as another build (keeping with the FS on windows agenda).

@diegoiast
Copy link
Contributor Author

In my repo, you can see https://github.com/elcuco/NotepadNext/actions/runs/2318840719 - which has an artifact for Windows (a zip). I know the scripts build zip files for qt5, and qt6, and for qt6 I should also have an installer. Trying to identify whats borked. IMHO - a mingw build can also be produced.

However, I build installer only for MSVC Qt6. Because, WTF not.

@diegoiast
Copy link
Contributor Author

Rebased against master.

@dail8859
Copy link
Owner

Maintaining both Qt5 and Qt6 would be nice but if it makes things easier then I'd say don't worry about Qt5 for now.

At some point Qt5 will be dropped anyways.

@diegoiast
Copy link
Contributor Author

sorry for late response.

I think - that the best solution I can provide is a set of config items - so instead of CPMAddPackage it will FIND_PACKAGE. No big deal then, in the SPEC you can cmake -DNEXTPAD_NEXT_USE_SYSTEM_LIBS=true or something similar.

  1. uchardet - is already in Fedora. As they don't have a release for so long I am pulling from master directly. I am unsure what the diff is between 0.8 and master.
  2. LuaBridge - I don't see it in Fedora. You know what to do.
  3. Qt-Advanced-Docking-System - I don't see it in Fedora. Note that also QtCreator (at least!) uses it internally, so when you package this for fedora - you should also fix QtCreator.
  4. SingleApplication - this library is very small. It also compiles differently using different flags (uses QCoreApplication, vs QApplication) and thus has no really stable API. This is not a candidate for packaging.
  5. editorconfig-core-qt - not packaged. You will also need to package editorconfig itself.
  6. lua - already in fedora.
  7. QSimpleUpdater - it should be disabled from this build. (I think this is disabled for linux anyway).

To - new tasks:

  1. Merge with main.
  2. Each library will have a different config - then check against pkgconfig (or just find? dunno).
  3. New flag to disable updater (can be useful on windows I guess).
  4. Finish the installer problems on Windows.
  5. Finish the macOS installer (! is it done already?).

1) updated CPM to v0.36.0
2) uchardet: the github mirror is no longer active. Instead use the
official freedesktop gitlab mirror.
3) Updated sha1/versions of libraries
4) ADS is now at v4.1.1
5) LanguagePropertiesModel and frieds are removed from code
@Conan-Kudo
Copy link

sorry for late response.

I think - that the best solution I can provide is a set of config items - so instead of CPMAddPackage it will FIND_PACKAGE. No big deal then, in the SPEC you can cmake -DNEXTPAD_NEXT_USE_SYSTEM_LIBS=true or something similar.

Works for me.

@Neustradamus
Copy link

Dear all,

Any progress on it?

* add missing files to the build system
* cpm: trivial update from 0.38.6 to 0.40.2
* uchatdet: update from 2f5c24006ebc7f005040358f58f22a61a3c92522 to
edae8e81cfb8092496f94da1a306c4c9f0ce32bb, also build in release mode,
since upstream needs address sanatizer when building in debug
* AdvancedDockingSystem: trivial update from 4.1.1 to 4.3.1
* EditorCondigCore: trival update from
bf86460fe29b53731bba650d2a59d4c47eb25de9 to
ee967262db4fdbd735f9971cc0c90cf4f3100d3a
* QSimpleUpdater: using upstream's build system (!)
911727370d90001077f93ea94545099e197400a0
* lua: trivial update from 26be27459b11feabed52cf40aaa76f86c7edc977 to
50c7c915ee2fa239043d5456237f5145d064089b
* Scintilla: trivial update from
v5.3.3 to v5.5.3, also - use a shallow clone, as this is a big project
* Lexilla: update from v5.2.1 to v5.4.1. Updated  local build system as
well (not used, will get removed soon).

Compiles and runs on linux. I hope Windows does not break.
 - Windows: ucharsel needs to be downgraded, as it does not compile
under MSVC.
- Lexilla: on my local machine the remote build system works, however on
the CICD, it failed. Lets try the local build system.
@diegoiast
Copy link
Contributor Author

Hi, I just updated the dependencies. Once its settled, I will add a few switches to use local packages.

HOWEVER: the packages needed are not in Fedora. I can have a "not dealing with that have fun" switch, but you need a lot of work to make it happen.

  1. ADS - not in fedora
  2. LUA - compat-lua-devel? I think this is the one.
  3. LuaBridge: I don't see it. I saw texlive support...
  4. SingleApplication - this is not packagable. You can compile it using 2 different modes, using a configure switch. Both more are incompatible (one uses QCoreApplication, the other uses QApplication). Small library. Might as well be static. Maybe Qt has a newer alternative by now?
  5. editorconfig-core-qt - not in fedora
  6. QSimpleUpdater - not in fedora
  7. Scintilla - looks OK.
  8. Lexilla - I don't see it in Fedora - is it part of Scintilla?

@diegoiast
Copy link
Contributor Author

It is clear that I lost interest in this branch. Code compiles and works, but the missing parts are the installers creation: App image, flatpack, macOS.

I hope someone finishes this work. It should be "trivial" :)

@diegoiast diegoiast closed this Nov 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants