ConstructionBase.jl icon indicating copy to clipboard operation
ConstructionBase.jl copied to clipboard

add LinearAlgebraExt

Open longemen3000 opened this issue 1 year ago • 8 comments
trafficstars

should make the package dependency free on julia > 1.9. motivated by JuliaFolds2/Trand

longemen3000 avatar Jul 16 '24 04:07 longemen3000

Hey thanks. In principle I agree this is a good move. However we were burnt by something similar before https://github.com/JuliaObjects/Accessors.jl/issues/127

@aplavin you think this PR would trigger the same issues?

jw3126 avatar Jul 16 '24 05:07 jw3126

seems that the problem is (somewhat) solved in 1.11, so maybe a good idea would be to guard extensions against that release instead? (see https://github.com/JuliaLang/julia/issues/52511)

longemen3000 avatar Jul 16 '24 06:07 longemen3000

seems that the problem is (somewhat) solved in 1.11, so maybe a good idea would be to guard extensions against that release instead? (see JuliaLang/julia#52511)

How would that work? Would you add julia = "1.11" to ConstructionBase/Project.toml?

jw3126 avatar Jul 16 '24 06:07 jw3126

seems that the problem is (somewhat) solved in 1.11, so maybe a good idea would be to guard extensions against that release instead? (see JuliaLang/julia#52511)

How would that work? Would you add julia = "1.11" to ConstructionBase/Project.toml?

i was thinking on just guarding the loading of extensions against if Base.VERSION < v"1.11" instead of using if !isdefined(Base,:get_extension)

longemen3000 avatar Jul 16 '24 06:07 longemen3000

Guarding against 1.11 seems fine to me, I guess you won't be using the LTS in projects that need to trim LinearAlgebra.jl anyway.

rafaqz avatar Jul 16 '24 07:07 rafaqz

Not sure what is the best way here. I'd suggest being totally sure a foundational package like this doesn't throw any warnings on installation/loading in at least 1.10 and newer. Test with running julia afresh as JULIA_DEPOT_PATH=$(mktemp -d) julia and installing it.

I understand it's potentially frustrating, but having a smooth user experience without confusing warnings for everyone is better than shaving a few stdlib dependencies. https://github.com/JuliaLang/julia/issues/52511 isn't fully fixed even on 1.11.

aplavin avatar Jul 16 '24 09:07 aplavin

What about just having the structure ready for when the problem is solved?, I suppose that this problem will be solved in a future 1.10 release and a 1.11 one, so it is just a matter of waiting for those releases, and loading the ext unconditionally until then?

longemen3000 avatar Jul 16 '24 11:07 longemen3000

Yeah, that's what we ended up doing in Accessors for now: https://github.com/JuliaObjects/Accessors.jl/blob/270e78fed32763096c448541148d520aa275e667/src/Accessors.jl#L19-L22 and https://github.com/JuliaObjects/Accessors.jl/blob/master/ext/AccessorsDatesExt.jl

aplavin avatar Jul 16 '24 11:07 aplavin

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.

Project coverage is 2.00%. Comparing base (8e3e773) to head (6947d65). Report is 10 commits behind head on master.

Files Patch % Lines
ext/ConstructionBaseLinearAlgebraExt.jl 0.00% 18 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

:exclamation: There is a different number of reports uploaded between BASE (8e3e773) and HEAD (6947d65). Click for more details.

HEAD has 20 uploads less than BASE
Flag BASE (8e3e773) HEAD (6947d65)
22 2
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #86       +/-   ##
==========================================
- Coverage   25.49%   2.00%   -23.50%     
==========================================
  Files           5       6        +1     
  Lines         153     150        -3     
==========================================
- Hits           39       3       -36     
- Misses        114     147       +33     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Aug 19 '24 06:08 codecov-commenter

i would say that this PR is ready to merge. as mentioned before, this PR no longer sets LinearAlgebra as an extension, but it paves the way to allow that change.

test errors are integration errors, that are also present on the master branch.

longemen3000 avatar Aug 19 '24 23:08 longemen3000