compiler-research / CppInterOp

A Clang-based C++ Interoperability Library
Other
37 stars 21 forks source link

Update documentation setup configuration #99

Closed Krishna-13-cyber closed 1 year ago

vgvassilev commented 1 year ago

Why do we need llvm 10? CppInterOp will require some work to enable llvm 10 if possible at all.

Krishna-13-cyber commented 1 year ago

Why do we need llvm 10? CppInterOp will require some work to enable llvm 10 if possible at all.

The readthedocs supports llvm-version only till llvm-10 and also prior cmake versions.So I had to use these versions for readthedocs compatibility.

vgvassilev commented 1 year ago

Do we really need to build the codebase with llvm-10? I think if we specified the doxygen build flag we should probably ignore the llvm version checks as we could be only building the doxygen config.

davidlange6 commented 1 year ago

Where can we see this llvm-10 support? I don't immediately find it

On Jun 29, 2023, at 11:02 AM, Krishna Narayanan @.***> wrote:

Why do we need llvm 10? CppInterOp will require some work to enable llvm 10 if possible at all. The readthedocs supports llvm-version only till llvm-10 and also prior cmake versions.So I had to use these versions for readthedocs compatibilty. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.Message ID: @.***>

Krishna-13-cyber commented 1 year ago

Do we really need to build the codebase with llvm-10? I think if we specified the doxygen build flag we should probably ignore the llvm version checks as we could be only building the doxygen config.

We just need llvm to pass the cmake checks, it is nothing to do with docs generation. I will try to get some alter method for this

Krishna-13-cyber commented 1 year ago

Where can we see this llvm-10 support? I don't immediately find it

The llvm-10 support was working for clad but I had upgraded it for interop to llvm-14 but it didnt seem to work well. Yes, I found it too while building the docs in readthedocs build setup, I will just share a screen capture of build failure (llvm version incompatible). build

davidlange6 commented 1 year ago

whose cmake checks?

davidlange6 commented 1 year ago

ok, I see your graphic now..

davidlange6 commented 1 year ago

did you consider using conda to do the installs instead?

parth-07 commented 1 year ago

@vgvassilev @davidlange6 If I understand correctly, the pull request is not actually building CppInterOp in the readthedocs configuration. llvm-10 is only required to run CMake on the CppInterOp. CMake will generate documentation targets. These documentation targets are used to generate sphinx and doxygen docs.

I think if we specified the doxygen build flag we should probably ignore the llvm version checks as we could be only building the doxygen config.

Yes, this can be done. For this, we will have to add an option such as, CPPINTEROP_BUILD_ONLY_DOCS. This option can modify CMake to explicitly ignore all configurations related to building the project. But I don't think this option is required for this change, as we can use llvm-10 to bypass the CMake checks.

did you consider using conda to do the installs instead?

CppInterOp is not being built / installed. Only documentation is being built.

vgvassilev commented 1 year ago

@vgvassilev @davidlange6 If I understand correctly, the pull request is not actually building CppInterOp in the readthedocs configuration. llvm-10 is only required to run CMake on the CppInterOp. CMake will generate documentation targets. These documentation targets are used to generate sphinx and doxygen docs.

I think if we specified the doxygen build flag we should probably ignore the llvm version checks as we could be only building the doxygen config.

Yes, this can be done. For this, we will have to add an option such as, CPPINTEROP_BUILD_ONLY_DOCS. This option can modify CMake to explicitly ignore all configurations related to building the project. But I don't think this option is required for this change, as we can use llvm-10 to bypass the CMake checks.

did you consider using conda to do the installs instead?

CppInterOp is not being built / installed. Only documentation is being built.

Yes, but that change would make it look like we support llvm-10 which we do not. So users with llvm-10 might trigger a build which would then fail. That’s what I am trying to avoid here. The best would be to install the right llvm in the container. The second best would be to use a special cmake flag.

parth-07 commented 1 year ago

Yes, but that change would make it look like we support llvm-10 which we do not.

Oh, yes you are right.

The best would be to install the right llvm in the container. The second best would be to use a special cmake flag.

Yes, this seems right. @Krishna-13-cyber can you please add the special CMake flag for this if we cannot install right llvm versions?

Krishna-13-cyber commented 1 year ago

Yes, this seems right. @Krishna-13-cyber can you please add the special CMake flag for this if we cannot install right llvm versions?

Yes, I will get it done!

davidlange6 commented 1 year ago

"llvm-10 is only required to run CMake on the CppInterOp." - this doesn't make much sense to me. Whose limitation is this?

vgvassilev commented 1 year ago

"llvm-10 is only required to run CMake on the CppInterOp." - this doesn't make much sense to me. Whose limitation is this?

Apparently that's what's in the default container of the readthedocs infrastructure. So that limitation comes from there and I am not even convinced if that's a limitation per se. What we should not do is change our cmake files to seem that we support llvm-10 as that'd be incorrect.

davidlange6 commented 1 year ago

ok, I guess this is because build.image is basically obsolete. I'd suggest we follow build.os instead.. https://docs.readthedocs.io/en/stable/tutorial/#adding-a-configuration-file

Krishna-13-cyber commented 1 year ago

ok, I guess this is because build.image is basically obsolete. I'd suggest we follow build.os instead.. https://docs.readthedocs.io/en/stable/tutorial/#adding-a-configuration-file

I have used this approach and works perfect. Thanks!

vgvassilev commented 1 year ago

Can you configure your editor to remove trailing white space?

Krishna-13-cyber commented 1 year ago

Can you configure your editor to remove trailing white space?

Yes, I will do this but I don't see any trailing spaces here in this PR!