Skip to content

Conversation

@Teemperor
Copy link
Contributor

This header is directly included by TObject.h and directly including
it from some other header doesn't seem to be supported. As C++ modules
with submodule visibility simulate directl including each module header,
we seem to get some errors according to the comments in this header.

This patch removes it from the argument list of the
ROOT_GENERATE_DICTIONARY call which prevents it from getting directly
included in the Core C++ module. We can also remove it from the header
blacklist after this change.

The normal dictionary won't be affected by this. This header is anyway
not supposed to contain TVersionCheck.h but only TObject.h which will
still provide it in the right way.

@phsft-bot
Copy link

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, slc6/gcc62, ubuntu14/native, ubuntu14/native, windows10/vc15 with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

@Axel-Naumann
Copy link
Member

directly including it from some other header doesn't seem to be supported

That's to the best of my knowledge incorrect.

@Teemperor
Copy link
Contributor Author

@Axel-Naumann alright, then we have to see why the documentation seems to say that.

@vgvassilev Do you remember why we needed this? Is this still up to date?
https://github.com/root-project/root/blob/master/core/base/inc/TVersionCheck.h#L26

@vgvassilev
Copy link
Member

@Teemperor, that line may be obsolete. IIRC that was from the days when we were running without -fmodules-local-submodule-visibility.

@Teemperor Teemperor force-pushed the RemoveTVersionCheckModule branch from 34172e3 to 5202e46 Compare December 13, 2017 10:58
@phsft-bot
Copy link

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, slc6/gcc62, ubuntu14/native, ubuntu14/native, windows10/vc15 with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

@Teemperor Teemperor changed the title [cxxmodules] Remove TVersionCheck.h from Core C++ module [cxxmodules] Remove TVersionCheck.h from Core C++ module blacklist Dec 13, 2017
This header is supported in the C++ module, so we can remove it from
the blacklist.
@Teemperor
Copy link
Contributor Author

@phsft-bot build just on slc6/clang_gcc62 with flags -Dcxxmodules=Off -Druntime_cxxmodules=On -Dctest_test_exclude_none=on

@phsft-bot
Copy link

Starting build on slc6/clang_gcc62 with flags -Dvc=OFF -Dimt=ON -Dccache=ON -Dcxxmodules=Off -Druntime_cxxmodules=On -Dctest_test_exclude_none=on
How to customize builds

@phsft-bot
Copy link

Build failed on slc6/clang_gcc62.
See console output.

@Teemperor
Copy link
Contributor Author

@phsft-bot build just on slc6/clang_gcc62 with flags -Dcxxmodules=Off -Druntime_cxxmodules=On -Dctest_test_exclude_none=on

@phsft-bot
Copy link

Starting build on slc6/clang_gcc62 with flags -Dvc=OFF -Dimt=ON -Dccache=ON -Dcxxmodules=Off -Druntime_cxxmodules=On -Dctest_test_exclude_none=on
How to customize builds

@phsft-bot
Copy link

Build failed on slc6/clang_gcc62.
See console output.

@Teemperor
Copy link
Contributor Author

@phsft-bot build just on centos7/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on

@phsft-bot
Copy link

Starting build on centos7/gcc62 with flags -Dvc=OFF -Dimt=ON -Dccache=ON -Druntime_cxxmodules=On -Dctest_test_exclude_none=on
How to customize builds

@phsft-bot
Copy link

Build failed on centos7/gcc62.
See console output.

@Teemperor
Copy link
Contributor Author

@phsft-bot build just on slc6/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on

@phsft-bot
Copy link

Starting build on slc6/gcc62 with flags -Dvc=OFF -Dimt=ON -Dccache=ON -Druntime_cxxmodules=On -Dctest_test_exclude_none=on
How to customize builds

@Teemperor
Copy link
Contributor Author

@phsft-bot build just on slc6/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on -Droofit=Off

@phsft-bot
Copy link

Starting build on slc6/gcc62 with flags -Dvc=OFF -Dimt=ON -Dccache=ON -Druntime_cxxmodules=On -Dctest_test_exclude_none=on -Droofit=Off
How to customize builds

@Teemperor
Copy link
Contributor Author

@phsft-bot build just on slc6/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on -Droofit=Off

@phsft-bot
Copy link

Starting build on slc6/gcc62 with flags -Dvc=OFF -Dimt=ON -Dccache=ON -Druntime_cxxmodules=On -Dctest_test_exclude_none=on -Droofit=Off
How to customize builds

@phsft-bot
Copy link

Build failed on slc6/gcc62.
See console output.

Errors:

Warnings:

  • Warning in <GenerateModule>: warning: Couldn't find the following specified headers in the module Core:
  • Warning in <GenerateModule>: warning: Couldn't find the following specified headers in the module valarrayDict:
  • Warning in <GenerateModule>: warning: Couldn't find the following specified headers in the module listDict:
  • Warning in <GenerateModule>: warning: Couldn't find the following specified headers in the module unordered_setDict:
  • Warning in <GenerateModule>: warning: Couldn't find the following specified headers in the module unordered_multisetDict:
  • Warning in <GenerateModule>: warning: Couldn't find the following specified headers in the module multimap2Dict:
  • Warning in <GenerateModule>: warning: Couldn't find the following specified headers in the module map2Dict:
  • Warning in <GenerateModule>: warning: Couldn't find the following specified headers in the module mapDict:
  • Warning in <GenerateModule>: warning: Couldn't find the following specified headers in the module unordered_mapDict:
  • Warning in <GenerateModule>: warning: Couldn't find the following specified headers in the module setDict:

And 7 more

Failing tests:

And 23 more

Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

LGTM! I will open another PR removing the restriction I added some time ago.

@vgvassilev vgvassilev merged commit 2ec9ee8 into root-project:master Dec 14, 2017
@Teemperor Teemperor deleted the RemoveTVersionCheckModule branch December 19, 2017 08:39
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.

4 participants