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

Patch cleanup #162

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Patch cleanup #162

wants to merge 3 commits into from

Conversation

izahn
Copy link

@izahn izahn commented Mar 9, 2021

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.

Closes #161

This is partly a conversation starter, but mostly serious. Do we really need all those patches? The other R packaging efforts I've looked at (e.b., https://formulae.brew.sh/formula/r and https://github.com/archlinux/svntogit-packages/blob/packages/r/trunk/PKGBUILD) appear not to do any patching at all.

@conda-forge-linter
Copy link

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.

@izahn
Copy link
Author

izahn commented Mar 9, 2021

As far as I can tell this looks pretty good. I don't understand the osx_arm64 failure at all -- what is supposed to happen there?

@xhochy
Copy link
Member

xhochy commented Mar 12, 2021

I'm -1 on deleting all of them. Most of the things seem to work without them but some of the patches that adjust link flags probably don't have an impact on the tests executed in the test: section of the recipe but e.g. handle whether the libgcc symbols should be statically linked or not.

@izahn
Copy link
Author

izahn commented Mar 13, 2021

Right, I don't expect that we will end up dropping all patches. But each one imposes a maintenance burden, and it will simplify things if we can drop all but those we really need. My assessment is that most of the patches we've been maintaining here are trivial and can be dropped without consequence; most of the rest are better suited for contributing to upstream. My hope is that this PR will serve as a starting point for re-evaluating what we really want to maintain.

@mingwandroid
Copy link
Contributor

I am in favour of removing any of my patches that can be proven unneeded by adding an (ideally) run-time test to the package to make sure the removal of the patch doesn't regress the issue it is meant to resolve.

@mingwandroid
Copy link
Contributor

Some of the early patches I have I did intend to try to upstream a long time ago. @opencpu, what is your take on moving the Windows R build to work properly via autotools? Would upstream go for that? Would you like it?

@izahn
Copy link
Author

izahn commented Mar 15, 2021

Thanks all for engaging with this, I'll try to find time to do a more thorough assessment of each patch and get a better handle on what needs to be maintained. This can be a longer-term project, no need to hold up the 4.0.4 update for it.

@izahn izahn changed the title Patch cleanup 4.0.4 Patch cleanup May 12, 2021
@mbargull mbargull requested review from mbargull and a team as code owners June 29, 2024 18:34
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.

5 participants