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

Clear header directory before overwriting it #1211

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

QuentinLeeWeber
Copy link
Contributor

Before writing the new header files, the header directory is now cleared. This change helps to avoid potential conflicts.

@@ -46,6 +46,21 @@ fn write_headers_in(subfolder: &str) {
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

side point, wonder if sccache uses the hash of the file or the mtime ?

Eg does modifying a file in the header dir (by deleting and writing the same contents again) cause a cache miss ? Or is it comparing the contents anyway.

Looking at the times in CI it seems to help with macOS and Windows Qt 5 specifically 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now doing a clean run after rebasing things vs a run on main both with the same cache we seem to have.

Runner Before After
macOS 13 Qt 5 7m 42s 7m 33s
macOS 14 Qt 6 6m 15s 7m 28s
Ubuntu Qt 5 5m 50s 5m 43s
Ubuntu Qt 6 6m 59s 8m 4s
Windows Qt 5 10m 16s 14m 9s
Windows Qt 6 MSVC2019 12m 19s 12m 57s
Windows Qt 6 MSVC2022 13m 1s 24m 57s

Seems that this makes Qt 6 1min slower for Linux and macOS, then Windows is curious too. Would probably need more reruns to be sure of the timings.

But it does make me wonder if we should be more intelligent than just delete everything 🤔

Copy link
Collaborator

@LeonMatthesKDAB LeonMatthesKDAB Mar 7, 2025

Choose a reason for hiding this comment

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

The thing with our CI is that in most cases, this change shouldn't affect it.
Especially on Windows and macOS, we're doing only clean builds, so there is nothing to delete anyway, so this doesn't change anything.

The interesting case is the cargo_rerun test case under Linux, as that would show a difference in caching behavior.

Before writing the new header files, the header directory is now
cleared. This change helps to avoid potential conflicts.
@ahayzen-kdab ahayzen-kdab force-pushed the clear-include-directory branch from b9211d8 to e832961 Compare March 6, 2025 15:54
Copy link

codecov bot commented Mar 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (17c21f6) to head (e832961).

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1211   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           73        73           
  Lines        12527     12527           
=========================================
  Hits         12527     12527           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants