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 automatic API-doc generation & deployment #183

Merged
merged 11 commits into from
May 15, 2020

Conversation

zbeekman
Copy link
Member

@zbeekman zbeekman commented May 14, 2020

EDIT: Preview the documentation here: https://fortran-lang.github.io/stdlib-docs/index.html
NOTE: Autodeploy failed, not sure if it's due to this being a PR from a fork, or if ${{ secrets.ACTIONS_DEPLOY_KEY }} was never setup, or if it is incompatible with cross-repository deployment.

PR to automatically generate and deploy documentation using FORD published via GH pages.

Notes:

  • I think we should move the specs to a dedicated directory, and then they can be rendered by FORD.
  • It would be nice to deploy these to something like stdlib.fortran-lang.org.
  • May want to move the WORKFLOW.md and STYLEGUIDE.md documents into the FORD pages dir

For now this PR is a WIP.

Begins to address issues:

@milancurcic
Copy link
Member

Thanks, Zaak! I think this is a great first step forward.

Pending others' reviews, let's move this forward and I can tackle the DNS record. I agree stdlib.fortran-lang.org is a good subdomain for this.

After this is merged, then the GH actions will push to stdlib-docs repo. Then we can simply enable it to serve at the default location (fortran-lang.github.io/stdlib-docs), check that it looks good, then we can set up the CNAME record.

@zbeekman
Copy link
Member Author

After this is merged, then the GH actions will push to stdlib-docs repo. Then we can simply enable it to serve at the default location (fortran-lang.github.io/stdlib-docs), check that it looks good, then we can set up the CNAME record.

Yeah, that was my thinking too. Currently it will deploy aggressively because I want to check that it's all working. (Assuming the ACTIONS_DEPLOY_KEY secret is properly set and I haven't mucked something up in the workflow file. I think @certik set it for me a while ago, but someone with admin should double check.)

source: true
proc_internals: true
md_extensions: markdown.extensions.toc
graph: true
Copy link
Member Author

Choose a reason for hiding this comment

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

It currently takes ~20 minutes to generate the graphs. We may decide to disable this. We could set this to false and then manually use the entity metadata to enable a subset of graphs.

Copy link
Member

Choose a reason for hiding this comment

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

MAXRANK could be limited to 3. However, this will only marginally reduced the time. Because more procedures are expected, disabling the automatic generation of the graphs seems to be needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

When building during CI waiting ~20 minutes isn't the end of the world, IMO. I've added parameters to prune/limit the depth and nodes per graph. I think it's OK to leave this in the workflow, but could be convinced otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

Is this now done for every PR that gets submitted to stdlib?

20 minutes is not the end of the world, but it significantly slows down the development.

Do you know why it takes so long?

Copy link
Member

Choose a reason for hiding this comment

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

Do you know why it takes so long?

I guess that the issue is the generation of the graphs for all generated (hundreds?) functions in the different submodules.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can also limit MAXRANK to 3 as suggested by @jvdp1. Should speed things up, and folks can extrapolate to higher rank code.

Copy link
Member

Choose a reason for hiding this comment

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

@zbeekman do you know how to restrict the rank? As it is now, all tests finish quickly except this documentation test that takes forever. Anything we can do to speed it up would help, at least in our initial stages. As we turn into production (we move functionality from experimental to main), we can revisit this.

Copy link
Member Author

Choose a reason for hiding this comment

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

@zbeekman do you know how to restrict the rank? As it is now, all tests finish quickly except this documentation test that takes forever. Anything we can do to speed it up would help, at least in our initial stages. As we turn into production (we move functionality from experimental to main), we can revisit this.

@certik Yes: MAXRANK=<n> here Do you want me to submit a PR restricting API documentation to public and protected entities only, reducing the MAXRANK and turning off search index and graph building unless documentation is going to be deployed? Also, what RANK should we restrict to? 2 or 3?

Copy link
Member

Choose a reason for hiding this comment

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

If you could do that, that would be awesome. Can search index + graph building fail? If so, then we have to test it. Last thing we want is to fail at deploy.

Regarding rank, I don't know, try 3 and if it is fast enough, let's keep it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@certik

Can search index + graph building fail?

I've never seen search indexing fail and I don't think I've ever seen graph building fail, but this one I'm more nervous about.

project: Fortran-lang/stdlib
summary: A community driven standard library for (modern) Fortran
src_dir: src
exclude_dir: src/tests
Copy link
Member Author

Choose a reason for hiding this comment

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

I excluded the tests from the documentation. We could include them if we wish but processing takes longer and it might dilute the API documentation with other stuff.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with not including the tests in the API documentation. Furthermore, specs include some examples for each procedure.

@@ -0,0 +1,3 @@
---
title: Specs, examples & user documentation
---
Copy link
Member Author

Choose a reason for hiding this comment

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

This section needs expanding/sign-posting.

title: specs
---

@todo Explain what these are, how to write them, why they're needed, etc.
Copy link
Member Author

Choose a reason for hiding this comment

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

This section also probably needs expanding/signposting.


[TOC]
Copy link
Member Author

Choose a reason for hiding this comment

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

The anchor/backlink syntax seems to be different in python markdown, and we have the automatic TOC extension enabled, so I decided to use that rather than manually maintain a TOC.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM!

 - Also, see if redeployment will work now
Copy link
Member

@certik certik left a comment

Choose a reason for hiding this comment

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

That looks good to me as a start, thanks @zbeekman!

Milan, can you work with Zaak to get the dns setup and merge this PR when it's time?

@zbeekman
Copy link
Member Author

@milancurcic or @certik (or anyone else with full admin) can you please check and confirm that the ACTIONS_DEPLOY_KEY secret is properly set?

The deployment failed, but I'm not 100% sure why. If needed I can switch to a personal token.

@zbeekman
Copy link
Member Author

zbeekman commented May 14, 2020

@milancurcic or @certik (or anyone else with full admin) can you please check and confirm that the ACTIONS_DEPLOY_KEY secret is properly set?

The deployment failed, but I'm not 100% sure why. If needed I can switch to a personal token.

I think this is due to this being a PR from a fork because secrets are not available in this context. Would it be OK to push this branch to the main repo and try the PR again to confirm deployment will work?

@certik certik mentioned this pull request May 14, 2020
@certik
Copy link
Member

certik commented May 14, 2020

I pushed it in #184. Let me know if you have rights to push into that branch.

@zbeekman
Copy link
Member Author

Thanks @certik. I found a good work around for testing purposes: I added the secret to my fork which let me deploy successfully. If CI turns green here I'm happy to merge.

Copy link
Member

@jvdp1 jvdp1 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 @zbeekman for this. It is a really good start! Really appreciated! Some things need to be completed (and it will come time), but now we can mention this website to potential users of stdlib.

source: true
proc_internals: true
md_extensions: markdown.extensions.toc
graph: true
Copy link
Member

Choose a reason for hiding this comment

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

MAXRANK could be limited to 3. However, this will only marginally reduced the time. Because more procedures are expected, disabling the automatic generation of the graphs seems to be needed.

project: Fortran-lang/stdlib
summary: A community driven standard library for (modern) Fortran
src_dir: src
exclude_dir: src/tests
Copy link
Member

Choose a reason for hiding this comment

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

I agree with not including the tests in the API documentation. Furthermore, specs include some examples for each procedure.


[TOC]
Copy link
Member

Choose a reason for hiding this comment

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

LGTM!

API-doc-FORD-file.md Outdated Show resolved Hide resolved
A was missing

Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
@milancurcic
Copy link
Member

@zbeekman The CI is green and I will now merge this. Thank you!

@milancurcic milancurcic merged commit ce987d2 into fortran-lang:master May 15, 2020
@zbeekman zbeekman deleted the zb-documentation branch May 19, 2020 13:31
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.

4 participants