Skip to content

examples: add a unit test to ensure that qrc works #981

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

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

ahayzen-kdab
Copy link
Collaborator

No description provided.

@LeonMatthesKDAB
Copy link
Collaborator

Hm, I wonder whether we even want to support QRC files this way in a CMake-driven build.

There is really little reason to do this over simply adding the qrc file to the CMake build 🤔

This should definitely work if you're running a Cargo-only build, but maybe we shouldn't support this in CMake-based builds?

@ahayzen-kdab
Copy link
Collaborator Author

I think it'd be tricky to explicitly disable for CMake builds and in theory shouldn't really be any different between CMake or Cargo builds as seen in the diff here.

For the object files the initialiser can just go into the crate's general initialiser so that we don't need to know the qrc name at the figure stage?

@LeonMatthesKDAB
Copy link
Collaborator

That would work, as long as the qrc call happens in the build script of the final crate.
However, it wouldn't work for libraries, unless we somehow add this to the opts as well.
Which would definitely require a different API, as you couldn't just call qrc on the builder 🤔

@ahayzen-kdab
Copy link
Collaborator Author

Hmm fun, but that is also true for cargo builds too? Any qrc's added in cxx-qt-lib would need an initialiser to be passed through via opts to the final build script?

@LeonMatthesKDAB
Copy link
Collaborator

Cargo builds should work, even with dependencies as I'm adding the object file generated by each qrc file to the linker if it's a cargo-only build.

But I also haven't tested that yet.

Copy link
Collaborator

@LeonMatthesKDAB LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

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

more test, more better, and this should be doable to support in our upcoming changes in #986

@LeonMatthesKDAB LeonMatthesKDAB enabled auto-merge (rebase) July 8, 2024 08:48
@LeonMatthesKDAB LeonMatthesKDAB merged commit cb71242 into KDAB:main Jul 8, 2024
10 checks passed
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