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

Fix cmake soci patch (duplicate) #3915

Closed
wants to merge 2 commits into from

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Aug 26, 2021

High Level Overview of Change

This is a copy of #3886 with some changes suggested in review, but that @guidovranken has not yet addressed. When merging do not squash their commit so that their work can be acknowledged.

Original from #3886
This prevents soci getting patched more than once over the course of multiple builds within the same CMake build environment.

Context of Change

Original from #3886
The various unity_*_cxx.cxx.o kept getting built anew after they've been built already.
The reason for this is that each time the make command is issued, soci is patched anew. Because soci is a dependency of the unity_*_cxx.cxx.o files, the build system then reasons that the unity_*_cxx.cxx.o files must be built anew because soci has a more recent timestamp.

Patching soci only needs to be done once; from that point on it can be reused any number of times.

This PR makes a change to the script responsible for patching, and only proceeds to patch if it finds that there is no back-up of original (unpatched) soci. If a back-up already exists, this indicates that soci has already been patched, and the patch procedure is skipped.

Update:
This PR makes a change to the script responsible for patching. It patches the file, then compares the patched file to the original. If they are identical, it deletes the patched file and exits. If not, it backs up the original, then replaces it with the patch.

Type of Change

  • [X ] Bug fix (non-breaking change which fixes an issue)

Before / After

Before: Repeated builds in the same CMake environment with no changes will unnecessarily build many of the files. In unity builds, they will be unity_*_.cxx. In non-unity builds, they will be individual source files.

After: Repeated builds in the same CMake environment does not build any source, allowing the build to take far less time. On my workstation, time for various build configs was reduced from a range of 11s - 42s to a range of 2s - 7s. Unfortunately, CMake still has some overhead dealing with external projects and such, which accounts for some of the remaining time.

Future Tasks

Find any other unnecessary file replacements and fix those.

@ximinez
Copy link
Collaborator Author

ximinez commented Aug 27, 2021

I took a quick look at the other patch commands and noticed that Rocksdb was unnecessarily copying a file repeatedly. This fix was nearly trivial: Change copy to copy_if_different. Since it's so simple, I'm pushing it here. LMK if there are any objections.

@ximinez
Copy link
Collaborator Author

ximinez commented Aug 27, 2021

Oh, and I also cleaned up the commit messages a little bit.

@ximinez ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Sep 8, 2021
guidovranken and others added 2 commits September 8, 2021 14:17
* Patch the soci unsigned-types.h file. If no changes are made, delete
  the patched file and exit. If there are changes, backup the original
  and replace it with the patched file.
* Fixes XRPLF#3885
* The repeated patches do not appear to affect build times, but avoiding
  unnecessary copies is good for its own sake.
@ximinez
Copy link
Collaborator Author

ximinez commented Sep 8, 2021

Squashed and marked passed

@manojsdoshi manojsdoshi mentioned this pull request Sep 9, 2021
@nbougalis nbougalis mentioned this pull request Sep 9, 2021
@ximinez ximinez deleted the fix-cmake-soci-patch branch September 15, 2021 14:33
@legleux legleux removed their assignment Sep 17, 2021
@legleux legleux removed their request for review September 27, 2021 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Build System Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants