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

Investigate removal of gdebi in setup-pandoc #527

Closed
nikeee opened this issue Mar 12, 2022 · 8 comments
Closed

Investigate removal of gdebi in setup-pandoc #527

nikeee opened this issue Mar 12, 2022 · 8 comments

Comments

@nikeee
Copy link
Contributor

nikeee commented Mar 12, 2022

It requires an extra step to install gdebi-core in the setup-pandoc action.

Describe the solution you'd like
Using a combination of dpkg and apt actually does the same, so gdebi should not be needed. This could speed up installations:
https://askubuntu.com/a/621461

@nikeee nikeee changed the title Investigate removal of gdebi Investigate removal of gdebi in setup-pandoc Mar 12, 2022
@gaborcsardi
Copy link
Member

The whole setup-pandoc step takes about 10-15 seconds. On my VM the step that installs gdebi-core takes about 2s. I don't think it is worth changing this.

The dpkg + apt-get install -f solution is also not ideal non-interactively, because the first step will fail, and we need to ignore this failure, and depending on how it fails, the second step might end up with pandoc installed or might not.

@nikeee
Copy link
Contributor Author

nikeee commented Mar 12, 2022

because the first step will fail, and we need to ignore this failure, and depending on how it fails, the second step might end up with pandoc installed or might not.

Could you point me to some of these cases? I cannot reproduce this and would like to, so I can understand the problem better.
I just spun up an ubuntu:latest container and plain dpkg -id the latest deb package form the releases page and did not have missing dependency errors. So apt install -f would not actually be needed.

It's also mentioned in the setup instructions that pandoc for linux does not require external dependencies:

The executable is statically linked and has no dynamic dependencies or dependencies on external data files.

They also recommend using dpkg -i to install the package.

Another solution than installing the deb package would be installing the tarball, which just contains a binary that has to me moved to some ./local/bin dir in the PATH.

One thing to also consider is using the GH Actions tools cache. This could also save some time:
https://github.com/actions/toolkit/tree/master/packages/tool-cache#cache

takes about 10-15 seconds.

I've got a workflow that runs in about 30s, so this is 1/3-1/2 of the total time spent. I'd like to keep this time as low as possible and I'm willing to help to address this.

@gaborcsardi
Copy link
Member

gaborcsardi commented Mar 12, 2022

Could you point me to some of these cases? I cannot reproduce this and would like to, so I can understand the problem better.

E.g. if a broken deb file is downloaded, or we download the wrong deb file.

pandoc for linux does not require external dependencies:

That might be the current case, but I don't know if it is true for all past versions, and whether it will be true for future versions.

takes about 10-15 seconds.

I've got an action that runs in about 30s, so this is 1/3-1/2 of the total time spent. I'd like to keep this time as low as possible and I'm willing to help to address this.

That's the whole action. The gdebi download is ~2s.

I think the current method is more robust, and it is also very well tested, so we'll not change it to gain two seconds.

@gaborcsardi
Copy link
Member

Btw. these actions are for testing R packages and we also need gdebi to install R, so most of our users will not even see that 2s improvement.

@nikeee
Copy link
Contributor Author

nikeee commented Mar 12, 2022

so we'll not change it to gain two seconds.

I'm actually not proposing to ehnance 2s. Rather, my suggestion is that 1. no tools should get installed that aren't needed for the task (polluting globally installed tools might interfere in a bad way with other steps). Secondly, once the deb file is installed without dependencies, it can be cached with gh tools cache. That would reduce the time from 15s to effectively 0s. So it's not the 2s improvement which I've set as a target here; it's only the first step.

we also need gdebi to install R

Well, that's something that I'd consider out-of-scope for an action that sets up pandoc. Relying on implementation-specific side-effects is even considered bad practice by some people.

That might be the current case, but I don't know if it is true for all past versions,

The current case also does not work for all past versions. The first version of pandoc that comes with a deb package on GitHub is 1.13.2. Not only that, but there are versions released after that that do not have a deb package.

Thank you for your feedback.

@gaborcsardi
Copy link
Member

polluting globally installed tools might interfere in a bad way with other steps

I don't agree that a CI VM should be minimal. Most of the time your software will not be used in minimal environments, either. GHA's Ubuntu VMs are certainly not minimal to start with: https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md#ubuntu-20043-lts
But anyway, this is pretty much a mute argument, gdebi will certainly not interfere with anything else.

it can be cached with gh tools cache. That would reduce the time from 15s to effectively 0s.

I am pretty sure that the tool-cache is not kept on the VMs between runs, so it is not useful for most people. You could use a proper cache action, of course, which uploads/download to/from S3 or a similar storage place. It will be more than 0s, but could be fast. I don't think it is worth the complications to add this to setup-pandoc, though.

Since you are implementing your own action, you can do this there if you want, with of without our setup-pandoc action.

Well, that's something that I'd consider out-of-scope for an action that sets up pandoc.

Not really, look at the title of this repository.

Relying on implementation-specific side-effects is even considered bad practice by some people.

Nothing like that is happening here.

FWIW, the pandoc deb package does have dependencies, e.g. libgmp and zlib1g. These are typically present even on the most minimal Ubuntu containers, but still the current way with gdebi will work even if they need to add more dependencies.

@nikeee
Copy link
Contributor Author

nikeee commented Mar 12, 2022

In case anyone has similar issues, I've hard-forked the action in question: https://github.com/nikeee/setup-pandoc

It is available in the market place:
https://github.com/marketplace/actions/setup-pandoc

Main differences:

  • When not specified, it will use the latest pandoc version available
  • Support for tool cache
  • Support for other Linux distributions beside debian derivatives

Also, runs in 0s-1s without any cache:
image

I've also backported problems I've found to the original action, fixing #515: #528

@github-actions
Copy link

github-actions bot commented Nov 5, 2022

This issue has been automatically locked. If you believe you have found a related problem, please file a new issue and include a link to this issue

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants