search

tpaviot / oce

OpenCASCADE Community Edition (OCE): a community driven fork of the Open CASCADE library.
http://groups.google.com/group/oce-dev
GNU Lesser General Public License v2.1
808 stars 284 forks source link

Change source path in install rule to PROJECT_BINARY_DIR #708

Closed lvk88 closed 5 years ago

lvk88 commented 5 years ago

The cmake macro OCCT_CONFIGURE_AND_INSTALL assumes that the project is called "OCCT", which is not the case for OCE. Therefore, OCCT_BINARY_DIR is undefined and the install target was not able to copy files used by this macro. (e.g. custom.sh)

Without this change, make install fails with the following message:

CMake Error at cmake_install.cmake:8097 (file):
  file INSTALL cannot find "//custom_gcc_64.install.sh".

Tested on Ubuntu 16.04 with cmake 3.5.1

lvk88 commented 5 years ago

I see that travis was failing for the clang + pch builds. I committed a fix ( b9a541b ) that detects whether clang is used as a compiler and excludes the two culprit headers (stdint.h and inttypes.h) from the list of paths that cotire uses to compile the *_prefix.cxx file.

lvk88 commented 5 years ago

Ah, Travis was not merciful to me this time... The GCC build without PCH timed out unfortunately.

On my own fork, it passed though (see here )

tpaviot commented 5 years ago

@lvk88 thank you

lvk88 commented 5 years ago

Oh, I see travis is failing again on clang + pch with errors like this:

fatal error: file '/usr/include/string.h' has been modified since the
      precompiled header
      '/home/travis/build/tpaviot/oce/cmake-build/src/TKSTEP209/cotire/TKSTEP209_CXX_prefix.hxx.pch'
      was built

This is probably because of my changes... sorry! It is weird that it was not failing on my fork. I am checking it again.

lvk88 commented 5 years ago

Okay, I think I found the reason, but really don't get why it works like this. Basically, there are two different versions of Standard_TypeDef.hxx in the source tree. The two paths are:

oce/src/Standard/Standard_TypeDef.hxx
oce/inc/Standard_TypeDef.hxx

Now, line 22 is different for these two files. The hxx that is located in the inc folder has:

#if(defined(_MSC_VER) && (_MSC_VER < 1800))
...
#define PRIuPTR "u"
...
#endif

while the hxx that is located in the src goes like:

#if(defined(_MSC_VER) && (_MSC_VER < 1800)) or (defined(__clang__) && (__clang_major__ == 4))
...
#define PRIuPTR "u"
...
#endif

This leads to PRIuPTR being defined twice that generates a ton of warnings about double macro definitions and eventually the error appears. Now, the question is why do header files exist twice all around the place?

In any case, I could remove the preprocessor definition on clang, this will make the warnings and errors disappear.

lvk88 commented 5 years ago

Sorry if I keep spamming the topic... probably it's better if I document what I find. I had a look on what .hxx files differ in the inc/ folder and in the src/ folder and found out the following list:

Files ininc/BRepGProp_Gauss.hxx and insrc/BRepGProp_Gauss.hxx differ
Files ininc/BRepLib_CheckCurveOnSurface.hxx and insrc/BRepLib_CheckCurveOnSurface.hxx differ
Files ininc/BRepOffset_Inter2d.hxx and insrc/BRepOffset_Inter2d.hxx differ
Files ininc/HLRAlgo_PolyShellData.hxx and insrc/HLRAlgo_PolyShellData.hxx differ
Files ininc/IntTools.hxx and insrc/IntTools.hxx differ
Files ininc/IntTools_SurfaceRangeSampleMapHasher.hxx and insrc/IntTools_SurfaceRangeSampleMapHasher.hxx differ
Files ininc/NCollection_CellFilter.hxx and insrc/NCollection_CellFilter.hxx differ
Files ininc/NCollection_Vec2.hxx and insrc/NCollection_Vec2.hxx differ
Files ininc/NCollection_Vec3.hxx and insrc/NCollection_Vec3.hxx differ
Files ininc/NCollection_Vec4.hxx and insrc/NCollection_Vec4.hxx differ
Files ininc/ShapeConstruct_ProjectCurveOnSurface.hxx and insrc/ShapeConstruct_ProjectCurveOnSurface.hxx differ
Files ininc/Standard_CLocaleSentry.hxx and insrc/Standard_CLocaleSentry.hxx differ
Files ininc/Standard_TypeDef.hxx and insrc/Standard_TypeDef.hxx differ

Standard_TypeDef.hxx is one of them, which causes the clang build to fail then. Does anyone know why OpenCASCADE follows this pattern of duplicated headers? I thought that the comparison that happens when cmake is executed is exactly for this reason, to detect differing files. In any case, I will temporarily fix the clang issue and submit a different pull request.

Should we clean up all the differing files in a separate pull request?

a-betenev commented 5 years ago

Hello,

Note that OCCT does not store header files in inc folder in the source tree; they are stored in src only. Folder inc in OCE was filled in the past by the headers mostly generated from CDL, with some modifications indeed. Now this should not be necessary any more I suppose. If you are using OCCT CMake script (as it seems), then it  will create folder inc in the build folder at configuration step, filled by the headers either copied from src, or containing stub links to relevant files in src.

Besides, can you share your experience of using Cotire-generated PCH on Linux -- do they help accelerating the build? In my experiments in the past I have not seen essential improvements of the build time.

Andrey

On 23/11/2018 15:40, lvk88 wrote:

Sorry if I keep spamming the topic... probably it's better if I document what I find. I had a look on what .hxx files differ in the inc/ folder and in the src/ folder and found out the following list:

Files ininc/BRepGProp_Gauss.hxx and insrc/BRepGProp_Gauss.hxx differ Files ininc/BRepLib_CheckCurveOnSurface.hxx and insrc/BRepLib_CheckCurveOnSurface.hxx differ Files ininc/BRepOffset_Inter2d.hxx and insrc/BRepOffset_Inter2d.hxx differ Files ininc/HLRAlgo_PolyShellData.hxx and insrc/HLRAlgo_PolyShellData.hxx differ Files ininc/IntTools.hxx and insrc/IntTools.hxx differ Files ininc/IntTools_SurfaceRangeSampleMapHasher.hxx and insrc/IntTools_SurfaceRangeSampleMapHasher.hxx differ Files ininc/NCollection_CellFilter.hxx and insrc/NCollection_CellFilter.hxx differ Files ininc/NCollection_Vec2.hxx and insrc/NCollection_Vec2.hxx differ Files ininc/NCollection_Vec3.hxx and insrc/NCollection_Vec3.hxx differ Files ininc/NCollection_Vec4.hxx and insrc/NCollection_Vec4.hxx differ Files ininc/ShapeConstruct_ProjectCurveOnSurface.hxx and insrc/ShapeConstruct_ProjectCurveOnSurface.hxx differ Files ininc/Standard_CLocaleSentry.hxx and insrc/Standard_CLocaleSentry.hxx differ Files ininc/Standard_TypeDef.hxx and insrc/Standard_TypeDef.hxx differ

Standard_TypeDef.hxx is one of them, which causes the clang build to fail then. Does anyone know why OpenCASCADE follows this pattern of duplicated headers? I thought that the comparison that happens when cmake is executed is exactly for this reason, to detect differing files. In any case, I will temporarily fix the clang issue and submit a different pull request.

Should we clean up all the differing files in a separate pull request?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tpaviot/oce/pull/708#issuecomment-441231426, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNyShCCF4KhJ5pDhny24OWA1j8vW_9Uks5ux-yggaJpZM4Yst1L.

tpaviot commented 5 years ago

@a-betenev In the opencascade-7.2.0 tar.gz official source archive, there actually is a /inc full of thousands of .hxx files, I just checked. So the /inc folder of oce comes from upstream, I didn't change anything.

lvk88 commented 5 years ago

@tpaviot Yes, I am alspo pretty sure that the original opencascade sources are like that. I have always wondered why. In any case, it seems that at build time, the ones from the src folder are used.

@a-betenev I did some timings on a Ubuntu 16.04 system, Intel Xeon E31225@3.1GHz and 16GB RAM. OCCT without Draw was built, in Release, using with make -j4

With PCH Without PCH
GCC 5.4 12:32 15:49
Clang 5.0 11:09 19:09

These are the wall times, in seconds. The clang value is pretty surprising, I am not sure why it is that slow compared to GCC. With precompiled headers, they seem to be comparable. To me, using PCH looks like an acceptable speedup.

Edit: precompiled headers, not precomputed

a-betenev commented 5 years ago

Indeed there is an "inc" folder in the official source archive: these are copies of headers from "src" collected in one place to facilitate the build for people who do not want to use CMake. However the original source tree of OCCT (see Git repo at http://git.dev.opencascade.org/gitweb/?p=occt.git;a=tree) does not contain "inc".

On 24/11/2018 17:59, Thomas Paviot wrote:

@a-betenev https://github.com/a-betenev In the opencascade-7.2.0 tar.gz official source archive, there actually is a /inc full of thousands of .hxx files, I just checked. So the /inc folder of oce comes from upstream, I didn't change anything.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tpaviot/oce/pull/708#issuecomment-441373723, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNySmhuWoNTD-EUmJWuIG6Fyhmk8Vjxks5uyV68gaJpZM4Yst1L.

a-betenev commented 5 years ago

@lvk88: Thanks for sharing your timings, good to know it is useful with CLang too

lvk88 commented 5 years ago

Thanks for the clarification @a-betenev! @tpaviot : should I then submit a PR where the headers in inc and src are synchronized? Or is it better to get rid of inc completely then?

Edit: formatting

a-betenev commented 5 years ago

Headers located in subfolder "inc" of the source tree are not used by OCCT CMake build procedure (it creates folder "inc" in the build folder and populates it by copies of or references to headers from src). Thus unless your build process relies on presence of the "inc" folder in the source tree somehow, I would suggest removing it.

On 04/12/2018 09:32, lvk88 wrote:

Thanks for the clarification @a-betenev https://github.com/a-betenev! @tpaviot https://github.com/tpaviot : should I then submit a PR where the headers in

inc and src are synchronized? Or is it better to get rid of inc completely then?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tpaviot/oce/pull/708#issuecomment-443987540, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNySgwCC2RV1guEw9hSQX0kA83Rfbtbks5u1hbygaJpZM4Yst1L.

tpaviot commented 5 years ago

@a-betenev thanks for the information, I didn't know the /inc folder was dedicated to other build systems than cmake. Since oce only relies on cmake, I agree @lvk88 that the /inc folder should be removed, your PR is welcome