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

Move R dependencies to environment.yml and set r-akima version #2725

Closed
wants to merge 4 commits into from

Conversation

sloosvel
Copy link
Contributor

@sloosvel sloosvel commented Jul 19, 2022

Description

This PR tries to move the R dependencies to the environment file as suggested in #2695. It also pins the akima package to the latest working version, as the newest ones break several recipes.

It would be good if someone double checked if that is what it needs to be done.


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

New or updated recipe/diagnostic

- [ ] 🧪 Recipe runs successfully
- [ ] 🧪 Recipe is well documented
- [ ] 🧪 Figure(s) and data look as expected from literature
- [ ] 🛠 Provenance information has been added

New or updated data reformatting script

- [ ] 🛠 Documentation is available
- [ ] 🛠 The dataset has been added to the CMOR check recipe
- [ ] 🧪 Numbers and units of the data look physically meaningful


To help with the number of pull requests:

@bouweandela
Copy link
Member

My apologies, it seems we have been working on the same thing. I also worked on this in #2663, do you think it would be an option to merge that one? If you think it is too much just before a release, you could also have a look at the changes I made in there to see what is still missing here.

@sloosvel sloosvel closed this Jul 19, 2022
Copy link

@zklaus zklaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conda-forge doesn't use capitalizations, even when they are used upstream, so all package names should be converted to all lower-case.

- r-docopt
- r-functional
- r-ggplot2
- r-git2r # dependency of lintr
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be removed since conda will take care of dependencies.

- r-abind
- r-akima<=0.6-2.3
- r-climdex.pcic
- r-ClimProjDiags
- r-docopt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, r-dotcall64 seems to be missing. Was this on purpose?

@@ -37,9 +37,29 @@ dependencies:
- ncl
- r-base
# R packages needed for development
- r-abind
- r-akima<=0.6-2.3
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remember to add the pinning to https://github.com/conda-forge/esmvaltool-suite-feedstock/blob/46332d1f303b96679093b424fb0691ac39083e8b/recipe/meta.yaml#L147 when it comes to the release. Perhaps best to have an issue on the feedstock?

Comment on lines 64 to 65
- r-yaml
- r-udunits2 # needed by the docker build
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- r-yaml
- r-udunits2 # needed by the docker build
- r-udunits2
- r-yaml

@sloosvel
Copy link
Contributor Author

Sorry it seems like there is a PR mess. I closed this to take care of everything at #2663 .

@zklaus
Copy link

zklaus commented Jul 19, 2022

No worries. I saw Bouwe's comment and knew this was a risk.

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

Successfully merging this pull request may close these issues.

recipe_miles_* broken due to newest akima package versions
3 participants