Wrap everything in a version check #21

ararslan · 2017-07-06T19:08:47Z

The diff looks a little weird, but this just wraps the contents of src/FFTW.jl with

if VERSION < v"0.7.0-DEV.602"
    import Base.FFTW
else
    # do everything that was here before
end

stevengj · 2017-07-06T19:16:08Z

This still defines a module FFTW; import Base.FFTW; end in 0.6, which seems weird/wrong to me.

My thinking was that the entire FFTW.jl file should just consist of import Base.FFTW (one line ... no module or precompile directive) in 0.6 (via a versioned REQUIRE file ... I don't think it's worth worrying about older versions of 0.7-dev).

giordano · 2017-07-06T19:19:31Z

Yes, I was thinking about branching the package and keeping an (almost) empty package just for Julia 0.6.

stevengj · 2017-07-06T19:19:32Z

I guess you could also do:

if VERSION < ....
    import Base.FFTW
else
    __precompile__(true)
    module FFTW
        ....
    end
end

if you don't want to mess with multiple branches/versions of REQUIRE.

stevengj · 2017-07-06T19:21:27Z

Or perhaps

if VERSION < ....
    import Base.FFTW
else
    include("src.jl") # defines module FFTW
end

so that 0.6 doesn't pay the price of parsing the whole module FFTW ... end (nor does it need precompilation/caches).

(This way the diff will also be small, since you will just do git mv FFTW.jl src.jl on the old FFTW.jl file. And much easier than branching the package.)

ararslan · 2017-07-06T23:55:50Z

The fact that wrapping the AbstractFFTs imports was necessary is a little concerning. Perhaps we need to do something similar there but with Base.DFT?

stevengj · 2017-07-07T02:17:04Z

test/runtests.jl

 importall FFTW
-import AbstractFFTs: Plan, fft, ifft, bfft, fft!, ifft!, bfft!,
+if VERSION >= v"0.7.0-DEV.602"
+    import AbstractFFTs: Plan, fft, ifft, bfft, fft!, ifft!, bfft!,


shouldn't using AbstractFFTs be enough here?

(except for Plan)

No, to avoid name conflicts, AbstractFFTs only exports things if none of the functions are defined in Base. So using AbstractFFTs won't import anything into Main until after the deprecation period.

That's really annoying. I feel like we should make it possible to override a deprecated binding without a warning (precisely for things like this … it's going to be a continual annoyance to move things out of Base otherwise)

cc @JeffBezanson

It's the same thing we did with Primes and continue to do with SpecialFunctions, QuadGK, the priority queue stuff in DataStructures, etc. It's certainly not ideal. Being able to do that would make it easier to fully support multiple versions at once.

And actually, it seems that @deprecate doesn't call deprecate either.... only @deprecate_binding uses it. (This seems correct: @deprecate may not deprecate a whole function: it may only deprecate certain methods.)

Is it actually legit to deprecate in Main though? What happens to packages then?

Oh, sorry, the code above relied on patching Julia as well to check for the deprecation before emitting a warning message:

--- a/src/module.c +++ b/src/module.c @@ -190,7 +190,7 @@ static jl_binding_t *jl_get_binding_(jl_module_t *m, jl_sym_t *var, modstack_t * if (tempb == NULL || tempb->owner == NULL) // couldn't resolve; try next using (see issue #6105) continue; - if (owner != NULL && tempb->owner != b->owner && + if (owner != NULL && tempb->owner != b->owner && !tempb->deprecated && !(tempb->constp && tempb->value && b->constp && b->value == tempb->value)) { jl_printf(JL_STDERR, "WARNING: both %s and %s export \"%s\"; uses of it in module %s must be qualified\n",

Hopefully I'll have a PR for Julia shortly.

That would be awesome!

stevengj · 2017-07-07T02:17:39Z

test/runtests.jl

+                         fftshift, ifftshift, plan_inv
+    import FFTW: fftw_vendor
+else
+    import Base.DFT: Plan, fft, ifft, bfft, fft!, ifft!, bfft!,
                     plan_fft, plan_ifft, plan_bfft, plan_fft!, plan_ifft!, plan_bfft!,


I'm confused, why do you need to import plan_fft in 0.6, since that is already exported? The only thing here that is not exported in 0.6 is Plan.

I just copied and pasted, but originally I had tried with just Plan but there was something else it was getting mad about (can't recall what it was offhand, plan_ something or other)

stevengj · 2017-07-07T02:21:27Z

src/package.jl

+end
+
+# MKL provides its own FFTW
+fftw_vendor() = Base.BLAS.vendor() === :mkl ? :mkl : :fftw


Shouldn't this function just be called vendor(), with a qualified name of FFTW.vendor() analogous to BLAS.vendor()?

Yes. The name is a carryover from Base.

with a @deprecate fftw_vendor definition, of course.

timholy · 2017-07-13T19:08:33Z

I tried Pkg.checkout("FFTW", "aa/ugh-oh-my-god") (love the branch name!), but that doesn't fix FFTViews, sadly.

timholy · 2017-07-13T19:14:02Z

Can fix it by changing FFTW.fft to Main.FFTW.fft.

ararslan · 2017-07-13T19:16:24Z

I think that gets back to the fact that neither this package nor AbstractFFTs exports anything when the names are defined in Base...

ararslan · 2017-07-20T17:32:12Z

Since this gets things working on 0.6 I think we should just go ahead and merge this and I'll continue to try to get 0.7 working in separate PRs.

ararslan · 2017-07-20T17:52:26Z

(Actually let's wait until Compat is fixed) (side note, I hate that I had to add Compat)

ararslan · 2017-07-20T19:11:14Z

Actually the fix was wrong in Compat so this should be fine.

ararslan requested a review from stevengj July 6, 2017 19:08

ararslan force-pushed the aa/ugh-oh-my-god branch 7 times, most recently from daba494 to 8ebc2a3 Compare July 6, 2017 23:23

stevengj reviewed Jul 7, 2017

View reviewed changes

This was referenced Jul 11, 2017

Rename the package? #17

Closed

new deprecate_moved macro, better support for replacing deprecated bindings JuliaLang/julia#22763

Merged

ararslan added 2 commits July 17, 2017 13:32

Restructure the package to only define FFTW on 0.7

26d9c76

Adjust version bounds for Base change to deprecation handling

910255b

ararslan force-pushed the aa/ugh-oh-my-god branch 2 times, most recently from 03c299e to bab0bd6 Compare July 17, 2017 23:02

Don't build FFTW if we don't have to

4929524

ararslan force-pushed the aa/ugh-oh-my-god branch from bab0bd6 to 4929524 Compare July 17, 2017 23:14

Try not to build FFTW on Travis

ca7f37b

ararslan force-pushed the aa/ugh-oh-my-god branch from 1389901 to f1fbc3c Compare July 20, 2017 03:56

Add Compat and bump AbstractFFTs dependency

feb7c5f

ararslan force-pushed the aa/ugh-oh-my-god branch from f1fbc3c to feb7c5f Compare July 20, 2017 04:01

ararslan merged commit 070cd89 into master Jul 20, 2017

ararslan deleted the aa/ugh-oh-my-god branch July 20, 2017 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap everything in a version check #21

Wrap everything in a version check #21

ararslan commented Jul 6, 2017

stevengj commented Jul 6, 2017 •

edited

Loading

giordano commented Jul 6, 2017

stevengj commented Jul 6, 2017 •

edited

Loading

stevengj commented Jul 6, 2017 •

edited

Loading

ararslan commented Jul 6, 2017

stevengj Jul 7, 2017

stevengj Jul 7, 2017

ararslan Jul 7, 2017

stevengj Jul 7, 2017 •

edited

Loading

ararslan Jul 7, 2017

stevengj Jul 11, 2017 •

edited

Loading

ararslan Jul 11, 2017

stevengj Jul 11, 2017

stevengj Jul 11, 2017

ararslan Jul 11, 2017

stevengj Jul 7, 2017

ararslan Jul 7, 2017

stevengj Jul 7, 2017

ararslan Jul 7, 2017

stevengj Jul 7, 2017

ararslan Jul 7, 2017

timholy commented Jul 13, 2017

timholy commented Jul 13, 2017

ararslan commented Jul 13, 2017

ararslan commented Jul 20, 2017

ararslan commented Jul 20, 2017 •

edited

Loading

ararslan commented Jul 20, 2017

Wrap everything in a version check #21

Wrap everything in a version check #21

Conversation

ararslan commented Jul 6, 2017

stevengj commented Jul 6, 2017 • edited Loading

giordano commented Jul 6, 2017

stevengj commented Jul 6, 2017 • edited Loading

stevengj commented Jul 6, 2017 • edited Loading

ararslan commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj Jul 11, 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

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

Choose a reason for hiding this comment

timholy commented Jul 13, 2017

timholy commented Jul 13, 2017

ararslan commented Jul 13, 2017

ararslan commented Jul 20, 2017

ararslan commented Jul 20, 2017 • edited Loading

ararslan commented Jul 20, 2017

stevengj commented Jul 6, 2017 •

edited

Loading

stevengj commented Jul 6, 2017 •

edited

Loading

stevengj commented Jul 6, 2017 •

edited

Loading

stevengj Jul 7, 2017 •

edited

Loading

stevengj Jul 11, 2017 •

edited

Loading

ararslan commented Jul 20, 2017 •

edited

Loading