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

expand doc publish script, use products for sitegen tasks, and clarify the publish site subdir option #5702

Merged
merged 18 commits into from Apr 27, 2018

Conversation

Projects
None yet
5 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Apr 14, 2018

Problem

The build-support/bin/publish_docs.sh script doesn't allow changing the repo/site to publish to. Additionally, three separate pants invocations have to run to generate the site. They aren't cached whatsoever, but I didn't worry about that here.

Solution

  • Expand the help text and add some options to publish_docs.sh to support publishing to a different local or remote destination.
  • Merge the three pants invocations into one sitegen invocation which requires the reference and markdown products.
  • Clarify the meaning of the -d option to publish_docs.sh.

Result

  • Pants no longer has to be invoked multiple times to generate the site.
  • The site can be published somewhere besides the main site.
  • The options for publish_docs.sh are better described.

cosmicexplorer added some commits Mar 9, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Apr 15, 2018

@baroquebobcat I can't add reviewers in this repo, but I'd love it if you could take a look at this one.

@stuhood stuhood requested review from dotordogh , benjyw , lahosken and baroquebobcat and removed request for benjyw Apr 16, 2018

@lahosken
Copy link
Member

lahosken left a comment

Thank you for doing this.

PANTS_GH_PAGES='https://github.com/pantsbuild/pantsbuild.github.io.git'
GIT_URL="${GIT_URL:-$PANTS_GH_PAGES}"

PANTS_SITE_URL='https://pantsbuild.github.io'

This comment has been minimized.

@lahosken

lahosken Apr 16, 2018

Member

I think this part of the script predates https://www.pantsbuild.org/ . Probably makes sense to say https://www.pantsbuild.org/ nowadays (tho I wouldn't be shocked if someone who knows the magic dns/redir/something behind that tells us that's a terrible idea)

This comment has been minimized.

@benjyw

benjyw Apr 17, 2018

Contributor

the SITE_URL can change, I think, but not the PANTS_GH_PAGES obvi.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 16, 2018

Contributor

I was going to make that change, also wasn't sure if there was some magic that would break if I changed it. Will let others weigh in, but I've changed it to https://www.pantsbuild.org/ in af8de96. Thanks!

PANTS_GH_PAGES='https://github.com/pantsbuild/pantsbuild.github.io.git'
GIT_URL="${GIT_URL:-$PANTS_GH_PAGES}"

PANTS_SITE_URL='https://pantsbuild.github.io'

This comment has been minimized.

@benjyw

benjyw Apr 17, 2018

Contributor

the SITE_URL can change, I think, but not the PANTS_GH_PAGES obvi.

previews).
Environment Variables and Defaults:
GIT_URL=$PANTS_GH_PAGES

This comment has been minimized.

@benjyw

benjyw Apr 17, 2018

Contributor

Let's use ${PANTS_GH_PAGES} style everywhere, as it's a good habit to get into.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 19, 2018

Contributor

Ok, I'll do that from now on. My reasoning was that it makes it more clear whether or not an argument is purely a variable (e.g. "$GIT_URL") vs part variable, part another string (e.g. "${REPO_ROOT}/src/docs/publish_via_git.sh". Unless there's something I'm missing I find that distinction useful personally, but style consistency is almost the most important thing in shell scripts so I will use the ${} style from now on.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 19, 2018

Contributor

the SITE_URL can change, I think, but not the PANTS_GH_PAGES obvi.

Was this comment also in reference to the use of the ${} style, or did you mean that one should be declared readonly (which is definitely true)?

${publish_path}
do_open ${url}/index.html
"${REPO_ROOT}/src/docs/publish_via_git.sh" \
"$GIT_URL" \

This comment has been minimized.

@benjyw

benjyw Apr 17, 2018

Contributor

Ditto, and on the subsequent line.

# and reference tasks run before this one, but we don't use those
# products.
@classmethod
def prepare(cls, options, round_manager):

This comment has been minimized.

@benjyw

benjyw Apr 17, 2018

Contributor

Nice!

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 19, 2018

Contributor

I didn't go all the way to do that here because I think sitegen.py might benefit from a complete revamp (which would result in cutting the line count way down and making sitegen far easier to understand and extend). Still need to iterate on that one but this is a good first step.

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

lgtm aside from an inconsistency in the publish docs.

To see http://www.pantsbuild.org/ site's content as it would be generated based on your local
copy of the pants repo, enter the command
copy of the pants repo, enter the command:

This comment has been minimized.

@baroquebobcat

baroquebobcat Apr 18, 2018

Contributor

Could you update the following code snippet to add a -l so that the preview is published locally, or remove the comment?

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 19, 2018

Contributor

I added a little section right under the code snippet describing the -l option -- could you let me know if that addresses your concern?

@stuhood
Copy link
Member

stuhood left a comment

Thanks, looks good! Removing the extra pants.*.ini would be good before merging.

[reference]
pants_reference_template: reference/pants_reference_body.html
build_dictionary_template: reference/build_dictionary_body.html

This comment has been minimized.

@stuhood

stuhood Apr 24, 2018

Member

Why is this a separate file?

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 27, 2018

Contributor

...unclear. I moved it to the bottom of pants.ini and also added the path to the json config file for the sitegen task instead of specifying it in the pants invocation in publish_docs.sh.


:::bash
# This publishes the docs **locally** and opens (-o) them in your browser for review
# This publishes the docs in dist/docsite and opens (-o) them in your browser for review

This comment has been minimized.

@stuhood

stuhood Apr 24, 2018

Member

Removing the publishes keyword here would make it clearer that this is a sideeffect free operation (which I think was the reason for the bolding before).

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Apr 27, 2018

Contributor

Changed to # This generates the site html in dist/docsite and opens (-o) index.html in your browser for review..

cosmicexplorer added some commits Apr 27, 2018

@stuhood stuhood merged commit cae2c26 into pantsbuild:master Apr 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment