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

packages file or packages text #6

Closed
schloerke opened this issue Nov 9, 2023 · 2 comments · Fixed by #8
Closed

packages file or packages text #6

schloerke opened this issue Nov 9, 2023 · 2 comments · Fixed by #8

Comments

@schloerke
Copy link
Collaborator

For build-wasm-packages's packages input parameter, I am wondering what the underlying motivation for using a new/unfamiliar file format.

I'm not against it, just want to understand it.



Currently, r-lib/actions's setup-r-dependencies has three locations to where it will look for packages to install.

  • needs - Config/Needs fields to install from the DESCRIPTION, the
    Config/Needs/ prefix will be automatically included.
  • packages: - default deps::., any::sessioninfo. Which package(s) to
    install. The default installs the dependencies of the package in the
    working directory and the sessioninfo package. Separate multiple packages
    by newlines or commas.
  • extra-packages - One or more extra package references to install.
    Separate each reference by newlines or commas for more than one package.
  1. It will look for Config/Needs/<NEEDS> via the needs parameter.

  2. packages is a default that hardly anyone sets manually because in the context of checking an R package, this field should not be changed. (You will typically always want to install deps::., any::sessioninfo

  3. extra-packages is used by authors who do not want to clutter their DESCRIPTION file or for one-off changes.


If possible, I'd like to leverage r-lib/actions existing approach.

If r-wasm/actions is to be used within R packages, then I'd like to utilize the DESCRIPTION's Config/Needs/wasm field as the typical entrypoint, with a backup of packages or extra-packages.

If r-wasm/actions is to be used within a localized wasm-friendly CRAN repo, then packages file is a great choice for providing information.

If using a packages file, should it ever contain more fields such as description or R version or any other meta data? If so, should we upgrade the file to a yaml (to support comments) or DCF file (similar to the DESCRIPTION file)?

@schloerke
Copy link
Collaborator Author

After typing all this out, I am understanding it as it should be an easy way for devs to declare what packages are being used. While using packages as a string in the GHA workflow, those values are hidden away from the repo. Therefore, we should keep packages as a filename whose file contents contain the packages being used.

It still leaves the questions

  • Will there ever be any more information?"
  • If so, should we upgrade it to a yaml file? (This would allow for comments and or key/value pairs in the future, such as R version or any other meta info)

@georgestagg
Copy link
Member

For build-wasm-packages's packages input parameter, I am wondering what the underlying motivation for using a new/unfamiliar file format.

The current one-line-per-entry text format for the packages file is simply legacy from when we first started experimenting with building R packages for Wasm. Originally the process was powered by shell scripts, where using that kind of file as input makes a lot of sense.

I am not opposed to changing the format. In fact, I really like the idea to match setup-r-dependencies, where the relevant data is in the GHA workflow file and DESCRIPTION. Consistency is good and enables knowledge transfer for current users of r-lib/actions. So, my vote would be to indeed move towards that model.

The only issue I see is that I'd still like to be able to use this workflow in a kind of meta-repo, one without an R package or DESCRIPTION file in it. Under that use case, I still see the benefit of a flat text file. I think a yaml file would be fine. Another format I considered was renv lock files. However, I believe it should be possible to specify packages easily when writing such files by hand, and so I steered away from the lock file format.


Will there ever be any more information?

I am unsure, perhaps the intended webR version or R version would be useful? In the future we could use tagged r-wasm/webr Docker containers to build packages for a specific version of webR.

Note that, currently, package list entries are passed to rwasm::add_list(). This supports pkgdepnds style package references, which are already fairly expressive so extra information such as specific package source and versioning can be encoded. I think things like deps::. should also work.

Either way, switching to yaml would future-proof these files in a nice way for if/when we do want to track [web]R versions or other information. I would be OK with that.


Right, so what do you think about the following plan?

First, we switch around build-wasm-packages so as to match setup-r-dependencies behaviour:

  • Add the needs parameter and automatically search for Config/Needs/[needs] in the DESCRIPTION file for packages.

  • Change packages to take in text. Default to something like deps::. or file::. so that "this" package is installed by default. Ensure such references work with rwasm::add_pkg() as expected.

  • Add an extra-packages text field.

The above three package lists should be combined into an character R vector and passed to rwasm::add_pkg(), rather than using add_list() like currently.

In addition, we handle the case where there is no DESCRIPTION file in the repo and it's more like a meta-package:

  • Tweak rwasm::add_list() to work by taking in a yaml file and parsing it for a list of packages. Otherwise, it works as before. Perhaps we also rename it to rwasm::add_yaml() or something similar.

  • Ensure that the action still works when there is no DESCRIPTION file to be read for Config/Needs/wasm.

  • If packages is a single line and ends in .yml, change behaviour so as to pass the given filename to rwasm::add_list(). This will match current behaviour, and the special case should be documented clearly.

@schloerke schloerke mentioned this issue Nov 15, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants