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

Documentation updates: touch up BUILD.md, add Spack.md #3187

Merged
merged 1 commit into from
Jul 22, 2020

Conversation

jjwilke
Copy link

@jjwilke jjwilke commented Jul 15, 2020

  • Adds some extra details based on user feedback in the build documentation
  • Fixes some missing/incorrect options in the BUILD.md
  • Moves the README from kokkos-spack into kokkos since kokkos-space will no longer be maintained

Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Consider using

```cmake
```
and
```bash
```
when appropriate

BUILD.md Outdated Show resolved Hide resolved
BUILD.md Outdated Show resolved Hide resolved
Spack.md Outdated Show resolved Hide resolved
Spack.md Outdated Show resolved Hide resolved
Spack.md Outdated Show resolved Hide resolved
Spack.md Outdated Show resolved Hide resolved
Spack.md Outdated
We can verify we have `+volta70` and `+wrapper`, e.g.

### Spack Environments
The encouraged method of using Spack is to use environments.
Copy link
Member

Choose a reason for hiding this comment

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

That sentence sound weird to me.

Copy link
Author

Choose a reason for hiding this comment

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

Reworded.

@jjwilke jjwilke force-pushed the doc-updates branch 3 times, most recently from 99e6091 to c82e505 Compare July 15, 2020 04:01
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

LGTM but I think another pair of eyes on this would not hurt.

BUILD.md Outdated Show resolved Hide resolved
````cmake
cmake_policy(SET CMP0074 NEW)
````
If building in-tree, there is no `find_package`. You can use `add_subdirectory(kokkos)` with the Kokkos source and again just link with `target_link_libraries(Kokkos::kokkos)`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would mention examples/cmake_build_installed and examples/cmake_build_in_tree here as examples.

Copy link
Author

Choose a reason for hiding this comment

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

Added. I had to clean up these examples as well (redundant with another PR, but we should fix the comments in these examples ASAP).

BUILD.md Outdated Show resolved Hide resolved
Spack.md Outdated
kokkos:
variants: +hip amd_gpu_arch=vega900
````
Note: there is currently no support for the `hipcc` compiler in Spack. Spack currently does not have rules for building the compiler nor can an already installe version be added to Spack as a valid compiler.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Note: there is currently no support for the `hipcc` compiler in Spack. Spack currently does not have rules for building the compiler nor can an already installe version be added to Spack as a valid compiler.
Note: there is currently no support for the `hipcc` compiler in Spack. Spack currently does not have rules for building the compiler nor can an already installed version be added to Spack as a valid compiler.

That's not entirely true. You could trick Spack into thinking that hipcc is a clang compiler. That's what I did when testing the spack module. I am not sure if we want to mention any of this at all.

Copy link
Author

Choose a reason for hiding this comment

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

I just took out that language for now. It's probably not even worth mentioning since hipcc is an advanced use case at this point.

### Spack Environments
The encouraged way to use Spack is with Spack environments.
Rather than installing packages one-at-a-time, you add packages to an environment.
After adding all packages, you concretize and install them all.
Copy link
Contributor

Choose a reason for hiding this comment

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

Spack.md Show resolved Hide resolved
Make sure you have downloaded [Spack](https://github.com/spack/spack) and added it to your path.
The easiest way to do this is often (depending on your SHELL):
````bash
> source spack/share/spack/setup-env.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

I think spack will complain if you don't also set SPACK_ROOT in your environment variables

Copy link
Author

Choose a reason for hiding this comment

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

Reworded to make this clearer. Thanks for pointing this out.

@jjwilke jjwilke force-pushed the doc-updates branch 2 times, most recently from d16bb17 to 180aebf Compare July 15, 2020 17:54
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Looks good to me. I don't care much in which pull request we clean the examples, though.

@jjwilke jjwilke added the Blocks Promotion Overview issue for release-blocking bugs label Jul 15, 2020
@jjwilke
Copy link
Author

jjwilke commented Jul 15, 2020

I'm adding BLOCKS PROMOTION to this. We should really fix these docs before next release.

@masterleinad
Copy link
Contributor

Since we changed example/build_cmake_in_tree and example/build_installed in #3135, we need some rebasing TLC here.

add_subdirectory(${Example_SOURCE_DIR}/../.. ${Example_BINARY_DIR}/kokkos)

add_executable(example cmake_example.cpp)
add_executable(example cmake_example.cpp foo.f)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
add_executable(example cmake_example.cpp foo.f)
add_executable(example cmake_example.cpp)

We don't use Fortran here anymore.

@crtrott crtrott merged commit f1add8f into kokkos:develop Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocks Promotion Overview issue for release-blocking bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants