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

Package fails to install under R 4.0 #456

Open
klmr opened this issue Jul 2, 2024 · 14 comments
Open

Package fails to install under R 4.0 #456

klmr opened this issue Jul 2, 2024 · 14 comments

Comments

@klmr
Copy link

klmr commented Jul 2, 2024

According to its DESCRIPTION, the package requires R 4.0. However, the following line uses the native pipe operator, which is only available from R 4.1 onwards:

"The following variables have an unexpected type:" |>

@bms63
Copy link
Collaborator

bms63 commented Jul 2, 2024

Ohhh...goodspot @klmr !!

@pharmaverse/admiral and @pharmaverse/admiral_comm

ummm....how should we approach:

  • Remove native pipe and update to magrittr pipe
  • Require admiral/admiraldev to only work with 4.1 and keep native pipe?

@manciniedoardo
Copy link
Collaborator

Ohhh...goodspot @klmr !!

@pharmaverse/admiral and @pharmaverse/admiral_comm

ummm....how should we approach:

  • Remove native pipe and update to magrittr pipe
  • Require admiral/admiraldev to only work with 4.1 and keep native pipe?

I'd vote for the second option - R 4.1 is quite old anyway.

@bms63
Copy link
Collaborator

bms63 commented Jul 2, 2024

So should we handle this as an official bug fix with a re-release?

@ddsjoberg
Copy link
Collaborator

So should we handle this as an official bug fix with a re-release?

I don't think so. The min R version is something that is not correct is many many many pkgs on CRAN. I would just fix in dev, and it'll be correct in the next release

@bundfussr
Copy link
Collaborator

Could we add a CI/CD workflow which tries to install and test the package with the minimum versions stated in DESCRIPTION?

@ddsjoberg
Copy link
Collaborator

Perhaps we can just say somewhere on the website we support the last 2, 3, 4 (whatever we choose) versions of R, and supplement our workflows with the R CMD Check from Posit that makes it easy to check the last n versions of R.

          - {os: ubuntu-latest,   r: 'devel', http-user-agent: 'release'}
          - {os: ubuntu-latest,   r: 'release'}
          - {os: ubuntu-latest,   r: 'oldrel-1'}
          - {os: ubuntu-latest,   r: 'oldrel-2'}
          - {os: ubuntu-latest,   r: 'oldrel-3'}
          - {os: ubuntu-latest,   r: 'oldrel-4'}

@millerg23
Copy link
Contributor

We say we develop using the earliest of the last 3 versions of R, in documentation:
https://pharmaverse.github.io/admiraldev/articles/programming_strategy.html#r-and-package-versions-for-development

@bms63
Copy link
Collaborator

bms63 commented Jul 2, 2024

I believe we are testing 4.4, 4.3, 4.2 with our CI actions. We should add 4.1 to mix.

image

@ddsjoberg
Copy link
Collaborator

We already have the Posit workflow file here, and we can just uncomment the lines.

- {os: ubuntu-latest, r: 'release'}

@bms63
Copy link
Collaborator

bms63 commented Jul 2, 2024

ah i see - ummm... can we re-work the admiralci one since it is used in admiral as well as well as the other admiral packages. Just be nice to all pull from the same spot. I'm totally fine to simplify the one in admiralci!

@bundfussr
Copy link
Collaborator

We say we develop using the earliest of the last 3 versions of R, in documentation: https://pharmaverse.github.io/admiraldev/articles/programming_strategy.html#r-and-package-versions-for-development

There it says

Currently we test admiral against the three latest R Versions and the closest snapshots of packages to those R versions.

Is the part "and the closest snapshots of packages to those R versions" up to date? I remember vaguely that we changed our strategy to use the latest version of the packages for all R versions.

@bundfussr
Copy link
Collaborator

I believe we are testing 4.4, 4.3, 4.2 with our CI actions. We should add 4.1 to mix.

I think testing more R versions doesn't solve the issue. If we had tested with 4.1, we still wouldn't have discovered the problem.

If we can add a workflow which tests the package on the minimum R version and minimum package versions as specified in the DESCRIPTION file, it would be helpful. @cicdguy , is this possible?

Otherwise, I would rely on the developers and users that they report wrong minimum versions.

@bms63
Copy link
Collaborator

bms63 commented Jul 8, 2024

I believe we are testing 4.4, 4.3, 4.2 with our CI actions. We should add 4.1 to mix.

I think testing more R versions doesn't solve the issue. If we had tested with 4.1, we still wouldn't have discovered the problem.

If we can add a workflow which tests the package on the minimum R version and minimum package versions as specified in the DESCRIPTION file, it would be helpful. @cicdguy , is this possible?

Otherwise, I would rely on the developers and users that they report wrong minimum versions.

My understanding is that we are saying in our current DESCRIPTION that admiral works with R version 4.0. However, we are using a pipe |> that was introduced in R version 4.1 in a few lines of code

If we had been testing the package build on 4.0 I believe we would of found this issue out. Maybe we should just give a try to experiment.

Anyways, I do think the minimum version we suggest in the DESCRIPTION should be tested no matter what! :)

@cicdguy
Copy link
Collaborator

cicdguy commented Jul 9, 2024

If we can add a workflow which tests the package on the minimum R version and minimum package versions as specified in the DESCRIPTION file, it would be helpful. @cicdguy , is this possible?

Yes this is possible. We have a dedicated GitHub Action that we use in NEST already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

7 participants