search

btc-ag / service-idl

Xtext-based Service IDL (Interface Definition Language) and Code Generators for Protobuf, C++, Java and .NET
Eclipse Public License 2.0
8 stars 8 forks source link

#175 fixed #242

Closed yury-trofimov closed 5 years ago

yury-trofimov commented 5 years ago

please to review

sigiesec commented 5 years ago

The PR cannot be merged before all of these points have been solved.

sigiesec commented 5 years ago

Maybe the failing integration tests are due to inconsistent conan packages, since there have been several build failures on the CAB jenkins over the last days due to http://jira.e-konzern.de/browse/BOPINFRA-582

huttenlocher commented 5 years ago

Maybe the failing integration tests are due to inconsistent conan packages, since there have been several build failures on the CAB jenkins over the last days due to http://jira.e-konzern.de/browse/BOPINFRA-582

Probably yes, just now I checked out the master branch and tried to run the integration tests, same tests are failing also with "master". There is probably something wrong in the "lastest" package dependencies. I get linker errors in almost all projects for elementary classes like BTC::ServiceComm::API::InvalidReplyReceivedyException etc. which are correctly referenced in CMake files and not touched by last commits.

huttenlocher commented 5 years ago
  • As already mentioned above, the integration tests need to be extended to make use of the new feature.

Yes, we keep one eye on this, however we will not be able provide an integration test in this sprint. Instead, we will provide new integration tests along with new tests for #186; this is planned as Jira issue PRINS-4407. We need also to check whether all conditions are fulfilled, at least to clarify: does @yury-trofimov has permissions to commit to Bitbucket repository bitbucket.e-konzern.de/scm/btccabcom/serviceidl-integrationtests.git

sigiesec commented 5 years ago

The serviceidl-integrationtests now work again in general, however some test cases are still failing. This includes test cases that currently activate the ODB behaviour (which must be changed anyway as indicated above), apparently because they generate the PRINS encapsulation headers:

[ERROR<] Error when processing test case 'cpp-current', module 'None': 
[ERROR-] conan build (conan build ..) failed with exit code 1 and output: Project: Running build()
[ERROR-] D:\NLS\serviceidl-integrationtests\output-78\cpp\Infrastructure\ServiceHost\Demo\API\ExternalDBImpl\odb\entrytype.hxx:4:10: fatal error: modules/Commons/include/BeginPrinsModulesInclude.h: No such file or directory
[ERROR-]  #include "modules/Commons/include/BeginPrinsModulesInclude.h"
[ERROR-]           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[ERROR-] compilation terminated.
[ERROR-] 
[ERROR-] ----Running------
[ERROR-] > bin\protoc.exe --proto_path=D:\NLS\serviceidl-integrationtests\output-78\cpp --cpp_out="D:\NLS\serviceidl-integrationtests\output-78\cpp" D:\NLS\serviceidl-integrationtests\output-78\cpp\Infrastructure\ServiceHost\Demo\API\Protobuf\gen\DemoX.proto D:\NLS\serviceidl-integrationtests\output-78\cpp\Infrastructure\ServiceHost\Demo\API\Protobuf\gen\Types.proto
[ERROR-] -----------------
[ERROR-] 
[ERROR-] ----Running------
[ERROR-] > C:\.conan\r4c91d7j\1\bin\odb.exe --std c++14 -I C:\.conan\j6uv53n7\1\include -I C:\.conan\r4c91d7j\1\include -I C:\.conan\7b1t311t\1\include --multi-database dynamic --database common --database mssql --database oracle --generate-query --generate-prepared --generate-schema --schema-format embedded -x -Wno-unknown-pragmas -x -Wno-pragmas -x -Wno-literal-suffix -x -Wno-attributes --hxx-prologue "#include "traits.hxx"" --output-dir D:\NLS\serviceidl-integrationtests\output-78\cpp\Infrastructure\ServiceHost\Demo\API\ExternalDBImpl\odb D:\NLS\serviceidl-integrationtests\output-78\cpp\Infrastructure\ServiceHost\Demo\API\ExternalDBImpl\odb\entrytype.hxx
[ERROR-] -----------------
[ERROR-] ERROR: BTC.PRINS.Infrastructure.ServiceHost.Demo.API/0.1.0-unreleased-build.local@PROJECT: Error in build() method, line 52
[ERROR-]    self.generateODBFiles()
[ERROR-] while calling 'generateODBFiles', line 41
[ERROR-]    ' --multi-database dynamic --database common --database mssql --database oracle --generate-query --generate-prepared --generate-schema --schema-format embedded -x -Wno-unknown-pragmas -x -Wno-pragmas -x -Wno-literal-suffix -x -Wno-attributes --hxx-prologue "#include \"traits.hxx\"" --output-dir ' + odbdir + ' ' + odbdir + '\\entrytype.hxx')
[ERROR-]    ConanException: Error 1 while executing C:\.conan\r4c91d7j\1\bin\odb.exe --std c++14 -I C:\.conan\j6uv53n7\1\include -I C:\.conan\r4c91d7j\1\include -I C:\.conan\7b1t311t\1\include --multi-database dynamic --database common --database mssql --database oracle --generate-query --generate-prepared --generate-schema --schema-format embedded -x -Wno-unknown-pragmas -x -Wno-pragmas -x -Wno-literal-suffix -x -Wno-attributes --hxx-prologue "#include "traits.hxx"" --output-dir D:\NLS\serviceidl-integrationtests\output-78\cpp\Infrastructure\ServiceHost\Demo\API\ExternalDBImpl\odb D:\NLS\serviceidl-integrationtests\output-78\cpp\Infrastructure\ServiceHost\Demo\API\ExternalDBImpl\odb\entrytype.hxx
[ERROR-] 
[ERROR>]

The test case interface-query-raises-exception is also failing with a different problem:

[ERROR-] CMake Error at CMakeLists.txt:24 (include):
[ERROR-]   include could not find load file:
[ERROR-] 
[ERROR-]     D:/NLS/serviceidl-integrationtests/output-79/cpp/foo/ExternalDBImpl/build/make.cmakeset
huttenlocher commented 5 years ago

The serviceidl-integrationtests now work again in general, however some test cases are still failing.

Nice, we can then investigate those problems locally until everything is working. We will tackle this as soon as possible today.

huttenlocher commented 5 years ago

Now all integration tests are green. To address conditional ODB generation, I created the new issue #244, since this functionality goes beyond the scope of #175 (= "make ODB compatible with CMake" - this basic requirement is now implemented). To address extension of integration tests, we already created the issue PRINS-4407. So for our part, we consider this ticket as solved.

sigiesec commented 5 years ago

I am sorry, but it is not possible to merge this, since it changes the behaviour for existing uses. This is an implicit constraint of the compatibility of versions. In addition, integration tests for the new functionality are still missing, as mentioned above.

sigiesec commented 5 years ago

We could merge this PR, if you change the behaviour to disable ODB generation by default now, and address conditional activation (and integration tests) in a later PR.

huttenlocher commented 5 years ago

Done. By default, ODB-based projects will not be generated (so previous behavior is still guaranteed). I introduced a new command line option value for OPTION_PROJECT_SET to force ODB generation, if desired.

sigiesec commented 5 years ago

Thanks for restoring the default behaviour.

I rebased the branch and squashed some commits to conform with the contribution guidelines , and I will open a new PR for that branch on your behalf, and close this one.

However, the addition of integration tests must still be done in a timely manner, also for the changes of #238. Please provide these by December 14th, otherwise I consider reverting the changes in the master again, until the integration test extensions are provided. This is essential to avoid maintain the quality of the Service IDL generators. On the one hand, I cannot judge if your additions work without such tests, but what is even more important is that no one can judge with reasonable effort if future changes break these new features.

sigiesec commented 5 years ago

See #246