showyourwork / showyourwork-action

Build showyourwork articles on GitHub Actions
MIT License
2 stars 4 forks source link

Render `latexdiff` on pull requests #2

Closed MilesCranmer closed 2 years ago

MilesCranmer commented 2 years ago

This adds latexdiff functionality to publish.js. It introduces two new parameters: build-diff-on-pull-request and latexdiff-url that configure whether or not to build the diff, and what latexdiff URL to download, respectively. This addresses https://github.com/showyourwork/showyourwork/issues/123.

Right now there are a couple potential issues: the diff will only be generated with respect to the contents of ms.tex. If there are additional files, those will not be diffed. This could be fixed by using latexpand to flatten the files before diffing. However, I think this will work as a start.

I'm not sure how to actually test this @rodluger - could you have a look?

MilesCranmer commented 2 years ago

Also, it might be better to render both the PDF, as well as the diff'd PDF. Sometimes the diff may have weird behavior, so it would be good to have the normal render just in case.

rodluger commented 2 years ago

Also, it might be better to render both the PDF, as well as the diff'd PDF. Sometimes the diff may have weird behavior, so it would be good to have the normal render just in case.

Yes, good idea. In that case, make a temporary copy of the main PDF before running showyourwork build. Then rename the new one to diff.pdf and restore the original PDF. Note that the PDF is usually called ms.pdf but can be changed by the user. You can get the actual file name from the config file by doing this.

rodluger commented 2 years ago

Actually the tex file name is also configurable. You'll want to change src/tex/ms.tex to src/tex/${config['ms_name']}.tex.

MilesCranmer commented 2 years ago

Thanks, added! I also use latexpand now, which will deal with multi-file latex documents. Let me know what you think.

rodluger commented 2 years ago

It would also be nice to edit the comment posted by the bot to include a link to the diff: https://github.com/showyourwork/showyourwork-action/blob/main/process-pull-request/action.yml#L111

MilesCranmer commented 2 years ago

Edited the comment to include the diff.pdf URL!

MilesCranmer commented 2 years ago

Any idea how to actually test this? I guess one could create an example repo, and specify my branch in an action?

rodluger commented 2 years ago

Any idea how to actually test this? I guess one could create an example repo, and specify my branch in an action?

Yes! That's what I would do.

rodluger commented 2 years ago

Also, can you add some lines to the README listing the new action variables?

MilesCranmer commented 2 years ago

Thanks. I did a test here: https://github.com/MilesCranmer/showyourwork-test/runs/7310346935?check_suite_focus=true but it doesn't seem to correctly run the code to compile diff.pdf. The bot correctly comments the link here: https://github.com/MilesCranmer/showyourwork-test/pull/1#issuecomment-1182507688, but it's not actually generated. Any idea from glancing at the logs why that piece of code doesn't run?

rodluger commented 2 years ago

You had to change the action here as well. I'll run it again and see if it works.

rodluger commented 2 years ago

Making several little tweaks still. I think I'm close to getting it to work.

rodluger commented 2 years ago

OK, this is now in your hands @MilesCranmer as perl has only ever caused me pain: https://github.com/MilesCranmer/showyourwork-test/runs/7322402202?check_suite_focus=true#step:3:169

MilesCranmer commented 2 years ago

Thanks! Seems like I got perl to work. Except, now, it is still not generating the diff. I wonder if it is something to do with caching? https://github.com/MilesCranmer/showyourwork-test/pull/7

MilesCranmer commented 2 years ago

Ah, wait, it's still a perl problem... I'll keep trying.

MilesCranmer commented 2 years ago

Working: https://github.com/MilesCranmer/showyourwork-test/pull/9

image

I hope the last few days of Perl-induced pain has resulted in a useful feature for people 😅

MilesCranmer commented 2 years ago

By the way: latexdiff seems to have required some LaTeX packages despite being a standalone script, so I installed texlive-base which is the core latex packages. This package is only ~60 MB so it seems okay to have. What do you think?

rodluger commented 2 years ago

Awesome, thanks for the great contribution! The diff generation seems to have added about 2 min of overhead, which isn't terrible. But it might be worth caching the texlive-base install like we currently do with conda. But I think you've done enough already! Happy to merge this and punt on the caching. But one last thing: can you add a couple sections to the Inputs section of the README with the new config settings (build-diff-on-pull-request, latexdiff-url, and latexpand-url)?

MilesCranmer commented 2 years ago

Described them on the README!

I don't know how to set up the caching of texlive, but that definitely sounds like a good idea for the future.

rodluger commented 2 years ago

Awesome, thank you! I'll update the v1 tag momentarily to this commit.

MilesCranmer commented 2 years ago

Awesome, sounds good! Thanks!

rodluger commented 2 years ago

Ok, latexdiff now active on showyourwork-action@v1.