Unify the method name to get library versions #23323

musm · 2017-08-18T17:38:15Z

addresses #23314

Sacha0 · 2017-08-18T18:44:57Z

NEWS.md

@@ -312,6 +312,10 @@ Deprecated or removed
  * `Base.cpad` has been removed; use an appropriate combination of `rpad` and `lpad`
    instead ([#23187]).

+  * `GMP.gmp_version()` and `GMP.GMP_VERSION` have been renamed to `GMP.version()` and
+    `GMP.VERSION`, respectively. Similarly, `MPFR.get_version()`,


Perhaps nix the comma following MPFR.get_version()?

Sacha0 · 2017-08-18T18:46:42Z

base/libgit2/utils.jl

@@ -26,7 +26,7 @@ function version()
    minor = Ref{Cint}(0)
    patch = Ref{Cint}(0)
    ccall((:git_libgit2_version, :libgit2), Void,
-          (Ptr{Cint}, Ptr{Cint}, Ptr{Cint}), major, minor, patch)
+          (Ref{Cint}, Ref{Cint}, Ref{Cint}), major, minor, patch)


Unrelated? Perhaps better in a separate pull request as such?

Perhaps, but since this is related to version might as well just do it here. THis is in any case a very trivial change.

Sacha0

lgtm modulo the minor comments! :)

musm · 2017-08-18T19:29:41Z

@Sacha0 thanks for the review!

tkelman · 2017-08-19T04:28:47Z

base/gmp.jl

 gmp_bits_per_limb() = Int(unsafe_load(cglobal((:__gmp_bits_per_limb, :libgmp), Cint)))

-const GMP_VERSION = gmp_version()
+const VERSION = version()
 const GMP_BITS_PER_LIMB = gmp_bits_per_limb()


seems like the GMP_ and gmp_ prefixes are also a bit redundant with the module name here

Should those be done in the same PR? Will they also need a deprecation?

Should those be done in the same PR?

Sure, why not? :)

Will they also need a deprecation?

Sure, why not? :)

fredrikekre · 2017-08-22T09:41:05Z

base/deprecated.jl

@@ -1701,6 +1701,12 @@ export hex2num
 # issue #17886
 # deprecations for filter[!] with 2-arg functions are in associative.jl

+@eval GMP @deprecate gmp_version() version()


I think all of these should have a false at the end, since none of them are currently exported?

True (although they don't seem to be exported, but better be safe )

musm · 2017-08-22T15:54:43Z

unrelated libgit2 error on macos

musm · 2017-08-22T17:48:41Z

Nice more conflicts to rebase ... :P

fredrikekre · 2017-08-22T17:53:03Z

base/deprecated.jl

@@ -1696,14 +1696,6 @@ export hex2num
 @deprecate ctranspose adjoint
 @deprecate ctranspose! adjoint!

-@deprecate convert(::Type{Vector{UInt8}}, s::AbstractString)  Vector{UInt8}(s)


Rebase mistake?

uggh, very strange. The online tool is kinda terrible

StefanKarpinski

LGTM. It's probably a good idea to test the deprecations if you haven't already – they're easy to get wrong.

musm · 2017-08-23T14:46:31Z

Updated with LAPACK as well.

I tested the deprecations and they work, but you only get the error once, which is weird. See below (this seems unrelated)
e.g.

   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: https://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.7.0-DEV.1458 (2017-08-23 14:26 UTC)
 _/ |\__'_|_|_|\__'_|  |  ver/9e3931c (fork: 15 commits, 0 days)
|__/                   |  x86_64-linux-gnu

julia> Base.MPFR.get_version()
WARNING: get_version() is deprecated, use version() instead.
Stacktrace:
 [1] depwarn at ./deprecated.jl:68 [inlined]
 [2] get_version() at ./deprecated.jl:56
 [3] eval(::Module, ::Expr) at ./repl/REPL.jl:3
 [4] eval_user_input(::Any, ::Base.REPL.REPLBackend) at ./repl/REPL.jl:69
 [5] macro expansion at ./repl/REPL.jl:100 [inlined]
 [6] (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:96
while loading no file, in expression starting on line 0
v"3.1.5+r11590"

julia> Base.GMP.gmp_version()
v"6.1.2"

julia> ^C

julia>
mus@mus-laptop:~/julia$ ./julia
               _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: https://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.7.0-DEV.1458 (2017-08-23 14:26 UTC)
 _/ |\__'_|_|_|\__'_|  |  ver/9e3931c (fork: 15 commits, 0 days)
|__/                   |  x86_64-linux-gnu

julia> Base.GMP.gmp_version()
WARNING: gmp_version() is deprecated, use version() instead.
Stacktrace:
 [1] depwarn at ./deprecated.jl:68 [inlined]
 [2] gmp_version() at ./deprecated.jl:56
 [3] eval(::Module, ::Expr) at ./repl/REPL.jl:3
 [4] eval_user_input(::Any, ::Base.REPL.REPLBackend) at ./repl/REPL.jl:69
 [5] macro expansion at ./repl/REPL.jl:100 [inlined]
 [6] (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:96
while loading no file, in expression starting on line 0
v"6.1.2"

julia> Base.MPFR.get_version()
v"3.1.5+r11590"

musm · 2017-08-24T16:52:56Z

Can we merge this now? Osx had a spawn failure before the rebasing. Thanks.

musm · 2017-08-24T23:09:42Z

Thanks!

StefanKarpinski · 2017-08-24T23:11:11Z

No problem. Thanks for doing it!

tkelman · 2017-08-25T09:49:45Z

base/deprecated.jl

+@eval GMP @deprecate gmp_bits_per_limb() bits_per_limb() false
+@eval GMP @Base.deprecate_binding GMP_BITS_PER_LIMB BITS_PER_LIMB false
+@eval MPFR @deprecate get_version() version() false
+@eval LinAlg.LAPACK @deprecate laver() version() false


to give the same behavior as the previous function this deprecation should return a tuple

true (anyone mind fixing this? I don't have time to do it)

I think the best way to fix this is to leave laver as it was before this change and just add version since, generally, our LAPACK wrappers are pretty thin and don't add much Julia abstraction on top of LAPACK.

musm force-pushed the ver branch 2 times, most recently from 6046cae to 1387ea9 Compare August 18, 2017 18:20

Sacha0 reviewed Aug 18, 2017

View reviewed changes

Sacha0 approved these changes Aug 18, 2017

View reviewed changes

tkelman reviewed Aug 19, 2017

View reviewed changes

musm force-pushed the ver branch 3 times, most recently from fba7f29 to 0853a07 Compare August 21, 2017 21:53

fredrikekre reviewed Aug 22, 2017

View reviewed changes

musm force-pushed the ver branch from ac46eb5 to a65b88c Compare August 22, 2017 12:52

fredrikekre reviewed Aug 22, 2017

View reviewed changes

Unify method names to call library version's

b0838bc

musm force-pushed the ver branch from f358223 to b0838bc Compare August 22, 2017 18:01

StefanKarpinski approved these changes Aug 22, 2017

View reviewed changes

Merge branch 'master' into ver

ae761e5

Also rename LAPACK version

6b44b78

musm force-pushed the ver branch from 9e3931c to 6b44b78 Compare August 23, 2017 16:45

Merge branch 'master' into ver

59b5506

rfourquet approved these changes Aug 24, 2017

View reviewed changes

StefanKarpinski merged commit 2406b6e into JuliaLang:master Aug 24, 2017

musm deleted the ver branch August 24, 2017 23:09

tkelman reviewed Sep 8, 2017

View reviewed changes

cormullion mentioned this pull request Mar 18, 2018

NEWS.md is getting a bit untidy #26508

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify the method name to get library versions #23323

Unify the method name to get library versions #23323

musm commented Aug 18, 2017

Sacha0 Aug 18, 2017

Sacha0 Aug 18, 2017

musm Aug 18, 2017

Sacha0 left a comment

musm commented Aug 18, 2017

tkelman Aug 19, 2017 •

edited

Loading

musm Aug 19, 2017

fredrikekre Aug 19, 2017

musm Aug 22, 2017

fredrikekre Aug 22, 2017

musm Aug 22, 2017 •

edited

Loading

musm commented Aug 22, 2017

musm commented Aug 22, 2017

fredrikekre Aug 22, 2017

musm Aug 22, 2017

StefanKarpinski left a comment

musm commented Aug 23, 2017 •

edited

Loading

musm commented Aug 24, 2017

musm commented Aug 24, 2017

StefanKarpinski commented Aug 24, 2017

tkelman Aug 25, 2017

musm Sep 13, 2017

andreasnoack Sep 13, 2017

Unify the method name to get library versions #23323

Unify the method name to get library versions #23323

Conversation

musm commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

musm commented Aug 18, 2017

tkelman Aug 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musm Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

musm commented Aug 22, 2017

musm commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski left a comment

Choose a reason for hiding this comment

musm commented Aug 23, 2017 • edited Loading

musm commented Aug 24, 2017

musm commented Aug 24, 2017

StefanKarpinski commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman Aug 19, 2017 •

edited

Loading

musm Aug 22, 2017 •

edited

Loading

musm commented Aug 23, 2017 •

edited

Loading