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

Modern CMake Upgrade #481

Merged
merged 185 commits into from
Apr 29, 2019
Merged

Conversation

jameskr97
Copy link
Member

@jameskr97 jameskr97 commented Mar 19, 2019

This is my sixth pull request on this repository.

Here is a summary of everything to be changed in this pull request.

Required before the merge

These first three are the hardest to track/keep separate in different commits, since the build system affects everything, and change one can also affect the others. The commit where each OS was able to build properly regardless of the other OS's also compiling is noted with the commit ID.

  • Linux compiling on upgraded CMake b38e2e
  • Windows compiling on upgraded CMake (32bit and 64bit) daa507
  • macOS compiling on upgraded CMake c22b4e
  • Complete rewrite of build instructions. f43aa1
  • Travis upgraded to use new CMake setup
    • Linux b01107
    • macOS 6d9c4d
    • Windows Will not be done this PR. Currently, Travis is not set up for this.
  • Appveyor upgraded to use new CMake setup f13eb2
  • Hide warnings for all external libraries 301ccf
  • Replace following submodules with subtrees
    • discord-rpc ce4453
    • libuv d5b2e9
    • SQLiteCpp 240c9d
    • json 26e519
  • Create Windows installer with NSIS and CPack 32f928
  • Create macOS DMG with CPack b3ca40
  • Implement cppcheck, link-what-you-use, clang-tidy, clang-format within CMake To be added in different PR. Too much is done in this one

Windows-specific changes in PR

  • Sorting external libraries into their own folders within Visual Studio 336bf3
  • Run Etterna by default in visual studio 565815

Other changes within PR

  • Merge Update RageSurface_Save_JPEG to use stbi_write_jpg #489 into cmake-remake While unrelated, a significant part of Update RageSurface_Save_JPEG to use stbi_write_jpg #489 is changing what is linked. Since it would no longer be linkable in it's the current state with the old CMake I will merge to update the CMake related files.
  • STB related files moved to their own directory separate from Etterna source c02ef9
  • Removing unused parallel_lights_io.dll. 5b8334
  • Removed wldap32.lib in libcurl 4ecb34
  • Remove references to BYTE_ORDER, LITTLE_ENDIAN, BIG_ENDIAN. 16870a
  • Added macOS app bundle icon f65557
  • Removed unused icons folder d37f50

git-subtree-dir: extern/discord-rpc
git-subtree-split: c59fd6df20c6904ab39f026e87af1dd90fcac7ff
As of this commit, only linux can compile.
Travis-CI is not at the point where we can use it for Etterna CI. Though they are close to it. If OpenSSL could be preinstalled on Windows VM's, it would be possible to completely switch over to travis for CI/CD. The OpenSSL.light package within chocolatey only contains the DLL files, not the header files which are also needed by Etterna.
.appveyor.yml Outdated Show resolved Hide resolved
.ci/build.sh Outdated Show resolved Hide resolved
WindowsResources.rc should be evaluated to see if it's actually necessary to have or there is a better way to accomplish the same thing.
Fission suggested that this segment of code required an explanation as to why it was there, and it's relationship to the CMake install function within .ci/build.sh. I agreed and added said explanation.
Changes were discussed within Etterna Dev Group discord server
@jameskr97 jameskr97 requested a review from martensm April 10, 2019 08:21
This folder is genearted when the game is run. There is no need to keep any generated files within the repository.
The previous .gitignore was filled with a lot of lines which are useful only if the game was being built as an out-of-source build. While some  parts of the codebase are still used out-of-source, the majority of the builds are contained within the Build directory. That isolation allows much of the .gitignore to be removed.
The previous one accidentally ignored the entire Etterna directory
located in the src folder. This will only block the Etterna binary that
gets generated in the root when building on unix.
Previous commit was a mistake to commit. It was reverted and this commit takes it's place.
Copy link
Contributor

@martensm martensm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe there is anything else that should be done in the scope of this PR and it LGTM.

Copy link
Member

@poco0317 poco0317 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The game seems to have problems opening due to an issue with the game not being able to create the Loading Window on Windows. I've detailed this issue in Discord.

This file was removed to evaulate it's necessity. It was a heavy-handed removal being reversed in this commit, as this file gives the game it's error popup and executable icon. The file is still reliant on an unnecessary smzip.ico, and likely could be simplified and moved to a more suitable location.
Copy link
Member

@poco0317 poco0317 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the last change, it works well for me on 64 bit Windows; Release and Debug.
32 bit not tested, but I assume it works as well.

@jameskr97
Copy link
Member Author

jameskr97 commented Apr 18, 2019

@poco0317 My testing for the d8472fd commit was 32bit. I can confirm it still works.

@MinaciousGrace MinaciousGrace merged commit faea45c into etternagame:develop Apr 29, 2019
@jameskr97 jameskr97 deleted the cmake-remake branch May 11, 2019 21:39
@jameskr97 jameskr97 restored the cmake-remake branch May 11, 2019 21:39
@jameskr97 jameskr97 deleted the cmake-remake branch May 11, 2019 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants