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 LIPIcs article format, closes #232 #288

Merged
merged 8 commits into from
Oct 29, 2020
Merged

add LIPIcs article format, closes #232 #288

merged 8 commits into from
Oct 29, 2020

Conversation

nuest
Copy link
Contributor

@nuest nuest commented Apr 15, 2020

@yihui Here's another template :-)

If you have any idea why my local system does not find the plainurl.bst, I'm happy to remove that file, but it needed to be there for me to work.


  • Unless you have done it in any other RStudio's projects before, please sign the individual or corporate contributor agreement for a significant pull request (it is fine not to sign it if a PR is only intended to fix a few typos). You can send the signed copy to jj@rstudio.com.

  • Add the journalname_article() function to R/article.R if the output format is simple enough, otherwise create a separate R/journalname_article.R.

  • Add the Pandoc LaTeX template inst/rmarkdown/templates/journalname/resources/template.tex.

  • Add a skeleton article inst/rmarkdown/templates/journalname/skeleton/skeleton.Rmd.

  • Add a description of the template inst/rmarkdown/templates/journalname/template.yaml.

  • Please include the document class file (*.cls) if needed, but please do not include standard LaTeX packages (*.sty) that can be downloaded from CTAN. If you are using TinyTeX or TeX Live, you can verify if a package is available on CTAN via tinytex::parse_packages(files = "FILENAME"") (e.g., when FILENAME is plain.bst, it should return "bibtex", which means this file is from a standard CTAN package). Please keep the number of new files absolutely minimal (e.g., do not include PDF output files), and also make examples minimal (e.g., if you need a .bib example, try to only leave one or two bibliography entries in it, and don't include too many items in it without using all of them).

  • Update Rd and namespace (could be done by devtools::document()).

  • Update NEWS.

  • Update README with a link to the newly supported journal. Please add your Github username and the full name of the journal (follow other examples in the list).

  • Add a test to tests/testit/test-formats.R by adding a line test_format("journalname"). We try to keep them in alphabetical order.

  • Add your name to the list of authors Authors@R in DESCRIPTION. You don't need to bump the package version in DESCRIPTION.

Lastly, please try your best to do only one thing per pull request (e.g., if you want to add two output formats, do them in two separate pull requests), and refrain from making cosmetic changes in the code base: https://yihui.name/en/2018/02/bite-sized-pull-requests/

@nuest nuest force-pushed the LIPIcs branch 2 times, most recently from 2050263 to 103fe45 Compare May 7, 2020 15:11
@nuest nuest marked this pull request as draft July 7, 2020 09:22
@nuest
Copy link
Contributor Author

nuest commented Jul 7, 2020

I am now going through the actual submission and realize, that there are some things that must be configurable in addition.

@nuest nuest marked this pull request as ready for review July 7, 2020 09:51
@nuest
Copy link
Contributor Author

nuest commented Jul 7, 2020

Ready for review :-)

Copy link
Member

@yihui yihui 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 very much! Could you go through the checklist before we actually review the PR? https://github.com/rstudio/rticles/blob/master/.github/PULL_REQUEST_TEMPLATE.md (e.g. if CHANGELOG.md is not required for the template to work, you may leave it out)

BTW, if you are using TinyTeX, it should be able to find the correct package to install:

> tinytex::parse_packages(files = 'plainurl.bst')
tlmgr search --file --global '/plainurl.bst'
[1] "urlbst"

@yihui yihui requested a review from cderv August 3, 2020 15:00
Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

Hi @nuest !

Thanks a lot for your contribution. I posted a few comments some of which need to be dealt with, the other are for potential improvements if you think it is correct.

General comment :

  • There will be a conflict with the README because we change it recently. I will deal later when ready to merge - don't worry.
  • Are CHANGELOG.md and LICENCE.md necessary to be included in rticles ?
  • If I delete the plainurl.bst file from the created directory, it renders fine. I don't think this is useful. I am using TinyTex though. What is your issue with this file ?

I also let you go through the PR checlklist by yourself (you can edit you first post, copy the content and check the boxes when ready). Thank you.

R/lipics_article.R Outdated Show resolved Hide resolved
@nuest
Copy link
Contributor Author

nuest commented Aug 17, 2020

Thanks for the review @cderv I'll get to this hopefully by the end of the week.

@nuest nuest force-pushed the LIPIcs branch 2 times, most recently from a724546 to 25e53d8 Compare September 16, 2020 17:10
@nuest
Copy link
Contributor Author

nuest commented Sep 16, 2020

I've copied the new PR checklist over and think I covered all the requested changes. Remaining tasks

  • use new filter for corollary and proposition
  • fix plainurl.bst

However, for me it does not work when I remove plainurl.bst. I'm also using TinyTex (AFAICT?):

/usr/bin/pandoc +RTS -K512m -RTS MyArticle.utf8.md --to latex --from markdown+tex_math_single_backslash-autolink_bare_uris-auto_identifiers --output MyArticle.tex --self-contained --template /home/daniel/R/x86_64-pc-linux-gnu-library/4.0/rticles/rmarkdown/templates/lipics/resources/template.tex --highlight-style tango --pdf-engine xelatex --natbib --lua-filter /home/daniel/R/x86_64-pc-linux-gnu-library/4.0/rmarkdown/rmd/lua/pagebreak.lua --lua-filter /home/daniel/R/x86_64-pc-linux-gnu-library/4.0/rmarkdown/rmd/lua/latex-div.lua 
This is BibTeX, Version 0.99d (TeX Live 2020)
The top-level auxiliary file: MyArticle.aux
I couldn't open style file plainurl.bst
---line 20 of file MyArticle.aux
 : \bibstyle{plainurl
 :                   }
I'm skipping whatever remains of this command
I found no style file---while reading file MyArticle.aux
(There were 2 error messages)
Error: Failed to build the bibliography via bibtex

I have no idea what's going wrong here, thankful for any ideas.

@cderv
Copy link
Collaborator

cderv commented Sep 17, 2020

plainurl.bst is part of urlbst package

> tinytex::parse_packages(files = "plainurl.bst")
tlmgr search --file --global "/plainurl.bst"
[1] "urlbst"

But when this package is not installed and the file is not present, it does not throw an error inside TeX log, so the package is not install automatically by TinyTeX.

If we install the package, the Rmd compile correctly even if plainurl.bst is not in the folder.

# installed by 
tinytex::tlmgr_install("urlbst")
# then re render

So

  • either tinytex package is missing a way to handle those missing package containing bst file.
  • either we need to say to your article format that this package is required for tinytex to install it if missing. But it does not seem as a usual TeX package you can pass in extra_dependencies (https://ctan.org/pkg/urlbst).
  • either we shipped one version of the bst file with the skeleton.

@yihui is tinytex only parsing missing dependency from the log file ? I don't see any mention in this case about the missing plainurl.bst file that would trigger the installation from after tinytex::parse_package ?
You more expert than me on TeX Live - how should this work for bst file included in CTAN package ?
Thank you.

Also @nuest , don't forget at the end to remove the MyArticle folder that I guess you are using to test ? It should not be commited. 😄

@nuest
Copy link
Contributor Author

nuest commented Sep 17, 2020

@cderv Thanks for the investigation. PR updated without the test artefact. Let's see what @yihui has to say about the missing file.

@CLAassistant
Copy link

CLAassistant commented Sep 24, 2020

CLA assistant check
All committers have signed the CLA.

@nuest nuest requested a review from yihui September 26, 2020 11:30
@nuest
Copy link
Contributor Author

nuest commented Sep 26, 2020

I can confirm that the file plainurl.bst is not needed when urlbst is installed.

A hacky workaround for the urlbst package issue would be to add a snippet to the skeleton.Rmd that checks if urlbst is installed and if not it installs it:

if( !"urlbst" %in% tinytex::tl_pkgs()) tinytex::tlmgr_install("urlbst")

I would add a note that authors may remove the chunk after the first rendering.


I briefly checked extra_dependencies (didn't know that option), but that only leads to an error for me (the same for you @cderv ?): "! LaTeX Error: File urlbst.sty' not found." - I think that mechanism does not work for .bst` files?


I've taken a look at the tinytex source code, and it was straightforward to let detect_files(..) detect the log "I couldn't open style file plainurl.bst" BUT the problem is that this log is not in the .log file somehow. Could the problem be that the error message is earlier in the process?

Here is a screenshot of the relevant log output:

image

@nuest nuest force-pushed the LIPIcs branch 2 times, most recently from c829a56 to a1895e1 Compare September 26, 2020 14:32
@nuest
Copy link
Contributor Author

nuest commented Sep 26, 2020

I have switched a bunch of custom commans over to fenced blocks, and it works nicely except for the ones where the name ends in *:

  ::: {.remark* data-latex=""}
  Fusce eu leo nisi. Cras eget orci neque, eleifend dapibus felis. Duis et leo dui. Nam vulputate,   velit et laoreet porttitor, quam arcu facilisis dui, sed malesuada risus massa sit amet neque.
  :::
  
  ::: {.claim data-latex=""}
  \label{testenv-claim}
  
  Fusce eu leo nisi. Cras eget orci neque, eleifend dapibus felis. Duis et leo dui. Nam vulputate,   velit et laoreet porttitor, quam arcu facilisis dui, sed malesuada risus massa sit amet neque.
  :::
  
  ::: {.claim* data-latex=""}
  \label{testenv-claim2}
  
  Fusce eu leo nisi. Cras eget orci neque, eleifend dapibus felis. Duis et leo dui. Nam vulputate,   velit et laoreet porttitor, quam arcu facilisis dui, sed malesuada risus massa sit amet neque.
  :::

Gives me

image

@cderv Would rstudio/bookdown#940 be an appropriate place to report this? Do you think lemma* could be supported as a block name?

@cderv
Copy link
Collaborator

cderv commented Sep 28, 2020

@cderv Would rstudio/bookdown#940 be an appropriate place to report this? Do you think lemma* could be supported as a block name?

Yes this would be a good place to support this feature. Do you want unnumbered Lemma, is that it ?

Bookdown has a special support for theorem and proof env, and we are trying to make it work also with fenced div syntax for those specific environments.

Without bookdown, it is just the rmarkdown support for latex-divs. Can you report the bug there too about the star syntax not working ?

Also, seeing

  ::: {.claim data-latex=""}
  \label{testenv-claim}
  
  Fusce eu leo nisi. Cras eget orci neque, eleifend dapibus felis. Duis et leo dui. Nam vulputate,   velit et laoreet porttitor, quam arcu facilisis dui, sed malesuada risus massa sit amet neque.
  :::

I am thinking we could improve the lua filter by adding the \label line base on a provided id. That would make this works as you expect I think

::: {.claim #testenv-claim data-latex=""}
Fusce eu leo nisi. Cras eget orci neque, eleifend dapibus felis. Duis et leo dui. Nam vulputate,   velit et laoreet porttitor, quam arcu facilisis dui, sed malesuada risus massa sit amet neque.
:::

would be

\begin{claim} 
\label{testenv-claim}
Fusce eu leo nisi. Cras eget orci neque, eleifend dapibus felis. Duis et leo dui. Nam vulputate,   velit et laoreet porttitor, quam arcu facilisis dui, sed malesuada risus massa sit amet neque.
\end{claim}

Do you think it is a good idea ?

@cderv
Copy link
Collaborator

cderv commented Sep 28, 2020

@yihui Regarding the blg file, the error we got with this template comes in fact from build_bib() just before check_blg(). That is why it is not working because it errors, and the blg file is not parsed as the function exits.
https://github.com/yihui/tinytex/blob/9b48862b90a47d2f69b7e80b421ce1e3d65a2a91/R/latex.R#L235-L237
But from the log below, we see that the correct sentence is output in the log "I couldn't open style file plainurl.bst"

> rmarkdown::render("Untitled/Untitled.Rmd", clean = FALSE)
(...)
This is BibTeX, Version 0.99d (TeX Live 2020/W32TeX)
The top-level auxiliary file: Untitled.aux
I couldn't open style file plainurl.bst
---line 20 of file Untitled.aux
 : \bibstyle{plainurl
 :                   }
I'm skipping whatever remains of this command
I found no style file---while reading file Untitled.aux
(There were 2 error messages)
Erreur : Failed to build the bibliography via bibtex
> traceback()
7: stop("Failed to build the bibliography via ", bib_engine, call. = FALSE)
6: system2_quiet(bib_engine, shQuote(aux), error = {
       stop("Failed to build the bibliography via ", bib_engine, 
           call. = FALSE)
   })
5: build_bib()
4: latexmk_emu(file, engine, bib_engine, engine_args, min_times, 
       max_times, install_packages, clean)
3: tinytex::latexmk(file, engine, if (biblatex) "biber" else "bibtex")
2: latexmk(texfile, output_format$pandoc$latex_engine, "--biblatex" %in% 
       output_format$pandoc$args)
1: rmarkdown::render("Untitled/Untitled.Rmd", clean = FALSE)

So with no error, it should work as expected. But it seems to error because it does not find the .bst file.

Should this error be quiet for build_bib() and check_blg be run on the .blg file with the error ?
I can confirm that check_blg() works on the produced blg file - it is just not read due to the error.

@yihui
Copy link
Member

yihui commented Sep 28, 2020

@cderv Thanks for the investigation! I see what's wrong now, and will fix it in tinytex soon. For this PR, please ignore this particular issue and don't include plainurl.bst.

yihui added a commit to rstudio/tinytex that referenced this pull request Sep 28, 2020
…tex error occurs, should not stop immediately but try to parse and install missing packages first
@yihui
Copy link
Member

yihui commented Sep 28, 2020

The .bst issue should be fixed in tinytex now.

remotes::install_github('yihui/tinytex')

tinytex::tlmgr_remove('urlbst')

rmarkdown::render("Untitled/Untitled.Rmd")

It should automatically install urlbst.

@nuest
Copy link
Contributor Author

nuest commented Sep 29, 2020

@yihui I can confirm the problem disappeared with tinytex 0.26.3. Thanks @cderv for the hard work of pinpointing the problems.

@cderv :

  1. I think making the label an explicit option would be a nice added feature, though I don't think is is strictly needed for this PR, right?
  2. Yes, unnumbered blocks, i.e. ones with names ending in *, is what this template needs to not use any LaTeX at all for custom environments. Not sure either if this PR should wait for that. Opinion?

@cderv
Copy link
Collaborator

cderv commented Sep 29, 2020

I think making the label an explicit option would be a nice added feature, though I don't think is is strictly needed for this PR, right?

No not needed

Yes, unnumbered blocks, i.e. ones with names ending in *, is what this template needs to not use any LaTeX at all for custom environments. Not sure either if this PR should wait for that. Opinion?

This would be something to support in latex-divs.lua in rmarkdown. One issue though as this is not recognized by Pandoc as a Fenced Div

::: {.claim* data-latex=""}
Something
:::

However, this would be recognized by pandoc, and we could detect the *

::: claim*
Something
:::

But, you can't pass data-latex="" so for now it is not processed by our Lua filter.

I don't think this PR should wait for that either. It works as is, you just need to use latex syntax for unnumbered environement.

What we could quickly do though is this syntax

::: {.claim .unnumbered data-latex=""}
Something
:::

If we detect the class unumbered within a div for latex, we would add the * to the environment name.

Is the star a common pattern for Latex environment ?

I'll review your last chance this week anyway - you could modify your template later. Is the PR ready ?

@nuest
Copy link
Contributor Author

nuest commented Sep 29, 2020

Yes, the PR is ready from my side.

Is the star a common pattern for Latex environment ?

It is used for \(sub, ..)section, \chapter, etc., so I'm inclined to say yes. I like your suggestion to use .unnumbered because it fits what you can do with chapters/sections.

Note the PR includes a small fix for the springer_article() function so R CMD check passes. I'm happy to mention that in the NEWS, if you see the need.

@nuest nuest requested a review from cderv October 6, 2020 09:15
Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

Hi @nuest,

Sorry for the delay.

Here are my last questions / comments of the changes I plan to push before merging.
We'll need also to depend on currently dev tinytex package but I'll add that before merging.

Are you ok with those changes ?

README.md Outdated Show resolved Hide resolved
inst/rmarkdown/templates/lipics/skeleton/skeleton.Rmd Outdated Show resolved Hide resolved
- `\relatedversion` (if there is a related version, typically the ``full version''); please make sure to provide a persistent URL, e.\,g., at arXiv.
- `\begin{abstract}...\end{abstract}`.

\paragraph*{Please do not \ldots} <!-- Do not override the \texttt{\seriesstyle}-defaults} -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe you can write this also as markdown

#### Please do not \dots {.unumbered}
Suggested change
\paragraph*{Please do not \ldots} <!-- Do not override the \texttt{\seriesstyle}-defaults} -->
#### Please do not \ldots {.unnumbered}
<!-- Do not override the \texttt{\seriesstyle}-defaults} -->

not what you want ? Just asking in case there is a reason

- Use pdflatex and an up-to-date \LaTeX{} system.
- Use further \LaTeX{} packages and custom made macros carefully and only if required.
- Use the provided sectioning macros: `\section` (`#` in Markdown), `\subsection` (`##` in Markdown), `\subsubsection` (`###` in Markdown),
`\paragraph`, `\paragraph*`, and `\subparagraph*`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can also be written in markdown

  • \paragraph => ####
  • \paragraph* => #### title {.unnumbered}
  • \subparagraph => #####
  • \subparagraph* => ##### title {.unnumbered}

out of curiosity is there a reason not to use those ?

- fixes test error for springer_article()
- update NEWS file with proper thanks
@nuest
Copy link
Contributor Author

nuest commented Oct 28, 2020

@cderv I've added your changes manually so I can update the single commit from my end. Thank you for your perseverance with this PR! I did a rebase so the merge should work well.

@cderv
Copy link
Collaborator

cderv commented Oct 29, 2020

So it seems you've just forced push and we have now one commit. Some of the changes I suggested where not included. I'll handle that as you seem ok, and check our previous review comment.

If you just merge master in your branch, you can avoid to force-push and we can keep the previous commit in the PR history. When we'll merge in master, we will squash the change so that there will be one commit only in the master history. But I think it is great to keep PR will all the commits and iteration. No worries, just sharing my thoughts.

@cderv
Copy link
Collaborator

cderv commented Oct 29, 2020

@nuest I reverted some changes regarding the environment syntax. As you see there is an issue if you want to use bookdown::pdf_book (rstudio/bookdown#1001) For now it will not work.

I reverted it so that we merge this, and if you desire we work on an update when everything is working.

Test locally and it is ok. (We have issue with our CI currently)

@cderv cderv merged commit f0f03eb into rstudio:master Oct 29, 2020
@nuest
Copy link
Contributor Author

nuest commented Nov 3, 2020

Thanks @cderv !

This really was a more complicated beast than I hoped for. I've now written two articles based on the template though, so the foundation seems to be alright. Please feel free to ping me when the bookdown problem is solved - I'm not sure I'll be able to follow the development over there.

@cderv
Copy link
Collaborator

cderv commented Nov 3, 2020

This really was a more complicated beast than I hoped for.

That was worth it ! thank you for making this PR!

Please feel free to ping me when the bookdown problem is solved

Note that you can subscribe to an issue to reveive updates as Github notification. It is on the right column in the issue rstudio/bookdown#1001
Anyways, I'll try to remember and ping you.

clrpackages pushed a commit to clearlinux-pkgs/R-tinytex that referenced this pull request Nov 5, 2020
…0.27

Alfonso Ruzafa (1):
      fix install-bin-unix script (#243)

Yihui Xie (28):
      make sure that I'll remember to remove the temporary patch for tlmgr.pl in the future
      $(uname) has been stored in $OSNAME
      if tinytex_root() returns '', there is no need to check further if this root directory is TinyTeX (it definitely isn't)
      need to group the latter two expressions
      fix a problem discovered at rstudio/rticles#288 (comment): when a bibtex error occurs, should not stop immediately but try to parse and install missing packages first
      use xfun::grep_sub() in a few other places
      xfun 0.18 is on CRAN now
      keep (cached) R packages up-to-date
      only update packages that can be possibly updated (e.g. on Linux, don't update /usr/lib/R/library)
      suppress the false alarm on Windows when running `tlmgr path add`:
      resolve action earlier
      don't set R_LIBS_USER if it has been set
      always set R_LIBS_USER on *nix
      close rstudio/tinytex-releases#9: remove %APPDATA\TinyTeX% twice like https://github.com/yihui/tinytex/blob/ce0e9d311830673dbfd2aa945145c54c45020b7f/tools/install-windows.bat#L34 because rd once can't really remove this dir sometimes
      forgot to add path
      delete install-tl and install-tl-windows.bat because users don't need them, and they can confuse users: rstudio/tinytex-releases#9
      patch tlmgr.pl on Windows to avoid the false alarm: http://www.tug.org/svn/texlive/trunk/Master/texmf-dist/scripts/texlive/tlmgr.pl?r1=56562&r2=56561&pathrev=56562
      add ... arguments to r_texmf() and tlmgr_conf()
      add/remove R's texmf tree quietly
      add the `firstaid` package as discovered in https://travis-ci.com/github/yihui/tinytex/jobs/396057138
      one more package: https://travis-ci.com/github/yihui/tinytex/jobs/396082629
      remove the patch introduced in d9ed3c452bbcb526ad682b18a99207d084ce1dbd; the problem seesm to have been fixed upstream
      the bug in `tlmgr info --list --data relocatable` has been fixed, so remove this patch, and add a test so that we will be able to discover this problem if it occurs again
      add a tlmgr_repo function to query or set CTAN repository
      import two more functions from xfun
      wrong logic here: the commit ffce7ab7152cb241d26a299208c8642fda0d204e should be correct; not sure how I screwed it up later
      fix #252: for Linux machines, the arch must be x86_64 for the prebuilt binary version of TinyTeX to be installed; if it is not x86_64, install from source
      CRAN release v0.27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants