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

Add project name and version to API docs #8792

Merged

Conversation

straight-shoota
Copy link
Member

MVP implementation of #4754

This PR adds a new section in the API docs sidebar which contains the name and version of the project.

image

The values can be set using the new --project-name and --project-version CLI arguments.

name
When --project-name is missing the doc generator currently errors. This should be changed to provide a good default, so that crystal docs continues to just work and not require any configuration for most shards.
The optimal solution would be to use the name property from shard.yml. Properly reading that file depends on libyaml which is not an option. Alternatively we could use a naive text-based extraction (such as grep for /^name:\s+(.*)/) or call the shards binary to get the information. The latter seems like a good solution to me, but needs to be implemented in shards first (shards info --name, cf crystal-lang/shards#86).

Another alternative could be using the repository name (which has so far been used as the title) or directory name. Both are not ideal though, because shard names sometimes differ from repo names and we really want to show the shard name.

version
version currently defaults to master when --project-version is missing. IMO we could leave it like that because that's what you usually have. When building docs for a specific commit, it should be easy to notice and fix (by supplying --project-version explicitly).
An alternative would be reading the tag from git. Using the last tag from the commit history (i.e. git describe --abbrev=0 --tags) doesn't take into account that the repo might have advanced since then. Only when directly at a tagged commit (i.e. git tag --points-to HEAD) it would work reliably (ideally workdir and index need to be clean, too).
This is essentially the reason why we have src/VERSION in the crystal repository and it gets updated on every tag for the plain tag and afterwards -dev is added.
We could try to do this automatically, first checking for git tag --points-to HEAD and if there's no result use git describe --abbrev=0 --tags with -dev appended.
I'm not sure if there would be any issues with that? We didn't choose this approach for the compiler because it's supposed to work when distributed as a tarball without git.

@straight-shoota straight-shoota force-pushed the feature/docs-name-version branch from 194ce68 to 76b684b Compare April 3, 2020 22:15
@straight-shoota
Copy link
Member Author

@crystal-lang/crystallers Any feedback on this? I'd like to move on with this 💪

Regarding the version problem, it might be best to use a generic version identifier like dev by default. If in a git repository, the working directory and index are clean and HEAD is a tagged version, that version tag should be used. That should work great without any configuration when generating docs for releases.
When building docs in any other state, it often doesn't really matter what the version says (for development build). Otherwise it requires some explicit configuration using --project-version option, but that should be fine. It would often be difficult to determine automatically, anyway.
If we discover other use cases that can be easily modeled to work without configuration, we can add improve on it later.

@asterite
Copy link
Member

asterite commented Apr 8, 2020

I think this is great

@straight-shoota straight-shoota force-pushed the feature/docs-name-version branch from 56e80ed to 637191b Compare April 8, 2020 21:42
Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

I'd like this to not introduce more errors, it's not that important. Worst case should be that the info just doesn't generate.

src/compiler/crystal/command/docs.cr Outdated Show resolved Hide resolved
src/compiler/crystal/command/docs.cr Outdated Show resolved Hide resolved
@straight-shoota
Copy link
Member Author

I've considered simply ignoring missing data. But my train of thought was that the doc generator would often be used in automatic build scripts which generate and deploy the documentation. If anything goes wrong - for whatever reason - but the generator just proceeds with a default value, the error will go unnoticed. But I'd like a CI script to fail if it can't determine the values it needs. And not publish docs for unknown shard.
Obviously we could add another flag like --no-project-defaults for this mode but that makes everything even more complex. I don't think erroring is such a big deal.
Ideally, errors should only happen in irregular situations, so I'd definitely work on improving the version detection. For example, we could fall back to using the branch name and/or commit ref if not tagged, and append -dev if the index/workdir are dirty. That should probably eliminate most causes for the error.

@RX14
Copy link
Contributor

RX14 commented Apr 9, 2020

Erroring on not finding the project name is acceptable. Erroring if it's not a git repo is not. I think a fallback to shard.yml version would be fine?

@bcardiff
Copy link
Member

Also, I would avoid making git a hard requirement in the compiler. Surely it will most likely be there, but either way.

Other that than and what @RX14 said, LGTM.

@straight-shoota straight-shoota force-pushed the feature/docs-name-version branch 3 times, most recently from b5695ae to c606904 Compare April 10, 2020 12:30
Copy link
Member Author

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

I have improved the defaults detection. Examples for specific use cases are in project_info_spec.cr.

@straight-shoota straight-shoota force-pushed the feature/docs-name-version branch from e12c7ff to bfbc2eb Compare April 10, 2020 23:43
bin/ci Outdated Show resolved Hide resolved
@straight-shoota straight-shoota force-pushed the feature/docs-name-version branch from 5ef17e5 to 72f798e Compare April 14, 2020 00:14
@straight-shoota
Copy link
Member Author

I've added some improvements. Ready for final review.

@straight-shoota straight-shoota requested a review from RX14 April 14, 2020 08:49
@straight-shoota straight-shoota merged commit 9a2d704 into crystal-lang:master Apr 14, 2020
@straight-shoota straight-shoota deleted the feature/docs-name-version branch April 14, 2020 19:23
@straight-shoota straight-shoota added this to the 0.35.0 milestone Apr 14, 2020
@straight-shoota
Copy link
Member Author

This is now live on nightly: https://crystal-lang.org/api/master/

@bcardiff
Copy link
Member

FYI for contributors of std-lib this PR force them to compile the compiler before been able to run make docs, otherwise they'll get.

./bin/crystal docs src/docs_main.cr  --project-name=Crystal --project-version=0.35.0-dev
Error: Invalid option: --project-name=Crystal

It will only be for one release. Until then, is changing a bit the workflow.

carlhoerberg pushed a commit to carlhoerberg/crystal that referenced this pull request Apr 29, 2020
Introduces Crystal::Docs::ProjectInfo as container for doc generator configuration
@fabianhjr
Copy link

Just as a reference, encountered the issue mentioned by bcardiff while trying to build 0.35.1

building '/nix/store/7166z26xf45mfn6xz4vymm4g5q98y8cj-crystal-0.35.1.drv'...
unpacking sources
unpacking source archive /nix/store/bg8ib2w8m8y45dgj8f6gjx3rdl724pi4-source
source root is source
patching sources
substituteStream(): WARNING: pattern '{% if flag?(:gnu) %}"listen: "{% else %}"bind: "{% end %}' doesn't match anything in file 'spec/std/socket/tcp_server_spec.cr'
substituteStream(): WARNING: pattern '01 Jan 2020' doesn't match anything in file 'spec/std/http/cookie_spec.cr'
configuring
no configure script, doing nothing
building
build flags: -j16 -l16 SHELL=/nix/store/2jysm3dfsgby5sw5jgj43qjrb5v79ms9-bash-4.4-p23/bin/bash CRYSTAL_CONFIG_VERSION=0.35.1 all docs
Using /nix/store/gijq02j6irrp5f6q0f5nandzv5cdzx8b-llvm-9.0.1/bin/llvm-config [version=9.0.1]
g++ -c  -o src/llvm/ext/llvm_ext.o src/llvm/ext/llvm_ext.cc -I/nix/store/gijq02j6irrp5f6q0f5nandzv5cdzx8b-llvm-9.0.1/include -std=c++11   -fno-exceptions -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
gcc -fPIC    -c -o src/ext/sigfault.o src/ext/sigfault.c
./bin/crystal docs src/docs_main.cr  --project-name=Crystal --project-version=0.35.1 --source-refname=
Error: Invalid option: --project-name=Crystal
make: *** [Makefile:96: docs] Error 1
make: *** Waiting for unfinished jobs....
builder for '/nix/store/7166z26xf45mfn6xz4vymm4g5q98y8cj-crystal-0.35.1.drv' failed with exit code 2
error: build of '/nix/store/7166z26xf45mfn6xz4vymm4g5q98y8cj-crystal-0.35.1.drv' failed

Will investigate

@straight-shoota
Copy link
Member Author

Judging from the error message it seems bin/crystal points to the previous release 0.34.0 which doesn't know the CLI options added in 0.35.0. Docs should be build with the same (i.e. newly built) compiler version.

@fabianhjr
Copy link

Yeah, it seems to be the case though trough reading the makefile I am wondering if docs should depend on crystal.

@fabianhjr
Copy link

For the time being I think I will add a patch for docs: -> docs: crystal on the Makefile that seems to fix the build issue I encountered.

@straight-shoota
Copy link
Member Author

I am wondering if docs should depend on crystal.

That's probably a bad idea. It would force everyone to build the compiler when you just want to build the docs. But that's usually completely unnecessary. It's only relevant when there are significant differences between the version of the compiler binary and the source code which is rarely the case.

Most package builder workflows build the compiler first and then use that fresh compiler to build the docs. Does this not work for your use case? (I'm not familiar with nix build process, except that everythings pretty much isolated; but you should be able to somehow use the matching compiler version for dependend tasks?)

@fabianhjr
Copy link

fabianhjr commented Nov 16, 2020

It should be safe to apply that patch to the nix build, this only came up because it is attempting to do a make all docs simultaneously. Since this is for the build system that builds both crystal and crystal's docs it should not be a problem. That patch would only be added downstream.

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.

5 participants