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

[JOSS REVIEW] Minor documentation needed for Windows install #600

Closed
adam-m-jcbs opened this issue Jul 23, 2020 · 2 comments · Fixed by #601
Closed

[JOSS REVIEW] Minor documentation needed for Windows install #600

adam-m-jcbs opened this issue Jul 23, 2020 · 2 comments · Fixed by #601
Assignees
Labels
plat:windows This is related to the Windows Operating system reg:documentation This is related to documentation.

Comments

@adam-m-jcbs
Copy link

adam-m-jcbs commented Jul 23, 2020

This issue is related to the a JOSS functionality review (#597 ).

I have successfully completed a simple (no CUDA/OMP) build and install of ginkgo in Windows 10 using cygwin. However, with both cygwin and my VS attempts I get an error when building that seems to indicate the build system isn't properly preparing the windows_shared_library directory and adding it to $PATH.

It's simple to manually add the directory to $PATH to complete the build and verify the install. However, to meet the functional claims users should be able to build and install without manual intervention.

In a cygwin terminal with all needed dependencies for a non-CUDA/OMP build, I clone the repository and execute the following in a build directory:

CC=gcc CXX=g++ cmake -G "Unix Makefiles" -DCMAKE_INSTALL_PREFIX='/home/adamm/ginkgo/joss_install' \
      -DGINKGO_BUILD_TESTS=ON -DGINKGO_BUILD_EXAMPLES=ON -DGINKGO_DOC_GENERATE_EXAMPLES=ON \
      -DGINKGO_BUILD_REFERENCE=ON -DGINKGO_BUILD_OMP=OFF -DGINKGO_BUILD_CUDA=OFF \
      -DCMAKE_CC_COMPILER=gcc  -DCMAKE_CXX_COMPILER=g++ -DCMAKE_CUDA_HOST_COMPILER=g++ \
      -DGINKGO_DOC_GENERATE_PDF=ON .. && make -j6

However, the build fails due to

[100%] Completed 'rapidjson_external'
make[2]: Leaving directory '/home/adamm/ginkgo/build/third_party/rapidjson/download'
[100%] Built target rapidjson_external
make[1]: Leaving directory '/home/adamm/ginkgo/build/third_party/rapidjson/download'
CMake Error at cmake/build_helpers.cmake:129 (message):
  Did not find this build in the environment variable PATH.  Please add
  /home/adamm/ginkgo/build/windows_shared_library into the environment
  variable PATH.
Call Stack (most recent call first):
  cmake/build_helpers.cmake:40 (ginkgo_check_shared_library)
  core/devices/CMakeLists.txt:3 (ginkgo_compile_features)
  core/devices/omp/CMakeLists.txt:1 (ginkgo_add_object_library)


-- Configuring incomplete, errors occurred!
See also "/home/adamm/ginkgo/build/CMakeFiles/CMakeOutput.log".
See also "/home/adamm/ginkgo/build/CMakeFiles/CMakeError.log".

By manually creating an empty windows_shared_library directory and adding it to $PATH and rebuilding, things work.

Would it be possible to automate this within the windows building process? Or have I missed something for a Windows install? I must admit, I'm not used to building in Windows =p .

I get a similar issue when playing with the build hooks that are triggered when cloning the repository within Visual Studio (I'm using VS 2019).

@yhmtsai
Copy link
Member

yhmtsai commented Jul 23, 2020

Yes, need to add the path manually in windows but do not need to create the windows_shared_library (cmake will create it).
windows does not has RPATH like Windows (Visual Studio can write a config to register the program in the regedit), so we need to add the library path into environment or put the library and program (possible ways mentioned in the following).
In my opinion, I wouldn't like to allow the program/test program to change the environment.
Thus, we put all libraries together and add the directory into environment. otherwise, it is hard to handle the path. (#347 (comment))

There are some possible ways to achieve the target:

  1. copy/link all libraries into each testing folder (ex. omp/test/matrix, core/test/matrix, cuda/test/solver).
  2. Also, put all testing/libraries into windows_shared_library.

To be honest, I don't like these ways because 1 libraries have tons of link/copy and 2 running a specific test need pretty long name like cuda_test_matrix_coo_kernels.exe which is different from linux's

we only mention it in the option GINKGO_WINDOWS_SHARED_LIBRARY_RELPATH in https://github.com/ginkgo-project/ginkgo/blob/develop/INSTALL.md and CMake Error message.

Thanks for point that, I will add the information in windows install section of README.md.

@yhmtsai yhmtsai added plat:windows This is related to the Windows Operating system reg:documentation This is related to documentation. labels Jul 23, 2020
@yhmtsai yhmtsai self-assigned this Jul 23, 2020
@adam-m-jcbs
Copy link
Author

Thanks for the further information and context!

As the build functions, I'll keep the issue open but refer to it in the documentation review. For functionality purposes, it's resolved.

It's a justifiable and safe design choice to avoid changing the environment in this context. So as long as this detail is documented as suggested by @yhmtsai , the Windows install will be in line with JOSS functionally and documentation review criteria.

@adam-m-jcbs adam-m-jcbs changed the title [JOSS REVIEW] Error with simple Windows install [JOSS REVIEW] Minor documentation needed for Windows install Jul 24, 2020
@yhmtsai yhmtsai linked a pull request Aug 4, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plat:windows This is related to the Windows Operating system reg:documentation This is related to documentation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants