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

README.md: fix build/run instructions #250

Merged
merged 5 commits into from
Apr 25, 2024
Merged

Conversation

vlkale
Copy link
Contributor

@vlkale vlkale commented Apr 16, 2024

Revise to improve build and run instructions. Specifically, the current build instructions in the general usage section have the wrong path and it also is inconsistent (outdated) w.r.t. the detailed build instructions. This is based on a user discussion on Slack.

The fix here is to consolidate general usage and build and run instructions to remove and put that information at the top.

The current build instructions in the general usage section have the wrong path.

Fix build and run instructions, put information at the top.
@vlkale vlkale self-assigned this Apr 16, 2024
Given your tool shared library `<name_of_tool_shared_library>.so` (which contains kokkos profiling callback functions) and an application executable called yourApplication.exe, type:

`export KOKKOS_TOOLS_LIBS=${YOUR_KOKKOS_TOOLS_DIR}/<name_of_tool_shared_lib>; ./yourApplication.exe`

Copy link
Member

Choose a reason for hiding this comment

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

For those who also wondered what changed:

--- before
+++ after
@@ -1,19 +1,27 @@
-## Using cmake
+## Using cmake

+### Build
+
 1. create a build directory in Kokkos Tools, e.g., type `mkdir myBuild; cd myBuild`
-2. To configure the Type `ccmake ..`  for any options you would like to enable/disable.
+2. To configure the Type `ccmake .. -DCMAKE_INSTALL_PREFIX=`  for any options you would like to enable/disable.
 3. To compile, type `make`
 4. To install, type `make install`

-## Using make
+### Run

-To build with make, simply type `make` within each subdirectory of Kokkos Tools.
+Given your installed tool shared library `lib<name_of_tool_shared_lib>.so` and an application executable called yourApplication.exe, type:

+`export KOKKOS_TOOLS_LIBS=${YOUR_KOKKOS_TOOLS_INSTALL_DIR}/lib<name_of_tool_shared_lib>.so; ./yourApplication.exe`

-Building using `make` is currently recommended. Eventually, the preferred method of building will be `cmake`.

-# Running a Kokkos-based Application with a tool
+## Using make

-Given your tool shared library `<name_of_tool_shared_library>.so` (which contains kokkos profiling callback functions) and an application executable called yourApplication.exe, type:
+### Build

-`export KOKKOS_TOOLS_LIBS=${YOUR_KOKKOS_TOOLS_DIR}/<name_of_tool_shared_lib>; ./yourApplication.exe`
+To build some library `<name_of_tool_shared_lib>` with make, simply type `make` within that library's subdirectory `${YOUR_KOKKOS_TOOLS_LIB_SRC_DIR}` of Kokkos Tools. This generate the shared library within that subdirectory.
+
+### Run
+
+Given your installed tool shared library `<name_of_tool_shared_lib>.so` and an application executable called yourApplication.exe, type:
+
+`export KOKKOS_TOOLS_LIBS=${YOUR_KOKKOS_TOOLS_LIB_SRC_DIR}/<name_of_tool_shared_lib>.so; ./yourApplication.exe`

When you make changes and craft commits, please think about the audit-ability to help reviewers.

Copy link
Member

Choose a reason for hiding this comment

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

Don't add trailing whitespaces (after the cmake header for instance)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, got it and thanks.

I will see if I can make the changes clean.

README.md Outdated
### Build

1. create a build directory in Kokkos Tools, e.g., type `mkdir myBuild; cd myBuild`
2. To configure the Type `ccmake .. -DCMAKE_INSTALL_PREFIX=` for any options you would like to enable/disable.
Copy link
Member

Choose a reason for hiding this comment

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

???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, thanks - I intended to put here:

'ccmake .. -DCMAKE_INSTALL_PREFIX='${YOUR_KOKKOS_TOOLS_INSTALL_DIR}'

By the way, just doing ccmake .. and not putting the install prefix may still work (by luck) on some platforms/environments, but we ought to highly recommended Kokkos Tools users to not do this. Maybe we can go into that detail in more detailed documentation in a section like "Dos and Don'ts".

Copy link
Member

Choose a reason for hiding this comment

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

I was not going to get into it because it is outside the scope of the PR but using ccmake isn't exactly the first thing we should showcase. It may be mentioned as a possibly convenient way to discover available options but a plain vanilla build (invoke cmake without any extra option besides installation prefix) will be just right in the vast majority of cases. The ccmake interface will overwhelm beginners and introduce unnecessary cognitive load for new users.

Copy link
Member

Choose a reason for hiding this comment

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

I am NOT suggesting you fix it here (please don't) just pointing out this is a poor choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it

@vlkale vlkale marked this pull request as draft April 16, 2024 22:53
@vlkale vlkale marked this pull request as ready for review April 18, 2024 03:00
@vlkale
Copy link
Contributor Author

vlkale commented Apr 18, 2024

@dalg24 I just now have fixed the missing install directory in the build instructions and to make it auditable. I think no other unrelated changes are in this PR. Note that the only change should be in the build and run instructions, where I have pulled up the build instructions I had at the top and just removed the build instructions with kp_memory_events.

This PR is using a branch on Kokkos Tools. In principle, I should be creating a branch on my fork to merge. However, I want this branch to be visible to users since it is a quick but useful fix and so they know the README is currently being updated.

@vlkale vlkale requested a review from crtrott April 18, 2024 03:15
@vlkale
Copy link
Contributor Author

vlkale commented Apr 18, 2024

@masterleinad Please feel free to comment here also.

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.

What exactly is the fix here? What was broken?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines +24 to +28
### Run

Given your installed tool shared library `lib<name_of_tool_shared_lib>.so` and an application executable called yourApplication.exe, type:

`export KOKKOS_TOOLS_LIBS=${YOUR_KOKKOS_TOOLS_INSTALL_DIR}/lib<name_of_tool_shared_lib>.so; ./yourApplication.exe`
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no reason to distinguish between Makefiles and CMake for running.

Suggested change
### Run
Given your installed tool shared library `lib<name_of_tool_shared_lib>.so` and an application executable called yourApplication.exe, type:
`export KOKKOS_TOOLS_LIBS=${YOUR_KOKKOS_TOOLS_INSTALL_DIR}/lib<name_of_tool_shared_lib>.so; ./yourApplication.exe`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a subtle difference. Maybe that should be changed in the build, but that's another PR.

The difference is that Makefiles generate <name_of_tool_shared_lib>.so in the source directory of the tool connector, while the cmake generates a library lib<name_of_tool_shared_lib>.so in the install directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Homogenizing the library names sounds like worth fixing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Homogenizing the library names sounds like worth fixing.

I agree with you and thanks for that feedback.

This would be a separate PR and is outside the scope of this PR (if you disagree, let me know).

Note the updated and more specific goal of this PR that I put in the description, so it is clear what needs to be fixed in this PR.

vlkale and others added 2 commits April 18, 2024 07:48
@vlkale
Copy link
Contributor Author

vlkale commented Apr 18, 2024

What exactly is the fix here? What was broken?

See edited description as noted in another reply to your review.

@vlkale
Copy link
Contributor Author

vlkale commented Apr 24, 2024

Hi, just pinging here.

I have addressed the comments to this PR and all is done from my end. Please let me know if this can reviewed and merged if it looks good to you, and if possible by Thursday. Thank you!

Co-authored-by: Daniel Arndt <[email protected]>
@crtrott crtrott merged commit dd39767 into develop Apr 25, 2024
14 checks passed
@vlkale vlkale deleted the README-buildrun-updates branch May 8, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants