Page MenuHomePhabricator

[libc++][doc] Use sphinx variables to make updating the docs version easier
Needs ReviewPublic

Authored by tstellar on Oct 15 2021, 10:58 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

tstellar requested review of this revision.Oct 15 2021, 10:58 PM
tstellar created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2021, 10:58 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
tstellar updated this revision to Diff 380142.Oct 15 2021, 11:00 PM

Add some more changes.

Mordante added inline comments.
libcxx/docs/ReleaseNotes.rst
13–16

Would it be possible to automatically remove this block, including the .. warning:: at line 11, when the version is a release?

libcxx/docs/conf.py
45–46

The current output "libc++ 14.0.0git documentation" looks weird.

45–46

I expect for a release I'd have to remove + 'git', but doing so results in errors when building the documentation.

make[3]: *** [projects/libcxx/docs/CMakeFiles/docs-libcxx-html.dir/build.make:58: projects/libcxx/docs/CMakeFiles/docs-libcxx-html] Error 2
make[2]: *** [CMakeFiles/Makefile2:25091: projects/libcxx/docs/CMakeFiles/docs-libcxx-html.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:25098: projects/libcxx/docs/CMakeFiles/docs-libcxx-html.dir/rule] Error 2
make: *** [Makefile:7544: docs-libcxx-html] Error 2

Do I misunderstand the intention or is there a real error?

tstellar updated this revision to Diff 380451.Oct 18 2021, 9:55 AM

Thanks for the feedback. Here is an updated version of the patch with
some of your suggestions.

tstellar marked an inline comment as done.Oct 18 2021, 9:57 AM
tstellar added inline comments.
libcxx/docs/conf.py
45–46

I decided to drop the 'git' suffix in order to simplify the config. I can add it back (with the '-' in front of it) if you think it helps remind readers that it is a pre-release.

tstellar updated this revision to Diff 380471.Oct 18 2021, 10:47 AM

I rewrote this patch to take variables directly from cmake, so we don't need to hard-code
them in the libcxx config file. With this change, Makefile.sphinx will stop working, so
I've deleted it, but I can add it back and fix it if people are still using it.

I plan to update the other sub-projects to take advantage of this new approach, once we're done
with libcxx.

Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2021, 10:47 AM
tstellar marked an inline comment as not done.Oct 18 2021, 10:55 AM
tstellar added inline comments.
libcxx/docs/conf.py
45–46

The version string is 14.0.0git again now that I'm using PACKAGE_VERSION directly from CMake. This form of the version string is used in other places, so I would lean towards just keeping it as is.

The intent of the change looks good to me, i.e. using variables to set the version number & al.

However, I checked out this patch and built the documentation, and it doesn't show "In Progress" anymore, nor does it have any mention of being the libc++ 14.0.0 incoming release. Is that intended? IMO, it was useful to have that visual marker about notes being in-progress release notes for an upcoming release.

If that's not intended, then I assume this is most likely caused by a difference in how we run our builds. What I'm using is:

$ git clone https://github.com/llvm/llvm-project.git
$ cd llvm-project
$ mkdir build
$ cmake -G Ninja -S runtimes -B build -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;libunwind" -DLLVM_ENABLE_SPHINX=ON
$ ninja -C build docs-libcxx-html
libcxx/docs/Makefile.sphinx
1

This is fine - I don't think there's any desire to keep this around, at least not from my side.

tstellar marked an inline comment as not done.Oct 18 2021, 11:23 AM

The intent of the change looks good to me, i.e. using variables to set the version number & al.

However, I checked out this patch and built the documentation, and it doesn't show "In Progress" anymore, nor does it have any mention of being the libc++ 14.0.0 incoming release. Is that intended? IMO, it was useful to have that visual marker about notes being in-progress release notes for an upcoming release.

It's not intended. I will try to reproduce with your build configuration.

The intent of the change looks good to me, i.e. using variables to set the version number & al.

However, I checked out this patch and built the documentation, and it doesn't show "In Progress" anymore, nor does it have any mention of being the libc++ 14.0.0 incoming release. Is that intended? IMO, it was useful to have that visual marker about notes being in-progress release notes for an upcoming release.

It's not intended. I will try to reproduce with your build configuration.

It looks like the runtime builds don't have access to the LLVM_VERSION* cmake variables, which is why the version information is missing. I think I'll try to convert some of the non-runtime sub-projects first and then revisit libcxx once I'm done with those.

The intent of the change looks good to me, i.e. using variables to set the version number & al.

However, I checked out this patch and built the documentation, and it doesn't show "In Progress" anymore, nor does it have any mention of being the libc++ 14.0.0 incoming release. Is that intended? IMO, it was useful to have that visual marker about notes being in-progress release notes for an upcoming release.

It's not intended. I will try to reproduce with your build configuration.

It looks like the runtime builds don't have access to the LLVM_VERSION* cmake variables, which is why the version information is missing. I think I'll try to convert some of the non-runtime sub-projects first and then revisit libcxx once I'm done with those.

Alright, sounds good to me. Sorry our build system migration is clashing with this effort, but at least it's good that we're aware of it.

I really like the direction this patch is taking. I want to give it another testrun after the runtime builds are fixed.
(I still use a non-runtime build.)

libcxx/docs/ReleaseNotes.rst
3

Please resize line 1 and 3 to match line 2.

libcxx/docs/conf.py
45–46

Since we use 14.0.0git in other places I agree we should be consistent and do the same in the documentation.