Skip to content
This repository has been archived by the owner on Mar 1, 2024. It is now read-only.

Add deprecation notice on using #112

Merged
merged 4 commits into from
Feb 26, 2024
Merged

Conversation

Seelengrab
Copy link
Contributor

@Seelengrab Seelengrab commented Feb 24, 2024

This adds a deprecation notice that will be printed on using Formatting, to inform users that the package has been deprecated and that they should consider using a different package.

This is intended to be released in conjunction with #111. This will also need a release to General, since it bumps the version.

@kescobo
Copy link
Member

kescobo commented Feb 24, 2024

If there are people using this package, and don't need additional support / features, does this have any ongoing runtime cost?

Also, is it going to show up if this is an indirect dependency of something that I use?

This adds a deprecation notice that will be printed on `using Formatting`,
to inform users that the package has been deprecated and that they should
consider using a different package.
@Seelengrab
Copy link
Contributor Author

Seelengrab commented Feb 24, 2024

If there are people using this package, and don't need additional support / features, does this have any ongoing runtime cost?

No, this is only printed during using through __init__. All packages added there are part of the sysimage or are stdlibs already, highly likely to be loaded already. I guess it could be moved to precompilation too?

I'm mostly concerned with people using this package assuming that it works, when it evidently has fatal flaws, such as #108, which prints completely wrong results. Whoever is using this package and trusting its output might be in for a rude awakening.

Also, is it going to show up if this is an indirect dependency of something that I use?

Possibly? If so, we can add a marker that this is specifically for Formatting.jl, since the current message doesn't specify. Good catch, thank you! I'll try it out.

@Seelengrab
Copy link
Contributor Author

Seelengrab commented Feb 24, 2024

Ok, with packages of a dependency chain like Baz -> Foobar -> Formatting.jl I get this output on 1.0.5:

image

and this on 1.11+:

image

so unfortunately, __init__ won't work for these purposes, since I can't seem to get it to print something on earlier versions. The logging message does make it clear that it's originating from Formatting, so I guess I can add a note directing the user to check ]why Formatting to find out which dependency they have to report this upstream issue to:

image

The downside is that this has only been supported since 1.9...

src/Formatting.jl Outdated Show resolved Hide resolved
src/Formatting.jl Show resolved Hide resolved
@kescobo
Copy link
Member

kescobo commented Feb 24, 2024

The downside is that this has only been supported since 1.9...

I'm fine with that for something like this that's usability - focused.

@kescobo
Copy link
Member

kescobo commented Feb 24, 2024

I get this output on 1.0.5:

If people are using exceptionally old Julia releases, frankly they should expect that lots of what they use is deprecated/ unmaintained.

Ideally, it would work at least with LTS, but even if it's only on the current release, I'd prefer it being in __init__ so we're not spamming people where it's an indirect dep every time they load a package

@MasonProtter
Copy link
Contributor

MasonProtter commented Feb 24, 2024

JuliaHub says this has 81 direct dependants and 666 indirect dependants, so yeah we need to be kinda careful about not overzealously spamming people.

I would suggest putting it in an if ccall(:jl_generating_output, Cint, ()) == 1 block or something

@Seelengrab
Copy link
Contributor Author

If people are using exceptionally old Julia releases, frankly they should expect that lots of what they use is deprecated/ unmaintained.

Yeah, unfortunately the precompilation printing is only available with 1.11, it doesn't work with LTS. Even if 1.10 is the next LTS, it won't show up until the next LTS after that for everyone bound to LTS.

I could add one-time printing into the constructor of FormatSpec, which probably catches everyone that uses the package. I guess that would break any and all printing that assumes it just works though.

I would suggest putting it in an if precompiling_output block or something

This package has compats back to 1.0, is there a version of that that goes that far back? Kinda bad that there's no CI set up here :/

@MasonProtter
Copy link
Contributor

Seems to work on v1.0:

mason:~$ julia +1.0 -q --startup=no
julia> ccall(:jl_generating_output, Cint, ())
0

@MasonProtter
Copy link
Contributor

mason:~$ cat Foo/src/Foo.jl
module Foo

if ccall(:jl_generating_output, Cint, ()) == 1
    println("hellllllo!")
end

end # module
mason:~$ julia +1.0 -q --startup=no
julia> using Foo
hellllllo!

julia> exit()
mprotter@dirac:~$ julia +1.0 -q --startup=no
julia> using Foo

julia> exit()

@Seelengrab
Copy link
Contributor Author

Hmm.. I've added another dependency to the fake chain, ala Baba -> Baz -> Foobar -> Formatting and if I nuke the packages from .julia/compiled, I truly do get triple the output if it's in __init__, even if it's wrapped in jl_generating_output:

image

which is way too much. This needs a better solution :(

@Seelengrab
Copy link
Contributor Author

Seelengrab commented Feb 24, 2024

Precompilation wipes the slate clean when it comes to global state, so I can't even really cache this properly, right? Seems like this will have to be hacked into some other place, perhaps the constructor here would be a good candidate? This will delay printing the warning until the first time someone formats something, but at least it'll only be to stderr. The upside is that this will work on all currently supported versions, so it has the biggest reach.

@MasonProtter
Copy link
Contributor

the generating_output thing shouldn't be in init, it should be toplevel in the package.

@Seelengrab
Copy link
Contributor Author

That did the trick! This really should be documented & made official. Only shows up on <1.11 when having Formatting.jl as a direct dependency, but oh well. All is good on 1.11 now!

@MasonProtter
Copy link
Contributor

Yeah I only learned that trick by @macroexpanding PrecompileTools.@compile_workload

@kescobo
Copy link
Member

kescobo commented Feb 24, 2024

@Seelengrab I can't get to this right now, but please feel free to ping me again if I haven't taken care of it by Monday

@Seelengrab
Copy link
Contributor Author

Will do!

src/Formatting.jl Outdated Show resolved Hide resolved
src/Formatting.jl Outdated Show resolved Hide resolved
@kescobo kescobo merged commit 9ebb21c into JuliaIO:master Feb 26, 2024
@Seelengrab
Copy link
Contributor Author

Thanks @kescobo!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants