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

Mbedtls #126

Merged
merged 2 commits into from
Sep 23, 2021
Merged

Mbedtls #126

merged 2 commits into from
Sep 23, 2021

Conversation

izahn
Copy link
Contributor

@izahn izahn commented Sep 10, 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 #125

Note that this needs conda-forge/staged-recipes#16113 , once someone reviews and merges that we can proceed to review this PR.

@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.

@mkitti
Copy link
Contributor

mkitti commented Sep 13, 2021

mbedtls does not exist in conda-forge.

@@ -35,13 +38,14 @@ requirements:
- libosxunwind # [osx]
- libunwind # [linux]
- arpack
- suitesparse =5.4.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove the pin?

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 used https://github.com/archlinux/svntogit-community/blob/packages/julia/trunk/julia-hardcoded-libs.patch to patch out the hard-coded mbedtls version. Since that patch also removes suitesparse version hard-coding we don't need to pin anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be interested to see how this interacts with the issue in #118, which is why the pin was added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we should test to make sure that issue is fixed by this PR before merging.

recipe/meta.yaml Outdated
- pcre2
- git
- libutf8proc
- libnghttp2
- zlib
- p7zip
- mbedtls
Copy link
Contributor

Choose a reason for hiding this comment

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

Which package are you referring to? mbedtls does not exist in conda-forge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mbedtls does not exist in conda-forge.

As noted in my PR it will once conda-forge/staged-recipes#16113 is merged.

@izahn
Copy link
Contributor Author

izahn commented Sep 20, 2021

@conda-forge-admin please restart ci

@mkitti
Copy link
Contributor

mkitti commented Sep 20, 2021

For Mac, we need to resolve the problem described in this comment when using system LibGit2:
#115 (comment)

@izahn
Copy link
Contributor Author

izahn commented Sep 20, 2021

For Mac, we need to resolve the problem described in this comment when using system LibGit2:
#115 (comment)

That thread is pretty long, can you summarize what the problem is?

@mkitti
Copy link
Contributor

mkitti commented Sep 20, 2021

That thread is pretty long, can you summarize what the problem is?

I linked to a specific comment in the thread. Here's the gist.

Changing from USE_SYSTEM_LIBGIT2=0 to USE_SYSTEM_LIBGIT2=1 we introduce an error on Mac OS X.

Error During Test at $PREFIX/share/julia/stdlib/v1.6/Downloads/test/runtests.jl:12
  Got exception outside of a @test
  error setting certificate verify locations:  CAfile: /Users/runner/miniforge3/conda-bld/julia_1628019948829/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeho while requesting https://httpbingo.julialang.org/base64/SnVsaWEgaXMgZ3JlYXQh
  Stacktrace:
    [1] (::Downloads.var"#9#18"{IOStream, Base.DevNull, Nothing, Vector{Pair{String, String}}, Float64, Nothing, Bool, Bool, String, Int64, Bool, Bool})(easy::Downloads.Curl.Easy)
      @ Downloads ~/miniforge3/conda-bld/julia_1628019948829/work/usr/share/julia/stdlib/v1.6/Downloads/src/Downloads.jl:356

@mkitti
Copy link
Contributor

mkitti commented Sep 20, 2021

Essentially we need USE_SYSTEM_LIBGIT2=1 for all platforms. It currently does not work on OS X because of "error setting certificate verify locations". I'm not sure what that error is about.

@izahn
Copy link
Contributor Author

izahn commented Sep 21, 2021

Essentially we need USE_SYSTEM_LIBGIT2=1 for all platforms. It currently does not work on OS X because of "error setting certificate verify locations". I'm not sure what that error is about.

I haven't had any luck solving this one. Does using libgit2 from BinaryBuilder on osx cause any known problems?

@izahn
Copy link
Contributor Author

izahn commented Sep 21, 2021

I cleaned this up and force-pushed, should be in good shape now. I also added explicit tests to make sure that this PR successfully addresses both #125 and #118

@izahn izahn reopened this Sep 21, 2021
@izahn
Copy link
Contributor Author

izahn commented Sep 21, 2021

This PR is ready for review. It fixes #125 and #118 by applying a patch adapted from the Arch Linux Julia package and using the system mbedtls on linux.

@izahn
Copy link
Contributor Author

izahn commented Sep 22, 2021

@conda-forge-admin please rerender

1 similar comment
@izahn
Copy link
Contributor Author

izahn commented Sep 22, 2021

@conda-forge-admin please rerender

@mkitti
Copy link
Contributor

mkitti commented Sep 22, 2021

I also added explicit tests to make sure that this PR successfully addresses both #125 and #118

Where are these explicit tests?

@izahn
Copy link
Contributor Author

izahn commented Sep 22, 2021

I also added explicit tests to make sure that this PR successfully addresses both #125 and #118

Where are these explicit tests?

Thanks for catching that! Somehow they didn't make it in. I pushed them up in 77a8d02

@mkitti
Copy link
Contributor

mkitti commented Sep 22, 2021

Things are looking great. I'm just trying to reduce the changes to the minimum. Currently, I think there are some changes that were reverted from #123. I have unreverted them here:
izahn#1

@izahn
Copy link
Contributor Author

izahn commented Sep 22, 2021

The changes in izahn#1 are all to files generated by conda smithy rerender. What is issue you are trying to address there?

@mkitti
Copy link
Contributor

mkitti commented Sep 22, 2021

I'm trying to minimize the actual diff against master.

@mkitti
Copy link
Contributor

mkitti commented Sep 22, 2021

I'm ready to merge this after it either does not modify .ci_support versus master or we can justify why this pull request should modify .ci_support versus master.

@izahn
Copy link
Contributor Author

izahn commented Sep 22, 2021

I'll back it up and let the bot do the rerender so we can see what happens.

@izahn
Copy link
Contributor Author

izahn commented Sep 22, 2021

5b15552 just changes the recipe, none of the CI or migration stuff. Now we rerender:

@conda-forge-admin please rerender

@izahn
Copy link
Contributor Author

izahn commented Sep 22, 2021

The bot ran conda smithy rerender for us in 3820715 . All perfectly normal and expected. Rerendering is standard part of the conda packaging workflow, changes produced by it do not need to be reverted.

@mkitti mkitti added the automerge Merge the PR when CI passes label Sep 22, 2021
@github-actions github-actions bot merged commit 75ef39e into conda-forge:master Sep 23, 2021
@github-actions
Copy link
Contributor

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

@ngam
Copy link
Contributor

ngam commented Dec 24, 2021

Essentially we need USE_SYSTEM_LIBGIT2=1 for all platforms. It currently does not work on OS X because of "error setting certificate verify locations". I'm not sure what that error is about.

I haven't had any luck solving this one. Does using libgit2 from BinaryBuilder on osx cause any known problems?

@izahn @mkitti we can try to see if the new libgit2 patch can resolve this for macs. I will start a new PR. Follow #155

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot add LibSSH2_jll
4 participants