cxx-qt-build: return an Interface from CxxQtBuilder #1224
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1224 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 73 73
Lines 12634 12642 +8
=========================================
+ Hits 12634 12642 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ahayzen-kdab
force-pushed
the
1125-build-system-changes
branch
from
dbe3464 to
08febd9
Compare
ahayzen-kdab
force-pushed
the
1125-build-system-changes
branch
from
08febd9 to
5f8f83d
Compare
ahayzen-kdab
force-pushed
the
1125-build-system-changes
branch
6 times, most recently
from
24e624b to
fb6ff0f
Compare
| .collect() | ||
| } | ||
|
|
||
| fn reexported_dependencies(interface: &Interface, dependencies: &[Dependency]) -> Vec<Dependency> { |
There was a problem hiding this comment.
This should be in impl Interface, no?
Same with all_include_prefix.
There was a problem hiding this comment.
Could be, this has just moved the existing code :-)
| .expect("Failed to convert Manifest to JSON!"); | ||
| std::fs::write(&manifest_path, manifest_json).expect("Failed to write manifest.json!"); | ||
| println!( | ||
| "cargo::metadata=CXX_QT_MANIFEST_PATH={}", |
There was a problem hiding this comment.
This assumes we have a links key set, right?
We should probably panic if it isn't the case here.
However, then we'd have to check whether we're currently exporting to CMake, as then it's not needed.
There was a problem hiding this comment.
A few lines further up (at the start of this export() function) we panic if the link name is empty?
crates/cxx-qt-build/src/lib.rs
Outdated
| @@ -1134,6 +1083,17 @@ extern "C" bool {init_fun}() {{ | |||
| // to the generated files without any namespacing. | |||
| include_paths.push(header_root.join(&self.include_prefix)); | |||
|
|
|||
| // Automatically add any include/ directory from the cargo manifest dir | |||
There was a problem hiding this comment.
This will mean any directory named include/ will be available directly without prefix, right?
e.g. something like this:
include/test.h
Cargo.toml
Will mean it'll be possible to do:
#include <test.h>I think CXX behavior was to have this available as:
#include <my_crate/include/test.h>There was a problem hiding this comment.
I think to specify certain include directories we wanted to add an include function that sets up the right include path with the given prefix and is passed to the Interface, right?
There was a problem hiding this comment.
We possibly need to
- copy CXX and add any headers relative to the manifest dir to the header root (OUT/include) under crate name (so includes become crate/include/header.h)
- one of the functions allows for copying things from header root to exported headers (OUT/crate-include)
Note that we currently rename to eg cxx-qt-lib/qstring.h instead of cxx-qt-lib/include/core/qstring.h, however we now have the problem of how do these headers include other the common.h as within the build they are at cxx-qt-lib/include/common.h but when exported they are now at cxx-qt-lib/common.h. Should we instead not perform our renaming and force the developer to create a clean input ? For cxx-qt-lib this would mean you copy/write the headers into the structure you want inside the OUT dir and then manually add that to CxxQtBuilder rather than using the automatic relative manifest include. For other crates that don't need this functionality they can use the automatic relative manifest include.
There was a problem hiding this comment.
OK think i've sorted this out soooo
We now do the same as CXX with the best copy so any "header looking" files are copied from the source folder into the header root / include directory. So include/test.h ends up as crate/include/test.h.
If the export_include_directory is given a relative path then it is relative to the header root and is effectively copied across to the interface header directory.
This means that for cxx-qt/include/header.h these are automatically included and it only needs .export_include_directory("cxx-qt", "cxx-qt") to work, however note that now the path is cxx-qt/include/header.h rather than cxx-qt/header.h - but this then matches the same as CXX. This also works well in cxx-qt-lib-extras and in a similar way the include path has now changed.
For cxx-qt-lib we need to decide
- do we want to copy CXX include paths?
- should we just export all headers ignoring the features enabled?
If the answer is yes to both of these, then all the header writing/copying code in cxx-qt-lib can be removed and it becomes very simple.
If the answer is no to either/both of these, then we have to do the manual writing like currently in this code. And the way i've made this currently work is be passing an absolute path to export_include_directory which is pointing to the curated directory.
However i do see value in just simplifying this and copying CXX so that the include path patterns are very similar, this would mean a break with 0.8 if we went that route though or maybe we take that opportunity to have a single header you need to include instead like cxx-qt-lib/include/qtcore.h rather than a header for each type. @LeonMatthesKDAB what do we think?
ahayzen-kdab
force-pushed
the
1125-build-system-changes
branch
from
4bdf78f to
16f9f04
Compare
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.
ahayzen-kdab
force-pushed
the
1125-build-system-changes
branch
2 times, most recently
from
a113ad7 to
e2dcee0
Compare
ahayzen-kdab
force-pushed
the
1125-build-system-changes
branch
from
e2dcee0 to
2418af1
Compare
This is the first step towards #1125 towards a builder pattern with various stages.