Skip to content

Conversation

@RytoEX
Copy link
Member

@RytoEX RytoEX commented Apr 26, 2022

Description

Use the hash of the Qt patches for the cache key for the Qt build jobs. Use restore-keys to do a fuzzy-match to restore the cache if the exact cache key is not found. This should allow us to restore the Qt cache and do a rebuild if the patches used for Qt change. This means we don't have to remember to change the QT_REVISION variable if we add new patches or change the existing ones.

Motivation and Context

Hopefully, we can avoid full rebuilds of Qt on CI when we've just changed patch files. We're still unsure how well this will work, so it might be best to merge this and see how the subsequent Qt patch PRs (#101 and #102) behave.

Edit:
This will only force a rebuild if the Qt patches have changed. It won't let us do partial rebuilds by itself. For that, we would need to also cache the intermediate build directory.

How Has This Been Tested?

CI is the test.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@RytoEX RytoEX added the enhancement New feature or request label Apr 26, 2022
@RytoEX RytoEX requested a review from PatTheMav April 26, 2022 21:04
@RytoEX RytoEX force-pushed the improve-qt-cache branch from 14860f0 to dfcf835 Compare April 26, 2022 22:59
@RytoEX
Copy link
Member Author

RytoEX commented Apr 27, 2022

hashFiles is returning empty strings for some reason. I'll have to debug further.

@RytoEX RytoEX marked this pull request as draft April 27, 2022 16:00
Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

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

Looks right syntactically.

@RytoEX
Copy link
Member Author

RytoEX commented May 3, 2022

hashFiles is supposed to operate with GITHUB_WORKSPACE as its root, but I'm wondering if the path input for this step is causing it to change its root.

@RytoEX RytoEX force-pushed the improve-qt-cache branch from dfcf835 to 9a0d4ad Compare May 5, 2022 22:50
@RytoEX
Copy link
Member Author

RytoEX commented May 5, 2022

Looks like hashFiles does not like this pattern: QTBUG-[0-9]+.patch. I've amended to use QTBUG-*.patch instead.

@RytoEX
Copy link
Member Author

RytoEX commented May 5, 2022

What I'd love to achieve is only rebuilding the parts of Qt that the changed patches affect, but I'm not sure if that's achievable. At least this improves the cache-hit mechanism without us having to remember to forcibly and manually invalidate the cache.

@RytoEX RytoEX marked this pull request as ready for review May 6, 2022 01:04
@RytoEX RytoEX requested a review from PatTheMav May 6, 2022 01:04
@RytoEX
Copy link
Member Author

RytoEX commented May 6, 2022

This should be working as designed now.

  with:
    path: /Users/runner/work/obs-deps/obs-deps/macos/obs-dependencies-qt-x86_64
    key: macOS-deps-qt-cache-x86_64-04-5.15.2-a4d91fdf8b6becc94411e23e89d5d3fa0c7ed39630f66f7ff2faae511626951a
    restore-keys: macOS-deps-qt-cache-x86_64-04-5.15.2
[...]
Cache restored successfully
Cache restored from key: macOS-deps-qt-cache-x86_64-04-5.15.2
[...]
Cache saved with key: macOS-deps-qt-cache-x86_64-04-5.15.2-a4d91fdf8b6becc94411e23e89d5d3fa0c7ed39630f66f7ff2faae511626951a

It should restore from macOS-deps-qt-cache-x86_64-04-5.15.2, and then save to macOS-deps-qt-cache-x86_64-04-5.15.2-a4d91fdf8b6becc94411e23e89d5d3fa0c7ed39630f66f7ff2faae511626951a. Subsequent runs without changes to the patch files should use the latter key exactly and skip the build process. Subsequent runs that change the patch files should locate the most recently created cache that matches the restore-keys specification.

This doesn't get us to the quicker partial rebuild that I really want. For that, we'd probably have to cache the intermediate build directories, and I'm not sure what the tradeoffs would be (time spent restoring/saving the bigger cache and more cache usage versus time saved doing full vs. partial rebuilds if a patch is added/removed/changed). However, this does give us an idea of how to better ensure that patch file changes force a rebuild via cache invalidation.

Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

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

Yeah it's still a bit here and there, but the change is simple enough that I think it's worth the improvements it brings.

Use the hash of the Qt patches for the cache key for the Qt build jobs.
Use restore-keys to do a fuzzy-match to restore the cache if the exact
cache key is not found. This should allow us to restore the Qt cache and
do a rebuild if the patches used for Qt change. This means we don't have
to remember to change the QT_REVISION variable if we add new patches or
change the existing ones.
@RytoEX RytoEX force-pushed the improve-qt-cache branch from 9a0d4ad to 1437652 Compare May 6, 2022 15:35
@RytoEX RytoEX merged commit f18853f into obsproject:master May 6, 2022
@RytoEX RytoEX deleted the improve-qt-cache branch May 6, 2022 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants