Skip to content

Conversation

@jimporter
Copy link
Contributor

Spinning this off from the update to C++20 to nail down some strange behavior on macOS.

@codecov
Copy link

codecov bot commented Sep 24, 2025

Codecov Report

❌ Patch coverage is 81.57895% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.43%. Comparing base (4980c78) to head (323c490).
⚠️ Report is 190 commits behind head on main.

Files with missing lines Patch % Lines
...rc/platform/backends/qemu/linux/dnsmasq_server.cpp 40.00% 3 Missing ⚠️
src/network/url_downloader.cpp 33.33% 2 Missing ⚠️
src/utils/utils.cpp 92.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4390      +/-   ##
==========================================
+ Coverage   88.97%   89.43%   +0.45%     
==========================================
  Files         238      240       +2     
  Lines       15253    13851    -1402     
==========================================
- Hits        13572    12387    -1185     
+ Misses       1681     1464     -217     

☔ 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.

@jimporter
Copy link
Contributor Author

jimporter commented Sep 24, 2025

@jimporter jimporter force-pushed the update-qt branch 4 times, most recently from badcff1 to 4ec88e4 Compare October 20, 2025 21:09
In Qt 6.8, this function's behavior changed, which would force us to
work around those changes. Since we plan to remove our dependency on Qt,
we replace these calls with `std::filesystem` calls instead.
@jimporter jimporter force-pushed the update-qt branch 2 times, most recently from 8c41795 to 4a8c329 Compare October 20, 2025 22:49
@jimporter jimporter requested review from sharder996 and xmkg October 20, 2025 22:51
@jimporter jimporter marked this pull request as ready for review October 20, 2025 22:51
@jimporter
Copy link
Contributor Author

❌ Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.

This is a strange coverage report...

image

@sharder996
Copy link
Collaborator

How do you feel about moving to Qt 6.10 right away? Or, was there a specific reason for using 6.9?

@jimporter
Copy link
Contributor Author

jimporter commented Oct 24, 2025

How do you feel about moving to Qt 6.10 right away? Or, was there a specific reason for using 6.9?

I'm not strongly opposed, though Ubuntu 25.10 uses Qt 6.9.2, so sticking with the 6.9 series on Mac and Windows would mean less version skew on our official builds (once we start using 25.10 in CI anyway e: GitHub uses LTS images). If we don't think that matters though, we can switch to 6.10.

@xmkg
Copy link
Member

xmkg commented Oct 24, 2025

How do you feel about moving to Qt 6.10 right away? Or, was there a specific reason for using 6.9?

I'm not strongly opposed, though Ubuntu 25.10 uses Qt 6.9.2, so sticking with the 6.9 series on Mac and Windows would mean less version skew on our official builds (once we start using 25.10 in CI anyway e: GitHub uses LTS images). If we don't think that matters though, we can switch to 6.10.

FTR I plan to migrate the QT to vcpkg too after this one lands, so feel free to use the latest and the greatest.


- `C:\Program Files\CMake\bin`
- `C:\Qt\6.2.4\msvc2019_64\bin`
- `C:\Qt\6.10.0\bin`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: The directory structure for the qtbase binary archive is different now.

@jimporter jimporter force-pushed the update-qt branch 3 times, most recently from 5736e9b to dc53386 Compare October 24, 2025 21:09
@jimporter jimporter force-pushed the update-qt branch 7 times, most recently from 70ed8d4 to c6ece3c Compare October 24, 2025 23:21
The binaries we used from the previous version were built with C++14 on
macOS, preventing us from using Qt's `std::filesystem` integration.
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