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 test: for openssl problem #21

Merged
merged 12 commits into from
May 15, 2023

Conversation

bernt-matthias
Copy link
Contributor

@bernt-matthias bernt-matthias commented May 3, 2023

xref galaxyproject/tools-iuc#4901

fix might be analogous to https://github.com/conda-forge/curl-feedstock/blob/641bda20ddba19e4ff3b2cf9e179e10842c52930/recipe/meta.yaml#L104

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.

@bernt-matthias bernt-matthias requested a review from a team as a code owner May 3, 2023 15:25
@conda-forge-webservices
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.

@bernt-matthias
Copy link
Contributor Author

Test is failing for linux_64_r_base4.1, linux_aarch64_r_base4.1, linux_ppc64le_r_base4.1, osx_64_r_base4.1 ..

I see curl 7.86.0 installed in failing tests and libcurl and curl (8.0.1) in successful tests. Not sure if its missing libcurl or a problem of 7.x (or something else).

@mfansler
Copy link
Member

mfansler commented May 3, 2023

I'm not sure the openssl is justified (it doesn't link against it), however, the recipe is definitely missing a libcurl (for # [not win]) dependency and that might be the issue.

Also, rather than an ad hoc test against a KEGG link, it would be better to run the testthat tests in the package.

@bernt-matthias
Copy link
Contributor Author

Thanks for the fast feedback.

it would be better to run the testthat tests in the package

Do you have an example where this is done?

@mfansler
Copy link
Member

mfansler commented May 3, 2023

This recent recipe has the testthat tests: https://github.com/conda-forge/r-rgenoud-feedstock

Note that the build scripts need --install-tests added to the R CMD INSTALL.

@bernt-matthias
Copy link
Contributor Author

Seems to be a curl problem. When I execute curl http://rest.kegg.jp/conv/ncbi-geneid/dme -L locally (I have 7.81) I get the same error on my local ubuntu.

Here it is suggested that this due to openssl 3 ..

It feels like this should be fixed at the curl feedstock, but I'm not sure if this is possible because they are already at version 8.x (which we can't use here because it conflicts with R 4.1) ... SNAFU :)

@mfansler
Copy link
Member

mfansler commented May 3, 2023

My suspicion is that it all traces back to the fact that R 4.1 has not updated to the latest krb, which libcurl links against. This is what is triggering the solver to use an older version of libcurl and these maybe don't have correct upper bounds on the openssl. (everything should be solving to libcurl=8 as per the Conda Forge global pinnings)

It will take a bit to determine the proper solution here. Probably need to patch some metadata on libcurl to determine what the correct openssl constraints on the libcurl libraries should be. Also, we need to patch the metadata on this package to properly declare what versions of libcurl each build was linked against.

Forcing openssl=1 is not a proper solution for Conda Forge (though we could consider adding it as variant).

Unfortunately, I'm short on time, but could maybe get look into this over the weekend.

@mfansler
Copy link
Member

mfansler commented May 3, 2023

BTW, anytime you change a library host requirement, you need to rerender:

@conda-forge-admin please rerender

@bernt-matthias
Copy link
Contributor Author

Thanks for the detailed explanations. Let me know if I can help.

I will add the testthat testing and revert the pinning.. Let's see if this fails as well. Maybe that's a small step forward.

@bernt-matthias
Copy link
Contributor Author

The testthat tests seem not to cover the openssl problems, but they show a problem with windows (meh).

non-win link against libcurl
@mfansler
Copy link
Member

Maybe we just skip the testthat tests on Windows. As I suspected, everything else passes with the merge of conda-forge/r-base-feedstock/pull/236. We'll try it one more time, and if the Windows fails again we'll skip that test - seems to be only one failure on something minor anyway.

@mfansler
Copy link
Member

@conda-forge-admin please rerender

@mfansler mfansler mentioned this pull request May 15, 2023
2 tasks
@mfansler
Copy link
Member

I opted to retain the testing in Windows, but let it pass even if it fails. That way, we can still observe where it fails and consider it "good enough".

If you want to switch this out of draft mode, I'll give it a final review then merge. I think this is good progress on improving the recipe, but people may still encounter issues until #22 is addressed (i.e., patching metadata on old builds).

@bernt-matthias bernt-matthias marked this pull request as ready for review May 15, 2023 07:30
@bernt-matthias
Copy link
Contributor Author

Thanks for your time @mfansler. PR is now ready for review.

@mfansler mfansler merged commit 012ca5c into conda-forge:main May 15, 2023
@bernt-matthias bernt-matthias deleted the topic/pin-openssl branch May 15, 2023 17:45
@mfansler
Copy link
Member

@bernt-matthias thanks again for submitting this! Just as an afterthought, you may want to suggest directly adding a testthat test upstream in the curl package, since this uncovered a gap in the testing regime.

bernt-matthias added a commit to bernt-matthias/curl that referenced this pull request May 17, 2023
@bernt-matthias
Copy link
Contributor Author

good idea jeroen/curl#298

@bernt-matthias bernt-matthias mentioned this pull request Dec 8, 2023
3 tasks
@mbargull mbargull mentioned this pull request Jul 7, 2024
3 tasks
@mfansler mfansler mentioned this pull request Nov 26, 2024
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.

2 participants