grimbough / bioc-actions

GitHub Actions for developing and maintaining Bioconductor packages
MIT License
11 stars 3 forks source link

Display output from installation step #8

Closed csoneson closed 1 year ago

csoneson commented 1 year ago

Hi @grimbough - I wanted to bring up a suggestion/question about the build/install/check action. The issue we noticed recently is that if the installation of the package fails, e.g. since some dependency is not available, no output is displayed (see e.g. https://github.com/fmicompbio/mutscan/actions/runs/3273531248/jobs/5385851425#step:12:149). As a comparison, if we instead run rcmdcheck() manually, we immediately see what the problem is (see e.g. https://github.com/fmicompbio/mutscan/actions/runs/3279945799/jobs/5400120351#step:12:24). I'm wondering what you think about not redirecting stdout/stderr, or maybe to automatically upload the install-out.txt file as an artifact in case of failure (actually I was trying to do this, but I could not locate the file - https://github.com/fmicompbio/mutscan/actions/runs/3273531248/jobs/5385851425#step:15:20). Or maybe there's a better option that I'm not considering.

grimbough commented 1 year ago

I think my intention was for the next line with cat mutscan.install-out.txt was to print the contents and let some see any issues. However it looks like if it fails then you don't get to run that line, so it's a bit useless. I'll have a look and get back to you.

grimbough commented 1 year ago

Hi @csoneson

It took me a little while to remember why the action was written the way it is. I don't really want to stop the redirect because pushing the output of the R CMD INSTALL step to a text file and then using that in the subsequent R CMD check is a weird undocumented method that the Bioc build system uses to speed things up. IIRC there can occasionally be edge cases where this works differently from the standard procedure, which is why I try to emulate it here.

I've added another step to the action that will print the contents of .install-out.txt regards of whether the steps before are successful or not. Hopefully this helps identify problems that would otherwise be printed to screen e.g.

https://github.com/grimbough/biocActionsExamples/actions/runs/3295837347/jobs/5434855932#step:6:167

I've also added the location of the install log as an output to the action, so you can use that to upload the log as artefact if that's also useful. You can see an example of that in:

https://github.com/grimbough/biocActionsExamples/actions/runs/3295837347/workflow#L75-81

Those examples of for the devel tag of the actions, but I've merged the changes into v1 and they should already be available to you. Let me know how it works out.

csoneson commented 1 year ago

Brilliant, thank you @grimbough! The printing of the installation log works great (and this is actually enough for us). Uploading the file seems to cause issues with the path on both Windows and macOS: https://github.com/fmicompbio/mutscan/actions/runs/3296114516/jobs/5435397086. It seems that also in your example there is an issue finding the file, but the upload still works later on (maybe as it's in the working directory rather than the parent directory?): https://github.com/grimbough/biocActionsExamples/actions/runs/3295837347/jobs/5434855932#step:6:166

grimbough commented 1 year ago

Thanks for pointing that out. I do like an error that still produces the correct result! I turns out that you can't normalizePath() on a file that doesn't exist yet, but you do get back the original input, which was sufficient my example to still find the log file. I've amended that action to try and do that a bit better. Hopefully it works cross platform.

csoneson commented 1 year ago

Thank you! I think it is still looking one level too high - e.g. in your example here it can't find the file, and in my case it can't find the file /Users/runner/work/mutscan.install-out.txt (here), but /Users/runner/work/mutscan/mutscan.install-out.txt seems to exist (here).

It's not really clear to me what the issue is - could it be that here the job is already executed in the designated working directory, so there's no need to specify it again here?

grimbough commented 1 year ago

Ok, I think I've tested this properly now.

The first example you listed above was actually the original, definitely failing, version of the code. I think I must have hit "only re-run failing workflows" after an updated to the action and so that part wan't executed again.

None the less the problem still stood, and your suggestion about already running the appropriate directory was correct. I was then resolving a relative path again that didn't need to happen. I think I can just get away with a getwd() to find the location that step is working in.

This workflow run tests on all three platforms and also root and sub-folder locations for the package to be tested. Hopefully I've resolved the issue!

csoneson commented 1 year ago

I can confirm that this now also works as expected for our repository. Thanks a lot for the quick fix!