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 migration for r-base 4.3 #4363

Merged
merged 8 commits into from
Jun 10, 2023
Merged

Conversation

dpryan79
Copy link
Contributor

@dpryan79 dpryan79 commented Apr 22, 2023

Note that the previous r-base migration included the following lines (I've adjusted the commit message to be appropriate here):

  operation: key_add
  commit_message: "Rebuild for r-base 4.3"
  primary_key: r_base
  automerge: True
  longterm: True
  include_noarch: True
  pr_limit: 40

I don't see enough in the documentation to determine if that's needed here as well.

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@dpryan79 dpryan79 requested a review from a team as a code owner April 22, 2023 18:26
@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@dpryan79
Copy link
Contributor Author

dpryan79 commented May 1, 2023

@conda-forge/core Thoughts on starting an r-base migration?

@isuruf
Copy link
Member

isuruf commented May 1, 2023

You need all of the options in your original message

@dpryan79
Copy link
Contributor Author

dpryan79 commented May 1, 2023

@isuruf Thanks, I've updated the PR accordingly.

@@ -678,8 +678,7 @@ root_base:
ruby:
- 2.5
- 2.6
r_base:
- 4.1
r_base: # [not win]
Copy link
Member

Choose a reason for hiding this comment

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

When dropping 4.1, we need to skip windows entirely. Not sure how to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we could stick with 4.1 for windows still. That'd bypass the issue.

@dpryan79
Copy link
Contributor Author

dpryan79 commented May 3, 2023

I added 4.1 back in for windows only, since it's the last thing we got to build on it.

@mbargull
Copy link
Member

mbargull commented May 3, 2023

@conda-forge/r-base , @conda-forge/r :
We currently have the question whether we want to continue to support R 4.1 on Windows or not.
AFAIU, supporting R 4.2 and R 4.3 on Windows requires an updated toolchain compatible with Rtools 4.2/4.3 which we as of now not have yet.
This leaves us with 2 options:

  1. Drop Windows support for new builds of R packages until we have an updated toolchain and built R 4.2/4.3.
  2. Continue to support R 4.1 on Windows.

The second option imposes additional maintenance burden, of course:

  1. r-base=4.1's own Windows build needs to be fixed (currently has build issues due to MikTex), and
  2. more importantly, some R packages have issues/might not build with R 4.1 anymore and thus need manual interventions, etc.

Dropping support for base-level packages is rarely a nice thing, but ultimately it is a question of of maintenance burden and maintainability under resource (=manpower) restrictions. What do you think, @conda-forge/r-base , @conda-forge/r ?

@dpryan79
Copy link
Contributor Author

dpryan79 commented May 3, 2023

At this point I'm happy dropping windows support, but (A) I fully recognize that may be controversial and (B) I have no clue how to do that in a migration.

@mfansler
Copy link
Member

mfansler commented May 3, 2023

Dropping R 4.1 (and Windows R)

As much as I'd like to see support for Windows continue, we definitely have pressure on multiple fronts to drop Windows R 4.1 specifically:

I don't know a workaround for the former, so those packages have already been losing Windows support. Attempts at solving the latter by gutting the rwinlib dependencies and linking against Conda Forge libraries have been a crapshoot; it's often a lot of time (at least for me), and doesn't always work in the end.

If someone is going to do the toolchain work, I'd rather see Windows R 4.2/4.3 support added than toolchain maintenance for R 4.1.

(Conditional) Suggestion

If it is decided to drop Windows R, then I'd suggest we do at least a last round of non-Windows R 4.1 builds against the current pinnings. There are several r-base:4.1.x library migrations that have not gone through, specifically because Windows failed (see https://github.com/conda-forge/r-base-feedstock/pulls).

I adjusted a few feedstocks to build variants zipped over R 4.1/4.2 to work around this divergence in migrations, and it would be nice to get those reconciled first.

@dbast
Copy link
Member

dbast commented May 11, 2023

Its a pity that the windows toolchain got so far behind. Lots of people got started with R on windows with conda just by installing miniconda/miniforge without the need to have admin access to install wsl2 or docker.

Though the last R packages buildout by Anaconda last year was also without windows (and without osx) https://github.com/AnacondaRecipes/aggregateR/tree/R-4.2.0.

With dropping R windows builds, should we then actively add a section to the conda-forge user instructions that explains the option to use linux packages on windows via wsl2 / docker?... This not only is an alternative for windows users, but also gives them more packages then bevor as the linux-64 package list is more complete.

@dpryan79
Copy link
Contributor Author

@dbast It certainly wouldn't hurt.

@cbrueffer
Copy link
Member

I'm OK with dropping Windows, there seem no other good solution at the moment.

@isuruf
Copy link
Member

isuruf commented May 12, 2023

I adjusted a few feedstocks to build variants zipped over R 4.1/4.2 to work around this divergence in migrations, and it would be nice to get those reconciled first.

Since we will be dropping R 4.1, there's no need right?

@mbargull
Copy link
Member

Thanks for all the input!

I've updated r-base's 4.1.x branch so that the package is getting rebuild for the pending library migrations.
That way we can address concerns mentioned by @mfansler (and @h-vetinari and others elsewhere) about r-base=4.1 currently holding back some other migrations.
(We needed to look into getting some TeX distribution available anyway for whenever we have an updated Windows compiler toolchain to build R >=4.2 ; and rebuilding downstream packages can be handled independently from a 4.3 migration.)

This is not to suggest to necessarily continue to support R 4.1 package builds.
It merely gives the option to do so, but @conda-forge/r is not and should not feel to be obliged to do it.


AFAIUI, to move this PR forward, we need to either

  1. write a custom migration that can handle the removal of Windows builds,
  2. merge as-is and let @conda-forge/r continue to handle R 4.1 support for Windows until we have R >=4.2 builds available.

(I can't help with 1. since I'm not familiar with writing migrations; hence I did what I could and just re-enabled the other option...)

@mfansler
Copy link
Member

I adjusted a few feedstocks to build variants zipped over R 4.1/4.2 to work around this divergence in migrations, and it would be nice to get those reconciled first.

Since we will be dropping R 4.1, there's no need right?

@isuruf probably right, but it's done now since the migration in question has merged.

@mfansler
Copy link
Member

AFAIUI, to move this PR forward, we need to either

  1. write a custom migration that can handle the removal of Windows builds,
  2. merge as-is and let @conda-forge/r continue to handle R 4.1 support for Windows until we have R >=4.2 builds available.

(I can't help with 1. since I'm not familiar with writing migrations; hence I did what I could and just re-enabled the other option...)

Thanks @mbargull!

I'm also fine with 2, but going forward would be less hesitant to skip Windows on individual recipes when they fail.

@mbargull
Copy link
Member

mbargull commented May 14, 2023

but going forward would be less hesitant to skip Windows on individual recipes when they fail.

Of course! Personally, I'm not at all knowledgeable about the R ecosystem and thus have no idea what minimum R version support is common in R packages. Especially if a DESCRIPTION has

Depends:
    R (>= 4.2.0)

or similar, it certainly does not make sense to waste much time on it ;) (unless someone has particular reasons to do so...).
(Plus the other reasons, e.g., C++17 support etc., you mentioned!)


My suggestions to @conda-forge/r :
You can "vote" on the given options by Approve-ing this PR in its current state (i.e., continue R 4.1 Windows support) or Request changes to voice to rather wait for a custom migration to drop Windows support.

@isuruf
Copy link
Member

isuruf commented May 14, 2023

We don't need a custom migrator. You can do the above change

@mbargull
Copy link
Member

@isuruf, just to get clarification/confirmation:
With the suggested

  conda_forge_yml_patches:
    build_platform.win_64: win_disabled

applied now and - 4.1 # [win] in cbc.yaml still in place, this would give us a migration that disables the Windows builds and we won't have any problem with rerendering non-migrated feedstocks since they can still use 4.1 on Windows, correct?

Meaning, if we want to go with the "disable Windows R package builds" route, this PR should be ready?

@isuruf
Copy link
Member

isuruf commented May 15, 2023

Yup, it should be ready. I didn't realize this configuration would work. 🤦

Copy link
Member

@mfansler mfansler left a comment

Choose a reason for hiding this comment

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

Marking my 'yea' vote for this as stands (with win dropped).

All other outstanding migrations on r-* feedstocks have completed.

Copy link
Member

@mbargull mbargull left a comment

Choose a reason for hiding this comment

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

LGTM
@conda-forge/r , @conda-forge/core : Any further comments? Feel free to merge otherwise.

@dpryan79
Copy link
Contributor Author

Anyone want to hit merge on this? It sounds like it's ready to go.

@dpryan79
Copy link
Contributor Author

dpryan79 commented Jun 9, 2023

What about now? Anyone want to hit merge?

@cbrueffer
Copy link
Member

I would if I could :-)

@xhochy xhochy merged commit 2219d5c into conda-forge:main Jun 10, 2023
@mfansler
Copy link
Member

This seems to have had no effect. 🤔

@jakirkham
Copy link
Member

Maybe it is a naming issue? Other migrators seem to use _ (instead of -). Sent PR ( #4572 ) to try that. Though it is possible the issue is something else

@mfansler
Copy link
Member

Comment on lines +13 to +14
conda_forge_yml_patches:
provider.win_64: win_disabled
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, could there be a syntax error here? This seems to be the main change relative to the old migrator

@xhochy
Copy link
Member

xhochy commented Jun 13, 2023

It could also be a general issue. I'm also not seeing any PRs for aws_c_io01326and the status page still shows migrations I have closed some days ago.

@mfansler
Copy link
Member

Well, something got it rolling (maybe #4572 did it?).

@isuruf FYI, if the intent of

conda_forge_yml_patches:
provider.win_64: win_disabled

was to disable Windows builds it does not seem to be having that effect. Current rollout appears to retain Windows R 4.1 builds, while using 4.2 and 4.3 for non-Windows.

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 this pull request may close these issues.

8 participants