Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows compilation #231

Merged
merged 39 commits into from Jun 7, 2022
Merged

Windows compilation #231

merged 39 commits into from Jun 7, 2022

Conversation

nkx111
Copy link
Member

@nkx111 nkx111 commented May 28, 2022

nkx111 Large: 621

Since ROOT 6.26/02 release the "preview" label on windows binary distribution is removed (https://root.cern/releases/release-62602/). So I guess it can now run normally on windows. I am trying to make REST also compilable on windows. There are several changes to the code to make compilation successful.

  1. Use dllimport and dllexport to the global variables if they are accessed outside the library. https://docs.microsoft.com/en-us/cpp/cpp/dllexport-dllimport?view=msvc-170. Avoid accessing static class member outside a library, because it will make things complex. Console::CompatibilityMode changed to REST_Display_CompatibilityMode
  2. several methods are reimplemented for windows, include: setenv(), usleep(), getpid().
  3. Array initialization with dynamic length is not available for msvc. Changed to vector. e.g. Double_t x[nHits]; --> vector<Double_t> x(nHits);
  4. std::filesystem::path() returns by default wstring instead of string. Need to add additional string conversion
  5. shared memory very different on windows. I just exclud several processes for windows compilation
  6. fork() is not available on windows. I removed corresponding functionalities.
  7. The compiled dll file shall be installed to the bin directory instead of lib
  8. colored output for windows console(cmd, powershell) implemented differently in TRestStringOutput
  9. tmp path is different on windows and unix. For windows is C:\Users\xxx\AppData\Local\Temp

Several other compilation-unrelated changes:

  1. REST string converter now uses typeid hash to identify classes.
  2. Since ROOT 6.26 the new web browser is used. REST has not be prepered for that. Hence we set Browser.Name to TRootBrowser during startup.
  3. class TRestDBEntryLogger is removed since it is not used
  4. add -I../include definition during cint generation. This fixes header not found problem if we remove the source file.
  5. added method REST_StringHelper::MatchString().

The good point to have REST running on windows lies in that:

  1. Makes the framework protable. By providing several exe and dll file we can package the release.
  2. Attracts users who wants to do just quick analysis and don't want to compile any code.
  3. Faster gui compared to x11 through ssh tunnel.
  4. Always good to have more compilation platforms to check code quality

@rest-for-physics/core_dev

image

@nkx111 nkx111 marked this pull request as ready for review May 30, 2022 12:47
@nkx111 nkx111 requested review from lobis and juanangp and removed request for lobis May 30, 2022 12:52
@jgalan
Copy link
Member

jgalan commented Jun 3, 2022

Ok, I forgot to switch to this branch

@jgalan
Copy link
Member

jgalan commented Jun 3, 2022

I got the following error when trying to compile axionlib

C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.32.31326\include\xmemory(680): warning C4244: 'initializing': conversion from 'const _Ty' to '_Objty', possible loss of data
          with
          [
              _Ty=Double_t
          ]
          and
          [
              _Objty=float
          ]
  C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.32.31326\include\xmemory(1655): note: see reference to function template instantiation 'void std::_Default_allocator_traits<_Alloc>::construct<_Ty,const double&>(_Alloc &,_Objty *const ,const double &)' being compiled
          with
          [
              _Alloc=std::allocator<Float_t>,
              _Ty=float,
              _Objty=float
          ]
  C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.32.31326\include\xmemory(1656): note: see reference to function template instantiation 'void std::_Default_allocator_traits<_Alloc>::construct<_Ty,const double&>(_Alloc &,_Objty *const ,const double &)' being compiled
          with
          [
              _Alloc=std::allocator<Float_t>,
              _Ty=float,
              _Objty=float
          ]
  C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.32.31326\include\xmemory(1695): note: see reference to function template instantiation 'void std::_Uninitialized_backout_al<_Alloc>::_Emplace_back<const double&>(const double &)' being compiled
          with
          [
              _Alloc=std::allocator<Float_t>
          ]
  C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.32.31326\include\xmemory(1695): note: see reference to function template instantiation 'void std::_Uninitialized_backout_al<_Alloc>::_Emplace_back<const double&>(const double &)' being compiled
          with
          [
              _Alloc=std::allocator<Float_t>
          ]

@jgalan
Copy link
Member

jgalan commented Jun 3, 2022

Ok, I managed to compile restRoot.exe but simply clicking restRoot.exe it fails to execute because it does not find ROOT libraries,

If I go to CMD, I launch thisroot.bat then I launch restRoot.exe and nothing coming out.

@nkx111
Copy link
Member Author

nkx111 commented Jun 4, 2022

Ok, I managed to compile restRoot.exe but simply clicking restRoot.exe it fails to execute because it does not find ROOT libraries

I guess you need to launch root.exe once, then the registration keys will be automatically filled, then the root libraries could be found. Or you can try to set env ROOTSYS to the path you extracted ROOT binary, in case it makes same to my configuration.

If I go to CMD, I launch thisroot.bat then I launch restRoot.exe and nothing coming out.

I see, this is also a problem which took me many time to solve. My solution is to switch the cmake build type to Release.

image

@jgalan
Copy link
Member

jgalan commented Jun 4, 2022

Nice! The trick of changing to Release did succeed. I run thisroot.bat then restRoot.exe and works!

Still, for the double-click root.exe opens the terminal normally, but then after I launch restRoot.exe using double click and a dialog appears saying it cannot find libCore and others.

@jgalan
Copy link
Member

jgalan commented Jun 4, 2022

However, I get an error when I try to instantiate a class.

restWin

@nkx111
Copy link
Member Author

nkx111 commented Jun 4, 2022

This seems to be ROOT interpreter problem. If you type #include <assert.h> in root prompt, there will be similar errors. When I looked deeper it turns out that corecrt.h has no #ifdef/#define preprocessor, which was then included multiple times, and cause the problem. Maybe ROOT interpreter cannot response correctly to #pragma once preprocessor.

It seems the only solution is to avoid including assert.h on windows. I have updated the code now

@nkx111
Copy link
Member Author

nkx111 commented Jun 4, 2022

However, I get an error when I try to instantiate a class.

restWin

why your text is always green?

@jgalan
Copy link
Member

jgalan commented Jun 4, 2022

Not sure, but it does not always happen.

@jgalan
Copy link
Member

jgalan commented Jun 4, 2022

Perhaps is related to the VStudio version

@nkx111 nkx111 requested a review from a team June 5, 2022 03:50
@nkx111
Copy link
Member Author

nkx111 commented Jun 7, 2022

hi @lobis @juanangp I think we should merge this PR quickly to solve the pipeline failing problem

@lobis
Copy link
Member

lobis commented Jun 7, 2022

hi @lobis @juanangp I think we should merge this PR quickly to solve the pipeline failing problem

Sure, please answer my review and I will approve.

CMakeLists.txt Outdated
@@ -98,7 +109,7 @@ if (${HASCXX20} MATCHES "yes")
elseif (${HASCXX17} MATCHES "yes")
set(CMAKE_CXX_STANDARD 17)
else ()
message(FATAL_ERROR "Minimum C++ standard version is 17,
message("Minimum C++ standard version is 17,
Copy link
Member

Choose a reason for hiding this comment

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

why is the error removed here? I think we should stop execution if C++ std is not 17.

Update: I added it back on a343179, since I think its a must have, we should not allow to compile REST with a ROOT compiled with C++11 or C++14

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing out. I agree to force using c++17-compiled ROOT. I removed this error because on windows we cannot use root-config to collect HASCXX17, which makes cmake failed on this line. I have excluded this check for windows.

@nkx111 nkx111 merged commit 12edac2 into master Jun 7, 2022
@lobis lobis deleted the windows-compile branch June 30, 2022 09:25
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.

None yet

4 participants