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 working-directory input to setup-r-dependencies #441

Merged
merged 13 commits into from
Nov 22, 2021
Merged

Add working-directory input to setup-r-dependencies #441

merged 13 commits into from
Nov 22, 2021

Conversation

TNonet
Copy link
Contributor

@TNonet TNonet commented Nov 18, 2021

This is my attempt to close #438.

Looking for comments and feedback.

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2021

Codecov Report

Merging #441 (a641d28) into master (e2dce68) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #441   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            1         1           
=========================================
  Hits             1         1           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ee06a6...a641d28. Read the comment docs.

@jimhester
Copy link
Member

This looks good, could you test it out and verify it does what you want? You can use

- uses: TNonet/actions/setup-r-dependencies@master

To reference your code in your test workflow.

@TNonet
Copy link
Contributor Author

TNonet commented Nov 19, 2021

Hi Jim,

I was wondering how to test these actions. It is good to know how to reference actions from other repos.

I will report back (and most likely update the PR).

@jimhester
Copy link
Member

Thanks for working on this! I think I have been able to simplify your implementation a bit (31c24cc), and it seems to be working in a test. https://github.com/jimhester/r-actions-test/runs/4268523360?check_suite_focus=true#step:4:397

Try it out on your repo and let me know!

@TNonet
Copy link
Contributor Author

TNonet commented Nov 19, 2021

Thanks Jim,

I agree your simplification is preferable. I was looking for some documentation on how paks::pkg_deps accepts a directory that is not the current directory.

This line is key!

"local::${{ inputs.working-directory }}"

I can confirm that the change works on my repo!

@riccardoporreca
Copy link
Contributor

@TNonet, @jimhester, great to have this feature!

The same approach could be potentially extended to other actions, like check-r-package.
I am just wondering if working-directory is still the right name for the input: It is in fact the package source directory (and not the working directory where all steps are executed). This might also be confusing with the working-directory option of jobs.<job_id>.steps[*].run.

Not sure what good alternatives would be: pak calls the argument root as the "path to the tree" (see e.g. local_deps() local_install); in pkgdepends "directory that contains a package" (pkg_refs()); "path to a package directory" in rcmdcheck().
Perhaps package-path, package-directory, or package-root?

@TNonet
Copy link
Contributor Author

TNonet commented Nov 21, 2021

@riccardoporreca I agree that working-directory might not be the best name choice anymore. I think any of your options would be fine.

One thought that did come to mind is that devtools::install_github uses a parameter called subdir. This is actually how our package is installed from github.

library(devtools)
install_github("hazimehh/L0Learn", subdir="R")

So I would want to offer package-subdir or package-subdirectory as alternatives to more closely match my belief for the intended use case, which is supporting multi-language repos with the following layouts.

https://github.com/USER_ORG/PKG_NAME/
├─ .github/workflows 
│ # actions run from this directory in most cases, therefore requiring `package-subdir` input in R actions to properly test R package
├─ python/
│  ├─ PKG_NAME/  # containing py src files
│  ├─ setup.py
├─ R/
│  ├─ DESCRIPTION
│  ├─ R/  # containing R src files
│  ├─ ...
├─ OTHER_LANGS/
├─ ...

@jimhester
Copy link
Member

I think working directory seems reasonable, as you are specifying the working directory for the custom action.

@github-actions
Copy link

github-actions bot commented Nov 5, 2022

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

@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

Successfully merging this pull request may close these issues.

Allow setup-r-dependencies to accept working-directory or similar functionality
4 participants