Skip to content

cxx-qt-build: return an Interface from CxxQtBuilder #1287

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

LeonMatthesKDAB
Copy link
Collaborator

Another take on #1285.

The main difference is in how include paths are handled for the CXX-style automatic include of the header directory.

In this PR, the root of the automatic inclue can be configured.
This leads to less changes for downstream crates.
It also allows us to move to an include system of "cxx-qt-lib/core/qstring.h" while keeping backwards-compatibility with forwarding-headers

  • cxx-qt-build: split interface into a separate file
  • cxx-qt-build: move write_manifest to export of Interface
  • cxx-qt-build: move write of exported include directories to interface
  • cxx-qt-build: store manifest and dependencies in Interface for now
  • cxx-qt-build: return an Interface rather than taking as input
  • cxx-qt-build: check the links key when exporting
  • cxx-qt-build: remove CxxQtBuilder::library instead use new and export
  • cxx-qt-build: Add CXX-style automatic inclusion of header files
  • cxx-qt-build: Remove Interface::export_include_dir

@LeonMatthesKDAB LeonMatthesKDAB force-pushed the 1125-build-system-changes-experiments branch from 8f18f8a to 6a6c7c0 Compare May 26, 2025 07:08
Copy link

codecov bot commented May 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (96d3233) to head (adc3164).

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1287   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           75        75           
  Lines        12772     12772           
=========================================
  Hits         12772     12772           

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

@LeonMatthesKDAB LeonMatthesKDAB force-pushed the 1125-build-system-changes-experiments branch 3 times, most recently from 9f8bd4e to 4bb39aa Compare May 26, 2025 12:18
@ahayzen-kdab
Copy link
Collaborator

Overall looking good, does now need a rebase now the other branch has landed, unsure if it'll conflict in any way.

ahayzen-kdab and others added 9 commits May 28, 2025 12:30
This still needs more refactoring later but is a first step.
This allows export() to have no args meaning we can return an Interface
from the compile command next.
We however have two changes compared to CXX:
1. We allow you to change the root of the automatic include directory.
2. We allow you to include additional directories.

Both of these changes are there to accomodate crates like cxx-qt-lib,
that want to use e.g. `<cxx-qt-lib/qstring.h>` instead of
`<cxx-qt-lib/include/qstring.h>`.
Also cxx-qt-lib generates a definitions header which we need to include.
This is no longer needed.

Also clean up some documentation on the API.
@LeonMatthesKDAB LeonMatthesKDAB force-pushed the 1125-build-system-changes-experiments branch from 4bb39aa to f0c7317 Compare May 28, 2025 10:31
@LeonMatthesKDAB LeonMatthesKDAB marked this pull request as ready for review May 28, 2025 13:32
The function will now panic if it is called multiple times.
@LeonMatthesKDAB LeonMatthesKDAB force-pushed the 1125-build-system-changes-experiments branch from 8245fd7 to adc3164 Compare May 28, 2025 14:15
Copy link
Collaborator

@ahayzen-kdab ahayzen-kdab left a comment

Choose a reason for hiding this comment

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

As discussed think this is a good step in the right direction, didn't know if you want to use this in it's current state or keep working on it. Going to approve so you can land this if required and iterate separately :-)

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.

2 participants