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

Dont require sphinx to be installed #144

Merged
merged 3 commits into from
Sep 12, 2023
Merged

Dont require sphinx to be installed #144

merged 3 commits into from
Sep 12, 2023

Conversation

pfultz2
Copy link
Collaborator

@pfultz2 pfultz2 commented Sep 6, 2023

This should only be required when building the documentation. There is no reason to require it when users just wants to build the component.

Copy link
Collaborator

@cgmb cgmb left a comment

Choose a reason for hiding this comment

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

Good point. However, there should be a check added to rocm_add_sphinx_doc to ensure it was found before using the variable.

@pfultz2
Copy link
Collaborator Author

pfultz2 commented Sep 6, 2023

here should be a check added to rocm_add_sphinx_doc to ensure it was found before using the variable.

CMake should already error when the variable is set to not found. Either way, how would we write such an error?

@cgmb
Copy link
Collaborator

cgmb commented Sep 6, 2023

It's not directly related to this change, but I noticed a typo in the existing code where the option is named USE_DOXYGEN but the later code expects USES_DOXYGEN.

@cgmb
Copy link
Collaborator

cgmb commented Sep 6, 2023

CMake should already error when the variable is set to not found.

CMake won't emit an error. The error will occur at build time rather than at configuration time. Make will attempt to execute a program named SPHINX_EXECUTABLE-NOTFOUND, which will typically result in the error SPHINX_EXECUTABLE-NOTFOUND: Command not found.

I agree with you that including ROCMSphinxDoc shouldn't automatically cause sphinx to be required. However, allowing configuration to complete with a nonsense command for one of the targets doesn't seem right. I see two options:

  1. rocm_add_sphinx_doc could emit an error if(PARSE_USE_DOXYGEN AND NOT DOXYGEN_EXECUTABLE) or if(NOT SPHINX_EXECUTABLE). Users would have to guard calls to rocm_add_sphinx_doc with if(SPHINX_EXECUTABLE AND DOXYGEN_EXECUTABLE) if they set USE_DOXYGEN or just if(SPHINX_EXECUTABLE) if they do not use doxygen.
  2. rocm_add_sphinx_doc could return without doing anything if(PARSE_USE_DOXYGEN AND NOT DOXYGEN_EXECUTABLE) or if(NOT SPHINX_EXECUTABLE). Users would not need to guard calls to rocm_add_sphinx_doc, but the sphinx-${BUILDER} and ${PROJECT_NAME}-sphinx-${BUILDER} targets might not be created if the required tools are not available.

@pfultz2
Copy link
Collaborator Author

pfultz2 commented Sep 7, 2023

CMake won't emit an error. The error will occur at build time rather than at configuration time

We want the error to happen at build time not at configuration time.

Users would have to guard

Yea we dont want to require that. Unless a component has a CI that builds with a minimal dependencies(we dont have that), there nothing to enforce the users are adding the guard.

Users would not need to guard calls to rocm_add_sphinx_doc, but the sphinx-${BUILDER} and ${PROJECT_NAME}-sphinx-${BUILDER} targets might not be created if the required tools are not available.

Yea and I think SPHINX_EXECUTABLE-NOTFOUND: Command not found is much clearer to what when wrong then having it say *** No rule to make target 'sphinx-html'..

Also there would only be an error if calling make sphinx-html. Calling make doc will just silently not do anything, which is even worse.

@pfultz2
Copy link
Collaborator Author

pfultz2 commented Sep 7, 2023

Another idea, we could add the sphinx-${BUILDER} targets as add_custom_target(sphinx-${BUILDER} ${CMAKE_COMMAND} -E echo "Sphinx executable not found" COMMAND ${CMAKE_COMMAND} -E false).

@cgmb
Copy link
Collaborator

cgmb commented Sep 9, 2023

In the interest of time, maybe let's proceed with the parts that everyone is agreed on and defer error-handling improvements for later. If you roll the PR back to just 7d069fb and 99ab7b4, I'd approve. Those changes are necessary but the rest are merely nice to have.

@pfultz2
Copy link
Collaborator Author

pfultz2 commented Sep 12, 2023

Done!

@pfultz2 pfultz2 merged commit 5a34e72 into develop Sep 12, 2023
@pfultz2 pfultz2 deleted the require-sphinx branch September 12, 2023 18:50
@pramenku
Copy link

@lawruble13 @pfultz2

We need PR merge in https://github.com/RadeonOpenCompute/rocm-cmake/tree/release-staging/rocm-rel-6.0 branch too so that it will come to 6.0 Mainline.
please help.

lawruble13 pushed a commit to lawruble13/rocm-cmake that referenced this pull request Oct 11, 2023
* Dont require sphinx to be installed

* Dont require doxygen

* Fix doxygen variable name
lawruble13 pushed a commit to lawruble13/rocm-cmake that referenced this pull request Oct 11, 2023
* Dont require sphinx to be installed

* Dont require doxygen

* Fix doxygen variable name
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.

3 participants