b-com-software-basis / xpcf Goto Github PK

PROBLEM
Currently when we want a property to be a boolean, we use an int type and a convention to code for boolean values.
This can create inconsistency if the convention change in different locations.

For example 0 means false, but do we allow only 1 as another value, or any int values other than zero means true?
Plus in the code this leads to things like if ( p == 0 ) which is less legible than if ( !p ), for example.

SUGGESTED SOLUTION
Accept strings as boolean and parse valid values?
This parser could accept values like (case insensitive): true/false, yes/no, 0/1, ...

<property name="myBoolPropName1" type="boolean" value="True"/>

How do I create a dll file of xpcf?

HOW TO REPRODUCE
Use it on the SolAR framework interfaces with a command line like:

~/.remaken/packages/linux-gcc/xpcf_grpc_gen/2.6.2/bin/x86_64/shared/release/xpcf_grpc_gen  --module_uuid a0f522d1-b70e-4d0f-ad78-84e78a9af6bf -v 1.0.0 -r SolARbuild@github -u https://github.com/SolarFramework/SolARFramework --database_dir /home/user/work/SolAR/core/build-SolARFramework-Desktop_Qt_5_15_2_GCC_64bit-Release/ --std c++1z --remove_comments_in_macro -g protobuf -o ~/work/SolAR/core/SolARFrameworkGRPCRemote/

EXPECTED BEHAVIOR
Project files are generated correctly and program compiles.

OBSERVED BEHAVIOR
Path in #include are too long, sometimes absolute, sometimes from home dir (this is the case with the previous command line, where path to interfaces is expressed from ~):

#include "work/SolAR/core/SolARFramework/interfaces/api/display/I2DOverlay.h"

instead of

#include "api/display/I2DOverlay.h"

IIRC, generated path are absolute if path to framework is expressed relatively (../[...]) or absolutely.

OS: linux Ubuntun 18.04
xpcf_grpc_gen 1.6.2
SolAR Framework: develop branch (1.0.0)

HOW TO REPRODUCE:
In configuration file declare a component like this:

<!-- top level module-->
<module uuid="<uuid module>" name="Module" [...]>
    <component uuid="<uuid A>" name="A">
        <interface uuid="125f2007-1bf9-421d-9367-fbdc1210d006" name="IComponentIntrospect" description="IComponentIntrospect"/>
        <interface uuid="<uuid IA>" name="IA"/>
    </component>
</module>
[...]
<module>
  <!-- component used by top level module -->
  <component uuid="<uuid B>" name="B" >
      <interface uuid="125f2007-1bf9-421d-9367-fbdc1210d006" name="IComponentIntrospect" description="IComponentIntrospect"/>
      <interface uuid="<uuid IB>" name="IB"/>
  </component>
<module>
[...]
<bindings>
    <bind interface="IB" to="B" scope="Singleton"/>
</bindings>

Resolve top level component with this code:

auto manager= xpcf::getComponentManagerInstance();
manager->load(conf_file_path);
component = manager->createComponent<IA>(xpcf::toUUID(<uuid A>))->bindTo<IA>();

EXPECTED BEHAVIOR
component is created

OBSERVED BEHAVIOR
InjectableNotFoudException is thrown
No [auto|default|named] binding found to resolve component from interface UUID = <uuid IB>

COMMENTS
In case this is hard to reproduce, you can try to use the SolAR Unity plugin
https://github.com/SolarFramework/SolARUnityPlugin

With PipelineManager submodule at this commit:
SolarFramework/SolARPipelineManager@b00e5c3

(do not take a more recent commit because a fix will be uploaded, so the bug won't be reproducible)

And try to start the SLAM pipeline sample from Unity.

This behavior is not observed if component is instead created with the following method:
component= manager->resolve<IA>();
Here, the component is created with no error.

When a property is not well set in the registery file (property does not exist, wrong type, no value or null/empty value, wrong value format such as a float with a coma, etc.) could it be possible to add an explicit message when the exception is thrown ?

According to the documentation, currently supported types for properties are:

[int, uint, long, ulong, string, wstring, float, double, structure]

I don't know if this is possible, but it would be nice to be able to handle enum types, so that all the required checks are performed by XPCF instead of doing this kind of things in every components:

• Get the enum value as a string in the configuration file
• Maintain an associative container / add a method that performs a switch to map the string value to the enum value and check the input string is among the valid values of the enum.

Maybe something like this MIT-licensed library could be used ?
https://github.com/Neargye/magic_enum

It can get the enum value out of a string

std::string color_name{"GREEN"};
auto color = magic_enum::enum_cast<Color>(color_name);
if (color.has_value()) {
  // color.value() -> Color::GREEN
}

With XPCF, a component declaring a property could then look like this

enum class Orientation { Up, Down};
...
Orientation m_orientation;
...
declareProperty<Orientation>("orientation", m_orientation); // raises a configuration error if the string value cannot be converted to an Orientation

In class A, we defined a member variable m_prop as follows:
class A {
size_t m_prop;
}
and in constructor of class A,
A::A():ConfigurableBase(xpcf::toUUID())
{
declareProperty("prop", m_prop);
}

Then in class B, we had an object of class A named as a.
When we do a->bindToxpcf::IConfigurable()->getProperty("prop")->getUnsignedIntegerValue(), it always returned 0, whatever the value of "prop" we put in the xml file.

However, when we replace size_t by uint32_t, everything works, we get the expected property value.
class A {
uint32_t m_prop;
}

I think maybe it is related to the implementation below:

It would suggest to log an error message, instead of return 0, when getUnsignedIntegerValue() is called to access the value of a property of type size_t ?

Thanks in advance

PROBLEM

• In an attempt to reduce warnings in SolARFramework, we encountered several warnings emitted by GCC for each reference to a non-standard attribute to say it will ignore it.
  Ex:

warning: ‘xpcf::clientUUID’ scoped attribute directive ignored [-Wattributes]

• Unfortunately, there is no support for GCC to filter attributes to ignore.
  Here's the documentation for GCC 7 (the one we currently use):

-Wno-attributes
Do not warn if an unexpected __attribute__ is used, such as unrecognized attributes, function attributes applied to variables, etc. This does not stop errors for incorrect use of supported attributes.

• We could use this (in fact, I think we'll use that as a best effort option), but the trouble is that it will silence too much.
  For exemple, a typo when writing the name of a known attribute such as [[maybe_unused]] (but in this case this will probably emit a warning about an unused parameter (for example), so it won't stay unnoticed).

In GCC12, it becomes possible to specify such filter (doc):

-Wno-attributes=xpcf::clientUUID

But we're not going to be using GCC12/13 anytime soon.

SUGGESTED SOLUTION

• Currently, XPCF wraps a set of attributes using macros in helpers.h. The definition of theses attributes can be set to nothing thanks the the SWIG flag.
• We could use such a mechanism with another flag (since it's not related to SWIG) so that we can get rid of the warnings by removing the use of these attributes, since the gRPC attributes are not necessary for all of our projects, but only for the one that uses XPCF to generates gRPC clients and services.
• I've made a test locally with this flag: XPCF_DISABLE_ATTRIBUTES
  and this change to helpers.h:

#if !defined(SWIG) && !defined(XPCF_DISABLE_ATTRIBUTES)

NOTES

• I'm openning this as a discussion. I'm afraid it would introduce changes that are more related to our project than to XPCF.
• If you think this need might be generalized, we could discussed a way to do it properly, otherwise, it might not be worth integrating this change as a minor workaround specific to our project.

How to inject two same components with different names in another component? For example, I would like to create ComponentA that injects two ComponentB as follows:
declareInjectable<InterfaceComponentB>(componentB1); declareInjectable<InterfaceComponentB>(componentB2, "B2");
And my configuration file:
<factory> <bindings> <bind interface="InterfaceComponentB" to="ComponentB""/> </bindings> <injects> <inject to="ComponentA"> <bind interface="InterfaceComponentB" to="ComponentB" name="B2"/> </inject> </injects> </factory>
Then I run my program to create ComponentA but it crashes when injecting the second ComponentB.
I found a not clear solution to resolve this problem by moving the bind of the second ComponentB in the bindings as follows:
<factory> <bindings> <bind interface="InterfaceComponentB" to="ComponentB""/> <bind interface="InterfaceComponentB" to="ComponentB" name="B2"/> </bindings> <injects> <inject to="ComponentA"> </inject> </injects> </factory>

HOW TO REPRODUCE

• Generate SolARFrameworkGRPCRemote with script, i.e. in a mode where the SolARFramework compilation database is generated. Command should be

xpcf_grpc_gen \
   --module_uuid <...> \
   --name SolARFramework \
   --project_version <...> \
   --repository SolARBuild@github \
   --url https://github.com/SolarFramework/SolARFramework/releases/download \
   --database_dir <path to compilation db dir> \
   --std c++1z \
   --remove_comments_in_macro \
   --generator protobuf\
   --interfaces_folder ../SolARFramework/interfaces/ \
   --output gen

• Compare with génération after parsing of single file IARDevice.h

xpcf_grpc_gen \
   --module_uuid <...> \
   --name SolARFramework \
   --project_version <...> \
   --repository SolARBuild@github \
   --url https://github.com/SolarFramework/SolARFramework/releases/download \
   --database_dir <path to compilation db dir> \
   --database_file <path to compilation db dir>/compile_commands.json \
   --file ../SolARFramework/interfaces/api/input/devices/IARDevice.h \
   --std c++1z --remove_comments_in_macro \
   --generator protobuf \
   --interfaces_folder ../SolARFramework/interfaces/ \
   --output gen

EXPECTED BEHAVIOR

• both commands generate a proto, and then gRPC service/clients for IARDevice

OBSERVED BEHAVIOR

• Only the generation from the complete database produces a result. When invoking xpcf_grpc_gen with a single file, no proto is generated and thus no gRPC clients/servers.

Attached to this issue, you'll find the complete log of the --file version as well as the matching section in the complete database version log.

Here's the diff:

1c1
< AST for '/home/jmhenaff/dev/SolAR/core/SolARFramework/interfaces/api/input/devices/IARDevice.h':
---
> AST for '../SolARFramework/interfaces/api/input/devices/IARDevice.h':
31,45d30
< parseMethods::Base name=org::bcom::xpcf::IComponentIntrospect
<  ==> parsing member function start ()
< => constness [ not const ]
<  => type [ SolAR::FrameworkReturnCode ]
<  => type kind [  ]
<  => derefed type [ Enum type ]
< SolAR::FrameworkReturnCode
<  ==> Method declaration start()
<  ==> parsing member function stop ()
< => constness [ not const ]
<  => type [ SolAR::FrameworkReturnCode ]
<  => type kind [  ]
<  => derefed type [ Enum type ]
< SolAR::FrameworkReturnCode
<  ==> Method declaration stop()
83d67
< Entity Transform3Df is a type alias
102c86
<  => derefed type [ Enum type ]
---
>  => derefed type [ User defined type ]
119a104
> Found user defined type = SolAR::api::input::devices::IARDevice
157c142,147
< : `static constexpr char const* const DESCRIPTION="SolAR::IARDevice interface";`
\ No newline at end of file
---
> : `static constexpr char const* const DESCRIPTION="SolAR::IARDevice interface";`
> Searching protobuf compiler (protoc) from path: 
> ===> /home/jmhenaff/.conan/data/protobuf/3.21.9/_/_/package/774d45d697ce6041148f1cc945854892effde458/bin/
> Using protobuf compiler (protoc) from path: 
> ===> /home/jmhenaff/.conan/data/protobuf/3.21.9/_/_/package/774d45d697ce6041148f1cc945854892effde458/bin/
> m_protoNameFilesPathMap.size()=0

NOTES
Also other part of code are generated differently, for instance:

return static_cast<SolAR::FrameworkReturnCode>(respOut.xpcfgrpcreturnvalue());

With the DB, becomes, with only one file:

return xpcf::deserialize<SolAR::FrameworkReturnCode>(respOut.xpcfgrpcreturnvalue());

IARDeviceParse-db.txt
IARDeviceParse-file.txt

HOW TO REPRODUCE

• run install.sh to install wizards
• open an existing module project in Qt
• right click on Sources, select add New -> XPCF -> XPCF Component class
• give it name with upper and lower case letters (e.g. MyComponent) (+ fill other fields according to documentation)
• click next to the Summary page and click Finish

EXPECTED BEHAVIOR
Summary displays file to be added as MyComponent.cpp and MyComponent.h
After Finish is clicked, these files are located in Sources folder.

OBSERVED BEHAVIOR
File names in Summary and files actually created only have lowercase characters in their names (mycomponent.cpp, mycomponent.h)

OS: Linux
XPCF: v 2.5.0
QtCreator: v5.0.2
Qt: v5.15.2

I recently cloned xpcf and tried to build it on MacOs with the build_xpcf.sh script within scripts/unixes.
The build script doesn't work anymore when simply launching ./build_xpcf.sh leading to "Project root path '5.15.2' doesn't exist".
The scripts seem to be broken since the d9404af commit (august 26), when the qmake_path parameter was added at the top level, but not correctly reflected in the underlying script build_remaken_project.sh where the QMAKE parameter was added in the middle of the parameters list.

My advices:

1. the new parameter must have a default value in the top level script
2. the new parameter should have been added at the end of the parameters list, to preserve backward compatibility and behavior
3. the top level build_xpcf.sh script must forward correctly the parameter to the underlying scripts
4. the usage documentation in build_remaken_project.sh must reflect the added parameter
5. To prevent from breaking working behavior on the master branch, someone should be in charge to lead the Software Quality of the product. Maybe going for systematic peer review before any push/merge on the master branch could prevent from frequent fixes/revert and time waste for successive erroneous commits.

HOW TO REPRODUCE

• Write a project configuration file with a module listing a non existant component

I don't know if it important but, my project had also this:

• Add a factory/bindings/bind element with a name for "properties"
• Add a properties/configure section with some properties
• resolve in application with:

result= xpcfCompMngr->resolve<MyNonExistant::Component::Interface>("<binding name>");

EXPECTED BEHAVIOR
An error message like:

Error: could not find component '<UUID and/or binding name and/or component name>' in module '<module UUID and/or module name> at <path to resolved module>'

Or anything else that allows to understand the component is missing, and the error has been handled.

OBSERVED BEHAVIOR

<executable name>: /home/jmhenaff/.conan/data/boost/1.74.0/_/_/package/9cb93833acae02c84075e30719a252a53c8b6ca7/include/boost/smart_ptr/shared_ptr.hpp:728: typename boost::detail::sp_member_access<T>::type boost::shared_ptr<T>::operator->() const [with T = org::bcom::xpcf::IComponentIntrospect; typename boost::detail::sp_member_access<T>::type = org::bcom::xpcf::IComponentIntrospect*]: Assertion `px != 0' failed.

And nothing else (also no xpcf::Exception seems to have been thrown)

Sorry I don't have an exact repro case (out of time, I just wanted to leave a trace here), let me know if you have some difficulties to reproduce it.

Thanks.

OS: Linux
XPCF version: 2.5.0
Configuration: debug

PROBLEM
Currently our configuration files tend to contain a comprehensive list of all available properties for all the components that are used.

This serves somehow as a documentation for the user, letting him know what are the available properties with their types.

This has a significant maintenance cost when the module components evolves (add/remove/rename properties).

But this is the only way for us to let the user know about the properties in a convenient manner, apart from documenting them in the component header file (which we do), or in a textual documentation shipped with the component.

All these solutions have the same maintenance issue.

SUGGESTED SOLUTIONS
It'd be nice if we could have only one source of truth, the source code.

It appears that the xpcfcli project does just that.

This could allow us to reduce the size of our configuration files, and give a user a way to update its configuration file without having access to the source (since the definition of the module properties is done in the .cpp file).

I gave it a try on on of our modules, but it crashed after having partially produced an xml output of the component list:

xpcfcli: ../../.conan/data/boost/1.78.0/_/_/package/9c097173c769786802e85151204758f1f62f461f/include/boost/smart_ptr/shared_ptr.hpp:786: typename boost::detail::sp_member_access<T>::type boost::shared_ptr<T>::operator->() const [with T = org::bcom::xpcf::IPropertyManager; typename boost::detail::sp_member_access<T>::type = org::bcom::xpcf::IPropertyManager*]: Assertion `px != 0' failed.
Aborted

So I guess this project might need an update.

We coud use it to dump a configuration file template for the all modules for example, filter components by name to get a template with only the one we intend to use in a given app, or find other usage to be determine.

Is it possible to add a .gitignore to the project root?

At least for :

• build
• *.pro.user

As I was removing warnings in our code base, I noticed one about ComponentBase::addInterface() being deprecated.
I had to look into the code to determine by what I could replace my call: addInterface() calls declareInterface().

• Could you add a comment in the attribute (e.g. [[deprecated("Use declareInterface() instead")]]) ? The method could also be removed but maybe you prefer to wait for a new major release to break this API ?

• Also, the assert messages in declareInterface() are not up-to-date:

"Type passed to addInterface is not an interface "
[...]
"Interface type passed to addInterface is not a derived class of IComponentIntrospect !!"

Either use the new declareInterface() name or both, until addInterace() is removed ?

• avoid unexplicit merge commits on master (Merge develop is not an explicit commit). Merge commits on master must contain the synthesis of the changelog occured between commits on master
• update CHANGELOG.md to reflect this information (not only update the xpcf version). MAINTAIN THE CHANGELOG.
• latest commit[a3a1ee6] removes exception handling/rewrapping: exception handling and error handling were made ON PURPOSE. Exceptions are wrapped in more specialized exceptions at will, removing the wrapping prevent to categorize exceptions from the user point of view (see Factory removal of such wrapping). Moreover no documentation about newly raised exceptions is added in the XPCF concerned headers. Update documentation in headers accordingly.
• Commits messages must reflect the scope of their changes: for instance ef13dc9 "fix(update): grpc generators update" not only modifies the grpc generator, but also changes the exception handling in xpcf factory which is sensitive. There is no relation between the commit message and the exception handling/rewrapping in xpcf Factory.

Maybe there must be a reviewing process before anything is merged in master with adequate people to validate changes.

PROBLEM

• Our project contains quite a lot of XPCF components subject to have gRPC files being generated by xpcf_grpc_gen.
• We want to generate those files only for a subset of these classes
• We must use XPCF_IGNORE macro for a majority of component and keep XPCF_CLIENTUUID/XPCF_SERVERUUID only for a handful of classes, because, IIUC, unannotated class uses generated UUIDs :

ProxyGenerator.cpp

xpcf::uuids::uuid proxyUUID = c->getClientUUID();
    if (proxyUUID.is_nil()) {
        proxyUUID = c->createClientUUID();
        m_serviceUuidMap[m_className] = xpcf::uuids::to_string(proxyUUID);
    }

• => this can be tedious and hard to check we haven't forgotten to add an XPCF_IGNORE somewhere as it will likely generate files we do not use, and only increase compilation time to our project that uses those generated files.

SUGGESTED SOLUTION

• Invert the default, and consider an unannotated class to be ignored, and explicitely add annotation for generation.

For example:

• set both UUIDs

class XPCF_CLIENTUUID("...") XPCF_SERVERUUID("...") MyClass:
    virtual public org::bcom::xpcf::IComponentIntrospect {

• set only one UUID, let the other one be generated (error if not both are set?)

class XPCF_CLIENTUUID("...") XPCF_SERVERUUID_AUTO MyClass:
    virtual public org::bcom::xpcf::IComponentIntrospect {

• no annotation -> ignored

class MyClass:
    virtual public org::bcom::xpcf::IComponentIntrospect {

HOW TO REPRODUCE
Use it on the SolAR framework interfaces with a command line like:

~/.remaken/packages/linux-gcc/xpcf_grpc_gen/2.6.2/bin/x86_64/shared/release/xpcf_grpc_gen  --module_uuid a0f522d1-b70e-4d0f-ad78-84e78a9af6bf -v 1.0.0 -r SolARbuild@github -u https://github.com/SolarFramework/SolARFramework --database_dir /home/user/work/SolAR/core/build-SolARFramework-Desktop_Qt_5_15_2_GCC_64bit-Release/ --std c++1z --remove_comments_in_macro -g protobuf -o ~/work/SolAR/core/SolARFrameworkGRPCRemote/

EXPECTED BEHAVIOR
In the output dir, which is the directory of the SolARGRPCFrameworkRemote project in the example, packagedependencies.txt stays the same and is OK, i.e.:

xpcf|2.6.2|xpcf|@github|https://github.com/b-com-software-basis/xpcf/releases/download|shared|
SolARFramework|1.0.0|SolARFramework|SolARBuild@github|https://github.com/SolarFramework/SolARFramework/releases/download

OBSERVED BEHAVIOR
File packagedependencies.txt is overwritten and becomes

xpcf|2.6.2|xpcf|@github|https://github.com/SolarFramework/binaries/releases/download

Which is wrong, since XPCF 2.6.2 is not present at this location.

HOW TO REPRODUCE

1. Get project and generate a compilation database that contains -isystem

• git clone https://github.com/SolarFramework/SolARFramework.git -b develop && cd SolARFramework && git checkout be919630e0b6d1192f4278707bdf78453a4ec48e
• Open QtCreator and open SolARFramework.pro
• Go to projects tab, and select run for qmake_system()
• Build project and then generate compilation database: Build -> Run Generator -> Generate Compilation Database
• The location of the generated compilation database should be indicated in the Genera Messages window. This should be something like ../build-SolARFramework-Desktop_Qt_6_5_3_GCC_64bit-Release/compile_commands.json

The compilation database should be ill-formed as long as the issue b-com-software-basis/builddefs-qmake#13 is not resolved.
To reproduce this bug, you must manually alter the JSON file by replacing -I-isystem/ string by -system/.

2. Get project that uses XPCF-gRPC-gen on the previous project.

• git clone https://github.com/SolarFramework/SolARFrameworkGRPCRemote.git -b develop && cd SolARFrameworkGRPCRemote && git checkout 8ca88680ec599d97d3d4d0c51cd69bb5072689ca
• run generate.sh

EXPECTED BEHAVIOR
Script exits with code 0.

OBSERVED BEHAVIOR

[simple file parser] [info] parsing file '/home/user/dev/SolAR/core/SolARFramework/src/api/reloc/IKeyframeRetriever.cpp'
[preprocessor] [error] In file included from /home/user/dev/SolAR/core/SolARFramework/src/api/reloc/IKeyframeRetriever.cpp: 17:
[preprocessor] [error] In file included from /home/user/dev/SolAR/core/SolARFramework/interfaces/api/reloc/IKeyframeRetriever.h: 20:
[preprocessor] [error] In file included from /home/user/dev/SolAR/core/SolARFramework/interfaces/datastructure/Keyframe.h: 22:
[preprocessor] [critical] /home/user/dev/SolAR/core/SolARFramework/interfaces/datastructure/GeometryDefinitions.h:22: 'xpcf/xpcf.h' file not found

NOTES

• XPCF headers directory is specified by the compilation database as

"-isystem/home/jmhenaff/.remaken/packages/linux-gcc/xpcf/2.7.0/interfaces",

after the manual find'n replace that removes the erroneous -I-isystem/ part.

• If all occurences of -isystem are replaced by -I, the project compiles successfully.

OS: Linux Ubuntu 22.04
XPCF: 2.7.0 (static)
remaken: 1.10.0
Qt Creator: 11.0.2
Qt: 6.4.3

After creating an XPCF component with the Wizard, at the top the cpp file, there's this statement:

template<> My::Component::Class * xpcf::ComponentFactory::createInstance<My::Component::Class>();

By looking at other components I saw that the equivalent of this call was made more compact with the use of a macro, e.g.:

XPCF_DEFINE_FACTORY_CREATE_INSTANCE(My::Component::Class)

If this is the recommended way to do it, it could be nice to make the wizard generate the call to the macro, in addition this leads to smaller boilerplate code.