Deprecation warnings: OTP20 and OTP21 compatible code

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Deprecation warnings: OTP20 and OTP21 compatible code

Danil Zagoskin-2
Hi!

This is a shout of pain we got while preparing our project for upcoming OTP release.

The problem:
 * We compile our code for production using warnings_as_errors option. This helps us to keep the code tidy.
 * OTP21 deprecates some functions (erlang:get_stacktrace/0, ssl:ssl_accept/0, may be more)
 * OTP20 does not support new API (catch C:R:S, ssl:handshake/2)

So, to be able to go with both OTP20 and OTP21, we need some hacks.

First thought: let's pass {nowarn_deprecated_function, [...]} with Emakefile, letting old code
compile smootly in newer OTP.
But that cannot be done — this option is only recognized when given in files.

Second thought: OK, let's just add this option to all affected files.
But "Warning: erlang:get_stacktrace/0 is not a deprecated function"

So, we needed to implement some preprocessor logic which adds nowarn only when compiling with OTP21.
Luckily, there is a OTP_RELEASE var defined in OTP21:
-ifdef(OTP_RELEASE).
-compile({nowarn_deprecated_function, [{erlang, get_stacktrace, 0}]}).
-endif.

Adding that to tens of files (large project, lots of dependencies in-tree) seemed too ugly,
so we implemented a parse_transform which can be added to Emakefile.


--
Danil Zagoskin | [hidden email]

_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Deprecation warnings: OTP20 and OTP21 compatible code

Jesper Louis Andersen-2
For a larger project, I stumbled into the following while trying to build it on OTP-21.0-rc2-69-g9ae2044073:

* `erlexec` sets `warnings_as_errors` which fails due to `erlang:get_stacktrace/0`
* `meck` sets `warnings_as_errors` which fails due to `erlang:get_stacktrace/0`
* `ranch_proxy_protocol` sets `warnings as errors which fails due to `ssl:ssl_accept/3`
* `jose` sets `warnings_as_errors` which fails due to `erlang:get_stacktrace/0`
* `cowboy_cors` sets `warnings_as_errors` which fails due to `erlang:get_stacktrace/0`

In effect, the warnings_as_errors is now a useless option on any system which wants an upgrade-path from 20 to 21. If you start using the new syntax straight away, you lock out users so they can't use 20 anymore, and in any software project, some system has to support multiple versions in order to do code upgrade[0].

Current fix for the above is to use rebar3's overrides:

{overrides,
    [{override, erlexec, [{erl_opts, [debug_info]}]},
     {override, ranch_proxy_protocol, [{erl_opts, [debug_info]}]},
     {override, cowboy_cors, [{erl_opts, [debug_info]}]},
     {override, jose, [{erl_opts, [debug_info]}]},    
     {override, meck, [{erl_opts, [debug_info]}]}]
}.

which plugs the hole for now. But I'm a bit hesitant to just write those projects since I'm not sure I have a good, easily implemented transition path here. I think one might be able to use a bit of macro-magic to accept the particular deprecation warning on 21 (only!) as not-an-error-and-intended-for-now, so one can postpone the 21 upgrade a bit in the software.

NOTE: This isn't a problem in fully vendored software where you control everything. You can simply define when you upgrade and handle all dependencies directly. But in a setting where other people rely on your libraries, it is usually a bit more flexibile to allow them the ability to run on the current version as well as the version one level back.

[0] Note this is somewhat similar to a code_change/3: Some function needs to understand how to transform old data to new data and thus work with both versions for a while.

On Tue, Jun 5, 2018 at 2:22 PM Danil Zagoskin <[hidden email]> wrote:
Hi!

This is a shout of pain we got while preparing our project for upcoming OTP release.

The problem:
 * We compile our code for production using warnings_as_errors option. This helps us to keep the code tidy.
 * OTP21 deprecates some functions (erlang:get_stacktrace/0, ssl:ssl_accept/0, may be more)
 * OTP20 does not support new API (catch C:R:S, ssl:handshake/2)

So, to be able to go with both OTP20 and OTP21, we need some hacks.

First thought: let's pass {nowarn_deprecated_function, [...]} with Emakefile, letting old code
compile smootly in newer OTP.
But that cannot be done — this option is only recognized when given in files.

Second thought: OK, let's just add this option to all affected files.
But "Warning: erlang:get_stacktrace/0 is not a deprecated function"

So, we needed to implement some preprocessor logic which adds nowarn only when compiling with OTP21.
Luckily, there is a OTP_RELEASE var defined in OTP21:
-ifdef(OTP_RELEASE).
-compile({nowarn_deprecated_function, [{erlang, get_stacktrace, 0}]}).
-endif.

Adding that to tens of files (large project, lots of dependencies in-tree) seemed too ugly,
so we implemented a parse_transform which can be added to Emakefile.


--
Danil Zagoskin | [hidden email]
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions

_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Deprecation warnings: OTP20 and OTP21 compatible code

Loïc Hoguin-3
It's worth noting that you did not have an issue with Cowboy, Ranch and
friends. That's because the default policy in Erlang.mk is to NOT use
warnings_as_errors for dependencies (and as a result this option is not
included in the generated rebar.config).

This saves a lot of headaches. erlang:get_stacktrace/0 is notable in how
widely used it is but issues with warnings_as_error have always existed
for as long as I've been doing Erlang. Tools should really have a saner
default policy for this. The build shouldn't break for your users just
because a new warning was introduced.

On 06/06/2018 01:01 PM, Jesper Louis Andersen wrote:

> For a larger project, I stumbled into the following while trying to
> build it on OTP-21.0-rc2-69-g9ae2044073:
>
> * `erlexec` sets `warnings_as_errors` which fails due to
> `erlang:get_stacktrace/0`
> * `meck` sets `warnings_as_errors` which fails due to
> `erlang:get_stacktrace/0`
> * `ranch_proxy_protocol` sets `warnings as errors which fails due to
> `ssl:ssl_accept/3`
> * `jose` sets `warnings_as_errors` which fails due to
> `erlang:get_stacktrace/0`
> * `cowboy_cors` sets `warnings_as_errors` which fails due to
> `erlang:get_stacktrace/0`
>
> In effect, the warnings_as_errors is now a useless option on any system
> which wants an upgrade-path from 20 to 21. If you start using the new
> syntax straight away, you lock out users so they can't use 20 anymore,
> and in any software project, some system has to support multiple
> versions in order to do code upgrade[0].
>
> Current fix for the above is to use rebar3's overrides:
>
> {overrides,
>      [{override, erlexec, [{erl_opts, [debug_info]}]},
>       {override, ranch_proxy_protocol, [{erl_opts, [debug_info]}]},
>       {override, cowboy_cors, [{erl_opts, [debug_info]}]},
>       {override, jose, [{erl_opts, [debug_info]}]},
>       {override, meck, [{erl_opts, [debug_info]}]}]
> }.
>
> which plugs the hole for now. But I'm a bit hesitant to just write those
> projects since I'm not sure I have a good, easily implemented transition
> path here. I think one might be able to use a bit of macro-magic to
> accept the particular deprecation warning on 21 (only!) as
> not-an-error-and-intended-for-now, so one can postpone the 21 upgrade a
> bit in the software.
>
> NOTE: This isn't a problem in fully vendored software where you control
> everything. You can simply define when you upgrade and handle all
> dependencies directly. But in a setting where other people rely on your
> libraries, it is usually a bit more flexibile to allow them the ability
> to run on the current version as well as the version one level back.
>
> [0] Note this is somewhat similar to a code_change/3: Some function
> needs to understand how to transform old data to new data and thus work
> with both versions for a while.
>
> On Tue, Jun 5, 2018 at 2:22 PM Danil Zagoskin <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hi!
>
>     This is a shout of pain we got while preparing our project for
>     upcoming OTP release.
>
>     The problem:
>       * We compile our code for production using warnings_as_errors
>     option. This helps us to keep the code tidy.
>       * OTP21 deprecates some functions (erlang:get_stacktrace/0,
>     ssl:ssl_accept/0, may be more)
>       * OTP20 does not support new API (catch C:R:S, ssl:handshake/2)
>
>     So, to be able to go with both OTP20 and OTP21, we need some hacks.
>
>     First thought: let's pass {nowarn_deprecated_function, [...]} with
>     Emakefile, letting old code
>     compile smootly in newer OTP.
>     But that cannot be done — this option is only recognized when given
>     in files.
>
>     Second thought: OK, let's just add this option to all affected files.
>     But "Warning: erlang:get_stacktrace/0 is not a deprecated function"
>
>     So, we needed to implement some preprocessor logic which adds nowarn
>     only when compiling with OTP21.
>     Luckily, there is a OTP_RELEASE var defined in OTP21:
>     -ifdef(OTP_RELEASE).
>     -compile({nowarn_deprecated_function, [{erlang, get_stacktrace, 0}]}).
>     -endif.
>
>     Adding that to tens of files (large project, lots of dependencies
>     in-tree) seemed too ugly,
>     so we implemented a parse_transform which can be added to Emakefile.
>
>     Parse transform itself:
>     https://gist.github.com/stolen/6a55221ffb906bde712cd939a729718d
>
>     --
>     Danil Zagoskin | [hidden email] <mailto:[hidden email]>
>     _______________________________________________
>     erlang-questions mailing list
>     [hidden email] <mailto:[hidden email]>
>     http://erlang.org/mailman/listinfo/erlang-questions
>
>
>
> _______________________________________________
> erlang-questions mailing list
> [hidden email]
> http://erlang.org/mailman/listinfo/erlang-questions
>

--
Loïc Hoguin
https://ninenines.eu
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Deprecation warnings: OTP20 and OTP21 compatible code

Fred Hebert-2
On 06/06, Loïc Hoguin wrote:

>It's worth noting that you did not have an issue with Cowboy, Ranch
>and friends. That's because the default policy in Erlang.mk is to NOT
>use warnings_as_errors for dependencies (and as a result this option
>is not included in the generated rebar.config).
>
>This saves a lot of headaches. erlang:get_stacktrace/0 is notable in
>how widely used it is but issues with warnings_as_error have always
>existed for as long as I've been doing Erlang. Tools should really
>have a saner default policy for this. The build shouldn't break for
>your users just because a new warning was introduced.
>

Rebar3 leaves the warnings that libs put for themselves there.

There's a PR currently going about adding a 'del' override, which would
let someone remove the option by specifying:

{overrides, [
    {del, [
        {erl_opts, [warning_as_errors]}
    ]}
]}

and would let you instantly remove the option from all dependencies
rather than having to go through the override pattern Jesper has shown
here.

If/Once merged, it would help address the problem. I'm a bit hesitant to
go and disable some options _by default_ when library authors have set
them there for a reason.


Now for the case of get_stacktrace(), macros are pretty much the only
way we've found to deal with this. I'm a bit disappointed that the issue
where the change was made had this debated a long time
(https://github.com/erlang/otp/pull/1783) but the changes to the
language have been railroaded without regards to community complaints
there.

Rebar3 ended up going with a macro-heavy approach suggested by @okeuday
in here: https://github.com/erlang/rebar3/pull/1773/files

Libraries like GPB had to resort to more trickery since the files can be
static and embedded in other projects without pre-defined macros: https://github.com/tomas-abrahamsson/gpb/issues/134

This ended up being a big pain in the ass for a lot of people due to the
very aggressive deprecation schedule.
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Deprecation warnings: OTP20 and OTP21 compatible code

Jesper Louis Andersen-2
In reply to this post by Loïc Hoguin-3
On Wed, Jun 6, 2018 at 1:12 PM Loïc Hoguin <[hidden email]> wrote:
It's worth noting that you did not have an issue with Cowboy, Ranch and
friends. That's because the default policy in Erlang.mk is to NOT use
warnings_as_errors for dependencies (and as a result this option is not
included in the generated rebar.config).


IIRC, Rebar doesn't include the option by default either. But people like to add that option as a way to rid themselves of warnings in their projects. Normally, this is a fine and sensible thing to do, but I'm currently a bit torn on its usefulness in libraries other people rely on, and I might want to recommend people not adding it to those.

Generally I think we agree here.

Individual projects can override this setting when you assemble the software on a grander scale and force warnings_as_errors. It is always easier to add strictness at a higher level. But even then: if you don't control all of the source code, you must be prepared for parts of it lagging behind in warning count and/or quality.


_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Deprecation warnings: OTP20 and OTP21 compatible code

Nathaniel Waisbrot
> IIRC, Rebar doesn't include the option by default either. But people like to add that option as a way to rid themselves of warnings in their projects. Normally, this is a fine and sensible thing to do, but I'm currently a bit torn on its usefulness in libraries other people rely on, and I might want to recommend people not adding it to those.


If a project has tests (especially if they're run automatically on code changes), rebar3 can use profiles:

{profiles, [
            {test, [
                    {erl_opts, [warnings_as_errors]}
                   ]}
           ]}.


so that way your CI tests should fail and keep your project clean from errors. But at the same time it won't break as a dependency.
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Deprecation warnings: OTP20 and OTP21 compatible code

Loïc Hoguin-3
In reply to this post by Fred Hebert-2
On 06/06/2018 01:18 PM, Fred Hebert wrote:

> On 06/06, Loïc Hoguin wrote:
>> It's worth noting that you did not have an issue with Cowboy, Ranch
>> and friends. That's because the default policy in Erlang.mk is to NOT
>> use warnings_as_errors for dependencies (and as a result this option
>> is not included in the generated rebar.config).
>>
>> This saves a lot of headaches. erlang:get_stacktrace/0 is notable in
>> how widely used it is but issues with warnings_as_error have always
>> existed for as long as I've been doing Erlang. Tools should really
>> have a saner default policy for this. The build shouldn't break for
>> your users just because a new warning was introduced.
>>
>
> Rebar3 leaves the warnings that libs put for themselves there.
>
> There's a PR currently going about adding a 'del' override, which would
> let someone remove the option by specifying:
>
> {overrides, [
>     {del, [
>         {erl_opts, [warning_as_errors]}
>     ]}
> ]}
>
> and would let you instantly remove the option from all dependencies
> rather than having to go through the override pattern Jesper has shown
> here.
>
> If/Once merged, it would help address the problem. I'm a bit hesitant to
> go and disable some options _by default_ when library authors have set
> them there for a reason.

They have a reason of course: this option is great during development.
Everyone should use it.

Building as a dependency is a different context though and having your
dependencies break every time there's a new OTP version is not the best
user experience. I suppose Rebar3 has a way to have stricter compilation
profiles for use during development, but I haven't seen much use of
that. So it falls down to each dependency's user to fix those issues,
and that's a problem.

It being a default may or may not be a good idea, I don't know. All I do
know is that nobody complained and nobody asked for an option to disable
that behavior so I think it's good enough for now. It's one less problem
to worry about. Rebar has more users and with a different mindset so the
same may not apply.

--
Loïc Hoguin
https://ninenines.eu
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Deprecation warnings: OTP20 and OTP21 compatible code

José Valim-2
FWIW, Mix also disables warnings as errors for both Erlang (Rebar) and Elixir (Mix) dependencies, for exactly the same reasons outlined by Loïc.



José Valim
Skype: jv.ptec
Founder and Director of R&D

On Wed, Jun 6, 2018 at 1:38 PM, Loïc Hoguin <[hidden email]> wrote:
On 06/06/2018 01:18 PM, Fred Hebert wrote:
On 06/06, Loïc Hoguin wrote:
It's worth noting that you did not have an issue with Cowboy, Ranch and friends. That's because the default policy in Erlang.mk is to NOT use warnings_as_errors for dependencies (and as a result this option is not included in the generated rebar.config).

This saves a lot of headaches. erlang:get_stacktrace/0 is notable in how widely used it is but issues with warnings_as_error have always existed for as long as I've been doing Erlang. Tools should really have a saner default policy for this. The build shouldn't break for your users just because a new warning was introduced.


Rebar3 leaves the warnings that libs put for themselves there.

There's a PR currently going about adding a 'del' override, which would let someone remove the option by specifying:

{overrides, [
    {del, [
        {erl_opts, [warning_as_errors]}
    ]}
]}

and would let you instantly remove the option from all dependencies rather than having to go through the override pattern Jesper has shown here.

If/Once merged, it would help address the problem. I'm a bit hesitant to go and disable some options _by default_ when library authors have set them there for a reason.

They have a reason of course: this option is great during development. Everyone should use it.

Building as a dependency is a different context though and having your dependencies break every time there's a new OTP version is not the best user experience. I suppose Rebar3 has a way to have stricter compilation profiles for use during development, but I haven't seen much use of that. So it falls down to each dependency's user to fix those issues, and that's a problem.

It being a default may or may not be a good idea, I don't know. All I do know is that nobody complained and nobody asked for an option to disable that behavior so I think it's good enough for now. It's one less problem to worry about. Rebar has more users and with a different mindset so the same may not apply.

--
Loïc Hoguin
https://ninenines.eu
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions


_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Deprecation warnings: OTP20 and OTP21 compatible code

Bryan Hunt
In reply to this post by Danil Zagoskin-2

Jose, your pragmatic approach is *very* much appreciated.

In a top level project, having explicitly chosen to specify the `warning_as_errors` option, 
a failed build as a result of warnings is sensible.

In a dependency, enforcing a top level build failure due to it’s own deprecated code,
is at best, unhelpful.

While upgrading a project to compile/run on a modern Erlang - it is nearly impossible to avoid 
the case where one achieves a successful compilation, yet experience a slew of deprecation warnings.

I fork all my dependencies in a non-backward-compatibile way to work around this limitation, 
but this approach is less than desirable from the perspective of cohesion/fragmentation/anything really.

Bryan



_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Deprecation warnings: OTP20 and OTP21 compatible code

Fred Hebert-2
I'm one of the few people who actually at times enforces warnings as errors to dependencies, so that I catch prospective bugs early and can go open pull requests and patch things to make sure I never get surprised with a thing that doesn't build through deprecation.

I'd be open to having a mechanism to force-remove it, but I'd be annoyed to lose the ability to go add it back though.


On Wed, Jun 6, 2018 at 10:30 AM, Bryan Hunt <[hidden email]> wrote:

Jose, your pragmatic approach is *very* much appreciated.

In a top level project, having explicitly chosen to specify the `warning_as_errors` option, 
a failed build as a result of warnings is sensible.

In a dependency, enforcing a top level build failure due to it’s own deprecated code,
is at best, unhelpful.

While upgrading a project to compile/run on a modern Erlang - it is nearly impossible to avoid 
the case where one achieves a successful compilation, yet experience a slew of deprecation warnings.

I fork all my dependencies in a non-backward-compatibile way to work around this limitation, 
but this approach is less than desirable from the perspective of cohesion/fragmentation/anything really.

Bryan



_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions



_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Deprecation warnings: OTP20 and OTP21 compatible code

José Valim-2
 
I'd be open to having a mechanism to force-remove it, but I'd be annoyed to lose the ability to go add it back though.

Maybe have a mechanism that explicitly opts-in to keeping the warnings as errors from dependencies? Something like {warnings_as_errors_from_deps, keep | force | ignore}, but defaults to ignore?



_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions