Various CMake build improvements by emabrey · Pull Request #533 · tenacityteam/tenacity-legacy

emabrey · 2021-08-22T17:15:46Z

CMake will now enable linking IPO if the compiler supports it.

Signed-off-by: Emily Mabrey emabrey@tenacityaudio.org

Resolves: (direct link to the issue)

(short description of the changes and the motivation to make the changes)

Checklist

I have signed off my commits using -s or Signed-off-by* (See: Contributing § DCO)
I made sure the code compiles on my machine
I made sure there are no unnecessary changes in the code*
I made sure the title of the PR reflects the core meaning of the issue you are solving*
I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"*

* indicates required

nbsp · 2021-08-22T17:17:48Z

Have you tested this on a compiler which doesn't support IPO?

Be-ing · 2021-08-23T23:50:01Z

.github/workflows/cmake_build.yml

What is the reason for changing this away from Ninja?

I am using this to troubleshoot issues with the 32 bit builds. When I get it finalized it will have an entry for msbuild and for Ninja. It's a work in progress, that is why it is marked as a draft PR.

.github/workflows/cmake_build.yml

Be-ing · 2021-08-24T00:24:04Z

I'm confused why you're making all these changes to the GitHub Actions workflow together with enabling interprocedural optimization.

emabrey · 2021-08-25T02:53:59Z

I'm confused why you're making all these changes to the GitHub Actions workflow together with enabling interprocedural optimization.

It's because enabling IPO revealed to me that we haven't been correctly building win32 targets this whole time and I'm investigating different types of changes. I also discovered several issues with our detection of 32/64 bit targets and how we were describing the CMake vars. I understand you are trying to be helpful but reviewing things before I mark them ready for review is just demoralizing and forces me to either temporarily ignore your feedback or explain changes that may not even make it to the final version of the PR.

emabrey · 2021-08-25T03:02:44Z

Have you tested this on a compiler which doesn't support IPO?

That is why it uses CheckIPOSupported. And it isn't finished yet so I've not gotten to testing it.

Be-ing · 2021-08-25T03:48:02Z

I'm confused why you opened a pull request if you don't want it reviewed yet?

emabrey · 2021-08-25T04:22:44Z

I'm confused why you opened a pull request if you don't want it reviewed yet?

GitHub has a feature called draft PRs. I haven't opened a true pull request, it is marked as a draft. Why did I open a draft PR? To trigger the CI builds, since that is the thing I am interacting with mostly.

Be-ing · 2021-08-25T04:29:21Z

By opening a pull request I assume that you want the code reviewed. As discussed the other day, you do not need to open a pull request to test changes on GitHub Actions.

emabrey · 2021-08-25T04:46:10Z

By opening a pull request I assume that you want the code reviewed. As discussed the other day, you do not need to open a pull request to test changes on GitHub Actions.

I didn't open a pull request, I opened a draft pull request, so your assumption was faulty. I appreciate the help, I do, but I do not feel as productive doing it your way because it requires me to spend a lot of time doing stuff that doesn't actually contribute to anything valuable to the project. I don't need to use the draft PR feature, I am of course able to do it your way and reach parity with my workflow, it just requires me to adjust the way that I am doing things and do a bunch of extra work duplicating the CI process on my personal repo, and for no apparent value. I do not need to do it this way, but I want to do it this way. It saves me a lot of time and effort and the only benefit I can see to doing it the way you would prefer is that it would make sure the feng shui of the PR list was more in line with your taste. The only other alternative would be to run CI on non-master branches, which would be a pointless waste of CI time and massively increase our usage of CI on Sourcehut and on GitHub, not to mention massively increase our usage of Nuget cache, which we have already exceeded limits for on Artifactory and which I am about to start working on a POC for moving to Github packages since the initial information we received about the limits there appears not to be correct.

Be-ing · 2021-08-25T05:33:28Z

It doesn't require any extra work. You just push to a different Git remote. That's it.

The only other alternative would be to run CI on non-master branches, which would be a pointless waste of CI time and massively increase our usage of CI on Sourcehut and on GitHub, not to mention massively increase our usage of Nuget cache

That only happens if you use branches on the upstream repository, which myself and @SFR-git have asked to not do.

CMake will now enable linking IPO if the compiler supports it Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>

Add CMake host architecture detection Correct debug messages about CMake target architecture Rearrange some sections of `CMakeLists.txt` to be more clear Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>

Rearrange workflow declaration to be more readable Reduce duplication of config variables within CI matrices Change usages of Windows builds to use Visual Studio 16 2019 generator Add correct usage of CMAKE_GENERATOR_PLATFORM for Windows targets Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>

Switch Nuget source to Github Packages instead of Artifactory Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>

Fix how CPack finds build directory in Linux/macOS packaging Make innosetup target use the same CMAKE_BUILD_TYPE in Windows packaging Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>

Add retry code because the `dependencies.sh` script would randomly fail on CI Fix typo in Nuget source usage in CI Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>

Rename usage of outdated Innosetup functions Configure CMake to use the same build type for Innosetup target Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>

Use Tenacity logos Update readme information displayed post-install Update license information displayed pre-install Fix installer generation Cleanup innosetup configuration Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>

Work around `float_cast.h` redefining compiler helpers intrinsically Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>

Incorporate hiDPI changes from upstream Co-authored-by: Lucas Fugmann <lucas@fugi.dev> Co-authored-by: Emily Mabrey <emabrey@tenacityaudio.org> Signed-off-by: Lucas Fugmann <lucas@fugi.dev> Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org> Reference-to: 3d826b0 Reference-to: https://github.com/tenacityteam/tenacity/issues/502

Remove unneccesary script `cd` command Reword vcpkg caching comment Make comment about wxwidgets Linux/macOS workaround more clear Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>

emabrey force-pushed the cmake-improvements branch 3 times, most recently from f9de828 to 5b4a135 Compare August 23, 2021 22:26