White-Oak / qml-rust

QML (Qt Quick) bindings for Rust language
MIT License
205 stars 18 forks source link

Bugs in the resources (qrc) example #12

Open rustonaut opened 7 years ago

rustonaut commented 7 years ago

Hy,

TL;TR: see bold text ;)

ther are a number of Bugs in the resource example:

  1. the sh build_lib.sh command is run in the repos root, not the resource dir, so it will fail to run at all
  2. the ExitCode of Command is not checked, through which bug 1 is ignored. (It is only checked, if there is no io::Error while executing it with .output
  3. "cargo:rerun-if-changed={}" does not automatically check all contents of a targeted dir, so you have to emit it for the .qrc file and all files it refers to.

There are also some think, which could be improved:

  1. place build artifacts into the target dir, not the resource folder making cargo clean work as expected
  2. add TARGET=qrc to the .pro file directly generating a libqrc.a (expect if there is a good reason to not do so??)
  3. ("factor out" the unsafe code in main.rs, or at last add some doc strings, so that people new to rust don't stumble over it)

Finally there might be some conceptual problems:

  1. the code registering the resource to the Qt resource manager might get "optimized away"

Originally I did only found point 1 & 2 and wanted to just make a small pull request, but given that the amount of codes does increas by quite a bit if you handle all bugs and possible improvements. As well as the conceptual problem and the fact that there are some "nice to have"s (like automatically generate a .qrc file for all entries in a folder) I decided to write a build-qrc crate. This can than be added as a build dependency providing some simple utilities for you build.rs files. Also I might provide some more "advanced" features too.

The will be an explanation of the "conceptual" problem at the end.

From here on I think there are two paths for the example:

  1. change it to use the build-qrc (or how ever I call it) crate when I push it's first version to cargo (I would make a pull request for it)
  2. decide not to use build-rc and I will pullrequest-push a minimal fixed version

It's up to you to decide :=)

To come back the the "conceptual problem": When generating libqrc.a the Qt's rcc fill generate a cpp file containing some constant arrays containing the data as well as a function which does register it (and one to de register it). It will also add a class instantiated once, which does call the registration on construction and the cleanup function with it's destructor. The problem with this is, that in no point in the "normal" program there is a reference to this code/file or even archive, giving the compiler the impression it can optimize it away. To be honest I'm not sure if this can or might apply in this case, but when trying it out it works, through I fear it just happend to work.

In Qt this is prevented by using the Q_INIT_RESOURCE(<name>) macro in the programm which uses the resources. It will add the needed reference. The code produced by rcc for initialization in the library is roughly:

namespace {
   struct initializer {
       initializer() { QT_RCC_MANGLE_NAMESPACE(qInitResources_tom)(); }
       ~initializer() { QT_RCC_MANGLE_NAMESPACE(qCleanupResources_tom)(); }
   } dummy;
}

The code used by the Q_INIT_RESOURCE(<name>) macro is irrelevant (some do { call_that_init() } while(0)).

Note that this problem mainly hits programs which use libraries which have some qrc resources.

rustonaut commented 7 years ago

The github repo is here: https://github.com/dathinab/build-qrc

White-Oak commented 7 years ago

@dathinab oh, man, thanks for the great write-up! On points:

  1. bugs in the resource example:
    1. Yes, I've recently noticed that as well, but forgot to fix :(
    2. and iii as well: these are actually also true for the main crate, 'm gonna address it later in the answer
  2. Things to improve:
    1. This is actually a great idea! I'm gonna do it for the main crate as well -- DOtherSide is being compiled in a root of the crate, which is not really good. Being placed in target is so much better.
    2. Whoops, I just didn't know of a way to do it, so came up with a terrible hack of renaming the library file. Big thanks :)
    3. This was mainly to try to thread quitting from the app, shouldn't have left it in the example, but now that you mention it, it would be better to provide some helper methods to do this kinds of things. On the other side, I just can make QmlEngine impl Sync+Send, I'm just not sure how much is Qt's QmlEngine multithreaded to allow this.

All in all, such a crate with QoL features (such as autogenerating .qrc file) is definitely appreciated, go ahead and build it! However, I will still leave a fixed qrc example as an example of how it can be done in bare bones. Of course, the proper way will be through using qrc-build and it will be mentioned in docs etc.

Considering a conceptual problem: does it remains a possible trouble in an unoptimized build? Remember, that we build resources in debug mode, since it doesn't matter much (or does it?). Can linker later throw out unused code from linked parts? If yes, is there a way to handle this?

rustonaut commented 7 years ago

Currently it works in all builds, even if you enable link time optimization (lto).

The problem is that I know that people had problems with similar thinks when using C++, I mean that's why there is the QT_INIT_RESOURCES macro. So I can't say if it is just a C/C++ think, or a general think which just does not appear currently because some rust compiler details which might change. But I would guess it is the later.

rustonaut commented 7 years ago

Ok, it works only half:

If you use the pattern in a program it works, but if you use it in a library it does not.

For example: Change the resource example into a library. Crate some new rust crate (binary) and add the resource example as dependency. Then in the main call the resource examples main. Result:

QQmlApplicationEngine failed to load component
qrc:///qml/resources.qml:-1 File not found

So it works for the example, but I have to find a different solution for my build tool :smile:

White-Oak commented 7 years ago

@dathinab can I use your commit from here to fix local example or would you prefer to PR it yourself?

rustonaut commented 7 years ago

feel free to use it, some thinks got in my way so I probably won't be able to work on rust before monday