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

PR: merge Rootstock updates on 2020-02-28 #991

Merged
merged 25 commits into from
Feb 28, 2020

Conversation

dhimmel
Copy link
Collaborator

@dhimmel dhimmel commented Feb 28, 2020

No description provided.

vincerubinetti and others added 23 commits December 13, 2019 15:12
merges manubot/rootstock#286
closes manubot/rootstock#284

- removes table scroll plugin
- relies on pandoc-tablenos to wrap `<table>` elements in `<div>`s that can be scrolled
- updates CSS such that breaks are forced when printing or when a table isn't given an id
  (and thus isn't wrapped with a `<div>`
- simplifies plugins a bit as a result of pandoc-tablenos and pandoc-fignos changes
- adds tolerance to scroll-out-of-bounds feature
- adds support for equations in anchors plugin
Merges manubot/rootstock#289

* Update metadata.yaml to use 'authors'
* Mention delete-me.md in USAGE
merges manubot/rootstock#290

The options for the pandoc commands in build/build.sh are now configured
in YAML files that are passed to Pandoc using --defaults.

export PANDOC_DEFAULTS_DIR to override the directory with pandoc --defaults files
merges manubot/rootstock#302

GitHub Actions: do not run deploy.sh if secrets.MANUBOT_SSH_PRIVATE_KEY
is not set resulting in `env: MANUBOT_SSH_PRIVATE_KEY: ""`
https://github.saobby.my.eu.orgmunity/t5/GitHub-Actions/If-expression-with-context-variable/m-p/34560/highlight/true#M1959

Travis CI: condition deploy on MANUBOT_SSH_PRIVATE_KEY being set
or variable name expansion for encrypted_*.
https://unix.stackexchange.com/a/290296/294987
https://wiki.bash-hackers.org/syntax/pe#variable_name_expansion
merges manubot/rootstock#303

SETUP manubot upgrade instructions. add  --no-rebase to git pull
Upgrade Manubot to fix get_rootstock_commit
GitHub Actions: fetch entire commit history to support get_rootstock_commit
merges manubot/rootstock#305

Create artifacts that are named like manuscript-32632624-d1c4caf
and contain the GitHub Actions run-id as well as the head of the triggering
commit hash. Note that on a pull request, this is the commit that was
added to the PR and not the auto-generated merge commit that the
action is actually evaluating.
Rename environment variables for deployment to:
$CI_BUILD_WEB_URL
$CI_JOB_WEB_URL

Fix bug with $CI_JOB_WEB_URL on GitHub Actions
merges manubot/rootstock#311

Includes manubot update with yamllint output for
invalid metadata.yaml files.
merges manubot/rootstock#310

Previously secrets.GITHUB_TOKEN did not work for deployment because
the commits it pushed to `gh-pages` were unable to trigger a GitHub Pages
build on public repositories. However, this issue seems to have been quietly
resolved. There has been no official GitHub announcement about the change,
so there is a risk that it will be temporary.

Deployment now uses MANUBOT_SSH_PRIVATE_KEY over
MANUBOT_ACCESS_TOKEN when both are set.
The idea is that if GitHub's support for GITHUB_TOKEN is temporary
(since no official commitment yet), then users who have configured
MANUBOT_ACCESS_TOKEN will not be affected.
Only users who did not set up MANUBOT_SSH_PRIVATE_KEY will be affected,
but they could then set up MANUBOT_SSH_PRIVATE_KEY without having to
upgrade Manubot Rootstock to get deployment to work.
merges manubot/rootstock#313

Previously deployment was governed by the presence of
MANUBOT_SSH_PRIVATE_KEY, but now that GITHUB_TOKEN
is supported for deployment, forks with master branch commits
attempted to deploy.
merges manubot/rootstock#314

* Simplify setup by creating branches later
For a new manuscript, the first deploy event won't be able to fetch
gh-pages and output because they were not pushed during setup.
However, the deploy script now ignores the error on fetch and
continues with the deployment, creating those branches at the first
deployment. All subsequent deployments work as they did previously.

* SETUP: Update with how to workaround GITHUB_TOKEN not
triggering Pages builds.
merges manubot/rootstock#315

Update environment on 2020-02-27
USAGE: clarify citation key syntax
@AppVeyorBot
Copy link

AppVeyor build 1.0.26 for commit dfaf888 by @dhimmel failed.

reference_counts is no longer added to manuscript_stats because
citation processing has been moved to pandoc-manubot-cite filter.
@dhimmel
Copy link
Collaborator Author

dhimmel commented Feb 28, 2020

I think this PR has to be merged before GitHub actions will start running. However, once that's the case, we could remove Travis and use GitHub Actions + AppVeyor. @agitter?

content/citation-tags.tsv is now deprecated
@dhimmel
Copy link
Collaborator Author

dhimmel commented Feb 28, 2020

builds for c822167 seem to be timing out. Not sure why. One possibility is that something wrote to stdout and has somehow caused pandoc to indefinitely wait on the pandoc-manubot-cite filter output

Ah I bet its https://github.com/manubot/manubot/blob/68ae7cfeb3928729228b5c9f0e11abbaceb7edb9/manubot/util.py#L128-L129

Actually it's looking like it's running just slowly. Looks like retrieving citation metadata is slow. Possibly due to datacite switch?

@AppVeyorBot
Copy link

AppVeyor build 1.0.27 for commit c822167 by @dhimmel is now complete. The rendered manuscript from this build is temporarily available for download at:

@AppVeyorBot
Copy link

AppVeyor build 1.0.28 for commit d8c2b79 by @dhimmel is now complete. The rendered manuscript from this build is temporarily available for download at:

Copy link
Collaborator

@agitter agitter left a comment

Choose a reason for hiding this comment

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

we could remove Travis and use GitHub Actions + AppVeyor

Yes, that switch works for me.

I wonder why the AppVeyor builds still work despite the Travis CI builds timing out? I made an initial pass over the changes, and they look good so far. I'd like to do a final check once the Travis timeouts are resolved. I haven't inspected the manuscript artifacts yet.

@@ -2,7 +2,8 @@

[![HTML Manuscript](https://img.shields.io/badge/manuscript-HTML-blue.svg)](https://greenelab.github.io/deep-review/)
[![PDF Manuscript](https://img.shields.io/badge/manuscript-PDF-blue.svg)](https://greenelab.github.io/deep-review/manuscript.pdf)
[![Build Status](https://travis-ci.org/greenelab/deep-review.svg?branch=master)](https://travis-ci.org/greenelab/deep-review)
[![GitHub Actions Status](https://github.com/greenelab/deep-review/workflows/Manubot/badge.svg)](https://github.com/greenelab/deep-review/actions)
[![Travis Build Status](https://travis-ci.org/greenelab/deep-review.svg?branch=master)](https://travis-ci.org/greenelab/deep-review)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can remove this if we switch to GitHub Actions+AppVeyor

@dhimmel
Copy link
Collaborator Author

dhimmel commented Feb 28, 2020

I wonder why the AppVeyor builds still work despite the Travis CI builds timing out?

Travis (by default) times out after 10 minutes of inactivity in terms of log output. AppVeyor must be more patient.

@AppVeyorBot
Copy link

AppVeyor build 1.0.29 for commit 968c918 by @dhimmel is now complete. The rendered manuscript from this build is temporarily available for download at:

@dhimmel
Copy link
Collaborator Author

dhimmel commented Feb 28, 2020

Okay I think the timeout was caused by the datacite DOI resolver being a bit slower than what we used previously combined with having to regenerate the cache for all DOIs. @agitter I think we can merge with the slowness.

One thing we should probably do is freeze the references. But possibly in a follow up PR

@dhimmel dhimmel force-pushed the rootstock-2020-02-28 branch from 968c918 to d8c2b79 Compare February 28, 2020 22:02
Copy link
Collaborator

@agitter agitter left a comment

Choose a reason for hiding this comment

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

I didn't notice any problems in the PDF.

One thing we should probably do is freeze the references. But possibly in a follow up PR

Yes, let's save this for a follow up PR.

[@tag:Gulshan2016_dt]: doi:10.1001/jama.2016.17216
[@tag:Gupta2015_exprs_yeast]: doi:10.1101/031906
[@tag:Gupta2015_prec]: arxiv:1502.02551
[@tag:Guetterman]: url:http://www.fasebj.org/content/30/1_Supplement/406.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we freeze references post-merge, we need to add a manual reference for this first. We're adding a broken DOI link https://doi.org/10.1096/fasebj.30.1_supplement.406.3

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.

5 participants