JuliaLinearAlgebra / AppleAccelerate.jl

Julia interface to the macOS Accelerate framework
Other
96 stars 18 forks source link

ccall using tuple rather than handle #52

Closed daviehh closed 1 year ago

daviehh commented 1 year ago

Reverts the part of the commits made in https://github.com/JuliaMath/AppleAccelerate.jl/commit/e843a3aca63b0b76e03e0fcd66322f8242e171ad where the first argument to ccall is changed to be a function pointer. Now it reverts back to the tuple form (function_name, library)

Should fix #51


Due to changes in how macos ships system dylibs, the original check using isfilewould result in errors, and e843a3a was first made to change the existence check of the accelerate library using dlopen. In that commit, since I though it would be good to pass handles to the ccall since we already have the handles from the dlopen check, the files src/Array.jl and src/DSP.jl is also changed. Now I see this might not be necessary and can actually be fragile, so the changes to ccall is reverted in this PR but we keep the change to check whether the dylib exists.

codecov[bot] commented 1 year ago

Codecov Report

Base: 81.28% // Head: 0.00% // Decreases project coverage by -81.29% :warning:

Coverage data is based on head (3140bb0) compared to base (d46c7fa). Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #52 +/- ## ========================================== - Coverage 81.28% 0.00% -81.29% ========================================== Files 4 3 -1 Lines 171 151 -20 ========================================== - Hits 139 0 -139 - Misses 32 151 +119 ``` | [Impacted Files](https://codecov.io/gh/JuliaMath/AppleAccelerate.jl/pull/52?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath) | Coverage Δ | | |---|---|---| | [src/Array.jl](https://codecov.io/gh/JuliaMath/AppleAccelerate.jl/pull/52?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath#diff-c3JjL0FycmF5Lmps) | `0.00% <0.00%> (-93.66%)` | :arrow_down: | | [src/DSP.jl](https://codecov.io/gh/JuliaMath/AppleAccelerate.jl/pull/52?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath#diff-c3JjL0RTUC5qbA==) | `0.00% <0.00%> (-77.46%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

daviehh commented 1 year ago

Not sure why codecov is unable to pick up the changes, looks like the test runs ok https://github.com/JuliaMath/AppleAccelerate.jl/actions/runs/4048059274/jobs/6962842738#step:6:190

but the upload to codecov failed https://github.com/JuliaMath/AppleAccelerate.jl/actions/runs/4048059274/jobs/6962842738#step:8:46

Locally coverage seems fine

using Pkg
using Coverage
Pkg.test(; coverage = true)

coverage = process_folder()

clean_folder("src")
clean_folder("test")

covered_lines, total_lines = get_summary(coverage)

gives

(121, 151)

ViralBShah commented 1 year ago

I believe I saw some discussion a few days ago around codecov fixes. Trying again.

ViralBShah commented 1 year ago

@daviehh I will also give you commit access soon.