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

[Docs] Rewrite build documentation #510

Merged

Conversation

hjabird
Copy link
Contributor

@hjabird hjabird commented Jun 10, 2024

Description

This PR completely rewrites the build documentation in an attempt to make it easier to follow and less intimidating. It separates out the DPC++ and AdaptiveCpp builds, and also adds a section on using oneMKL with CMake.

contributes to the solution of #459

Copy link
Contributor

@Rbiessy Rbiessy left a comment

Choose a reason for hiding this comment

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

This looks great to me!

docs/building_the_project_with_adaptivecpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
@hjabird
Copy link
Contributor Author

hjabird commented Jun 11, 2024

html.zip

The rendered html for 90ce137 is contained in the attached .zip.

@vbm23
Copy link

vbm23 commented Jun 11, 2024

Since at least 2 approving reviews are required to merge this pull request and 1 done, @hjabird who would be the 2nd here?

@hjabird
Copy link
Contributor Author

hjabird commented Jun 11, 2024

Since at least 2 approving reviews are required to merge this pull request and 1 done, @hjabird who would be the 2nd here?

I hope that @dnhsieh-intel, @mkrainiuk, @vmalia or @akabalov will take some interest! I'm hoping for feedback from more than two people ideally however since this is a very public-facing PR.

Copy link
Contributor

@andrewtbarker andrewtbarker left a comment

Choose a reason for hiding this comment

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

Thank you for doing this, this will be a great improvement when it is done.

docs/building_the_project_with_adaptivecpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_adaptivecpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_adaptivecpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_adaptivecpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/consuming_with_cmake.rst Outdated Show resolved Hide resolved
docs/consuming_with_cmake.rst Outdated Show resolved Hide resolved
docs/consuming_with_cmake.rst Outdated Show resolved Hide resolved
docs/consuming_with_cmake.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@andrewtbarker andrewtbarker left a comment

Choose a reason for hiding this comment

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

Thank you for the great care and thoroughness in your response to my review. I'm looking forward to seeing these docs merged.

Copy link
Contributor

@dnhsieh-intel dnhsieh-intel left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! I still need some time to go through it.

Here are somethings I noticed:

  • Should we say "building oneMKL interface library", "building oneMKL interfaces library", or just "building oneMKL interfaces"?
  • For CMake options, we have yes/no, True/False, ON/OFF. I think it would be better to unify the usage.

docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
@hjabird
Copy link
Contributor Author

hjabird commented Jun 14, 2024

Here are somethings I noticed:

* Should we say "building oneMKL interface library", "building oneMKL interface**s** library", or just "building oneMKL interface**s**"?

* For CMake options, we have `yes/no`, `True/False`, `ON/OFF`. I think it would be better to unify the usage.
  1. TIL I've always called it the wrong thing! Where its been called more than just oneMKL (i.e. oneMKL Interface Library or similar), I've renamed it oneMKL Interfaces to match the name given in the Github landing page.

  2. I've unified everything on True/False.

Thank you very much for comments!

@hjabird hjabird force-pushed the hjab/rewrite_building_the_project branch from 73d3510 to e06b9b9 Compare June 14, 2024 16:38
@hjabird
Copy link
Contributor Author

hjabird commented Jun 14, 2024

I've rebased on some recent changes, so the rendered html now looks prettier:
html.zip

Copy link
Contributor

@dnhsieh-intel dnhsieh-intel left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Thanks again for the great work!

docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_the_project_with_dpcpp.rst Outdated Show resolved Hide resolved
docs/building_and_running_tests.rst Outdated Show resolved Hide resolved
docs/building_and_running_tests.rst Outdated Show resolved Hide resolved
docs/building_and_running_tests.rst Outdated Show resolved Hide resolved
@dnhsieh-intel
Copy link
Contributor

Ah, I forgot one thing. The "fixes: #459" in the description will close the issue automatically after this PR is merged. GitHub doesn't recognize partially fixes.

@hjabird
Copy link
Contributor Author

hjabird commented Jun 19, 2024

@dnhsieh-intel how is this looking to you? I'm running a tutorial tomorrow, so if you think its ready I'd be delighted if we got this merged.

@hjabird hjabird merged commit 69bad82 into uxlfoundation:develop Jun 20, 2024
8 checks passed
normallytangent pushed a commit to normallytangent/oneMKL that referenced this pull request Aug 6, 2024
* Rewrites build documentation
  * Splits documentation into several files
* Adds docs for using with CMake.
@hjabird hjabird mentioned this pull request Aug 7, 2024
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.

5 participants