Skip to content
This repository was archived by the owner on Dec 18, 2022. It is now read-only.

CMake/CI Build Improvements#545

Merged
emabrey merged 4 commits intomasterfrom
emabrey/cmake-improvements
Sep 7, 2021
Merged

CMake/CI Build Improvements#545
emabrey merged 4 commits intomasterfrom
emabrey/cmake-improvements

Conversation

@emabrey
Copy link
Member

@emabrey emabrey commented Aug 27, 2021

  • Automatically enable inter-procedural optimization (IPO) on supporting compilers using CMake configuration changes
  • Fix 32-bit builds to not redefine compiler/cstdlib internal functions during IPO enabled builds ( as it is not possible because of IPO in-lining)
  • Add GitHub CI configuration to use Visual Studio Generator
  • Fix incorrect comments/debug messages which incorrectly described the meaning of the IS_64_BIT variable
  • Refactor GitHub CI cmake_build.yml workflow configuration to increase readability and maintainability
  • Update vcpkg binary cache to use Github Packages
  • Change CI builds to create caches by id instead of by name
  • Refactor CMakeLists.txt to use consistent formatting and be laid out more logically

I have no idea why, but this seems to have partially resolved the MacOS build problems we've been having with CPack. I have built this several times now and it builds two different jobs on MacOS and initially none of them failed.

Reference-to: https://github.com/tenacityteam/tenacity/pull/198
Resolves https://github.com/tenacityteam/tenacity/issues/526

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

@emabrey emabrey force-pushed the emabrey/cmake-improvements branch 2 times, most recently from 218e862 to 8b88103 Compare August 27, 2021 22:29
@emabrey emabrey changed the title CI Build Fixes CMake/CI Build Improvements Aug 27, 2021
@emabrey emabrey marked this pull request as ready for review August 28, 2021 03:10
@emabrey emabrey requested review from a team and Be-ing August 28, 2021 03:10
@emabrey emabrey force-pushed the emabrey/cmake-improvements branch from 0c8180e to d4ca047 Compare August 28, 2021 05:11
@caughtquick caughtquick added the build Related to the software build process label Aug 28, 2021
@emabrey emabrey added bug Some kind of fixable problem was encountered enhancement Idea or request for new feature labels Aug 28, 2021
Copy link
Contributor

@Be-ing Be-ing left a comment

Choose a reason for hiding this comment

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

This PR has a bunch of unrelated changes together. Please split it up so they can be discussed separately.

@emabrey
Copy link
Member Author

emabrey commented Aug 28, 2021

This PR has a bunch of unrelated changes together. Please split it up so they can be discussed separately.

No. They are all related fixes that improve the CI build. I'm not doing extra work to split up a bunch of related changes. Additionally, most of them don't work split apart. Please stop making this same comment on every single PR I make. It's not a review, it's not helpful and it's not productive.

@Be-ing
Copy link
Contributor

Be-ing commented Aug 28, 2021

I don't get how the changes to the GitHub Actions workflow file, many of which I think are steps backwards, have to do with the changes to the other files. I don't want to hold up the improvements like IPO over that.

@emabrey
Copy link
Member Author

emabrey commented Aug 28, 2021

I don't get how the changes to the GitHub Actions workflow file, many of which I think are steps backwards, have to do with the changes to the other files. I don't want to hold up the improvements like IPO over that.

If you think that there are problems with the way I improved the cmake_build workflow then you need to be specific.

Here are the things I fixed about that workflow:

  • It incorrectly used the job names (which include spaces and are not guaranteed to be unique) instead of the job ID (which are unique and non-spaced) to generate the filenames for the caches

  • It failed to specify manually what the job ID should be, so it was being auto-assigned one which is auto-generated and potentially less useful

  • It included massive amounts of redundancy and "copy paste" between jobs, all of which destroys readability and maintainability

  • It failed to use the CI process to test any CMake generator other than the one you personally prefer

  • It failed to centralize multi-step configuration variables and left them strewn out across all the steps

  • The packaging step somehow massively increased the prevalence of the timing issues with MacOS because of the use of && instead of placing the CPack directory cleanup/removal command on a separate line

  • It used the Artifactory Nuget provider instead of the Github Packages provider, which is a change that ties it implicitly in with the rest of the changes in this PR

  • It was using up massive amounts of disk space and getting very close (i.e. within 100MB before I noticed it when it broke the 50MB barrier and generated a warning) to exceeding the maximum allowed space for the CI of 14GB disk usage

@Be-ing
Copy link
Contributor

Be-ing commented Aug 28, 2021

I was trying to avoid getting into an in depth discussion about all of those points. Most of those are distinct and can be split to separate PRs.

@emabrey
Copy link
Member Author

emabrey commented Aug 28, 2021

I was trying to avoid getting into an in depth discussion about all of those points. Most of those are distinct and can be split to separate PRs.

This PR is a general improvement to the CI builds. All the changes implemented are scoped to that idea (improve builds on CI). The IPO changes aren't external to improving CI builds, so they shouldn't be split out. The rule about separate PRs doesn't make sense to apply in this context because what you are actually saying is you want me to split it apart because you don't like some of the improvements and that makes it easier for you to discard some of them by ignoring them (more or less the coding equivalent of a heckler's veto**). That's not what that rule is designed to do. If you can point out how any of the changes don't improve CI builds then I would be happy to consider splitting them out, but currently AFAIK all the changes are in scope to that change premise. Alternatively, if you can point out why you believe that some of the changes that are in scope to this PR are inappropriate or in need of additional changes (i.e., a review), that would also be helpful. The PR splitting procedure is not appropriately used to break a change-set into a bunch of small pieces and then ignore the ones you don't like without providing any actual criticism or review.

Edit: ** Without the connotative implications about being impolite. I'm not implying that.

@emabrey
Copy link
Member Author

emabrey commented Aug 28, 2021

I was trying to avoid getting into an in depth discussion about all of those points. Most of those are distinct and can be split to separate PRs.

This PR is a general improvement to the CI builds. All the changes implemented are scoped to that idea (improve builds on CI). The IPO changes aren't external to improving CI builds, so they shouldn't be split out. The rule about separate PRs doesn't make sense to apply in this context because what you are actually saying is you want me to split it apart because you don't like some of the improvements and that makes it easier for you to discard some of them by ignoring them (more or less the coding equivalent of a heckler's veto**). That's not what that rule is designed to do. If you can point out how any of the changes don't improve CI builds then I would be happy to consider splitting them out, but currently AFAIK all the changes are in scope to that change premise. Alternatively, if you can point out why you believe that some of the changes that are in scope to this PR are inappropriate or in need of additional changes (i.e., a review), that would also be helpful. The PR splitting procedure is not appropriately used to break a change-set into a bunch of small pieces and then ignore the ones you don't like without providing any actual criticism or review.

Edit: ** Without the connotative implications about being impolite. I'm not implying that.

The only change that maybe should be split out would be the fix to cmake-modules/FindVampHostSDK.cmake, but I was planning on adding CI configurations to test debug builds in addition to release builds, so it's more like it's related but not as strongly.

@emabrey emabrey force-pushed the emabrey/cmake-improvements branch 5 times, most recently from 2fd9d34 to 67c2d80 Compare August 29, 2021 03:04
@Be-ing Be-ing requested a review from nbsp August 29, 2021 06:20
Copy link

@nbsp nbsp 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 think anything related to DarkAudacity should be touched. If anything, we should remove it outright, but that's outside the scope of this PR.

@nbsp
Copy link

nbsp commented Aug 29, 2021

I don't think anything related to DarkAudacity should be touched. If anything, we should remove it outright, but that's outside the scope of this PR.

Wait, no, I might've misunderstood the change. Why is a file not related to DarkAudacity whatsoever named darkaudacity.ico?

Copy link
Contributor

@Be-ing Be-ing left a comment

Choose a reason for hiding this comment

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

Please stop making this PR even bigger with even more unrelated commits.

@emabrey emabrey force-pushed the emabrey/cmake-improvements branch 2 times, most recently from 1638783 to 325d596 Compare September 2, 2021 15:44
@emabrey
Copy link
Member Author

emabrey commented Sep 2, 2021

Assuming this last force-push builds successfully I will split this up into separate branches where possible and submit separate PRs.

@emabrey emabrey force-pushed the emabrey/cmake-improvements branch 8 times, most recently from a0e2dc0 to 310cf60 Compare September 3, 2021 20:51
@emabrey
Copy link
Member Author

emabrey commented Sep 3, 2021

@emabrey
Copy link
Member Author

emabrey commented Sep 4, 2021

I believe this will be fixed once https://github.com/tenacityteam/tenacity/pull/578 is merged and then it can be reviewed without any errors.

@emabrey emabrey force-pushed the emabrey/cmake-improvements branch from 310cf60 to 8df5c43 Compare September 4, 2021 05:39
@emabrey
Copy link
Member Author

emabrey commented Sep 4, 2021

I believe this will be fixed once #578 is merged and then it can be reviewed without any errors.

I was right 😎

@emabrey emabrey requested review from a team and nbsp September 4, 2021 13:21
CMake will now enable linking IPO if the compiler supports it

Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>
Reword vcpkg caching comment
Make comment about wxwidgets Linux/macOS workaround more clear
Give each step of `cmake_build.yml` workflow a unique ID
Change cache lookups to use step's id instead of step's name
Add glob protection to runner OS detection in CI
Set new default CI build type of MinSizeRel
Ensure consistent formatting across `CMakeLists.txt` files
Change mimetypes generated on Linux to use tenacity-project mimetype
Add more explanatory comments to both `CMakeLists.txt`
Replace improper build architecture debug messages

Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>
Add `umount` command behavior
Modify to retry 12 times over the course of 240 seconds total
Change `scripts\build\macOS\DMGSetup.scpt` to close DMG when finished.
Improve system detection for setting project name.

Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>
Remove github actions workaround and replace by changing upstream vcpkg commit

Signed-off-by: Emily Mabrey <emabrey@tenacityaudio.org>
@emabrey emabrey force-pushed the emabrey/cmake-improvements branch from 8df5c43 to a1756f3 Compare September 6, 2021 06:06
@emabrey emabrey merged commit c046542 into master Sep 7, 2021
@emabrey emabrey deleted the emabrey/cmake-improvements branch September 7, 2021 11:43
@Be-ing
Copy link
Contributor

Be-ing commented Sep 7, 2021

Whoa wtf? This branch was not approved by anyone!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Some kind of fixable problem was encountered build Related to the software build process enhancement Idea or request for new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows build is broken by missing vamp-hostsdk.dll

5 participants