[erlang-questions] Deprecation warnings: OTP20 and OTP21 compatible code

[Danil Zagoskin]

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 | z...@gosk.in

[Jesper Louis Andersen]

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.

_______________________________________________
erlang-questions mailing list
erlang-q...@erlang.org
http://erlang.org/mailman/listinfo/erlang-questions

[Loïc Hoguin]

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.

> Danil Zagoskin | z...@gosk.in <mailto:z...@gosk.in>
> _______________________________________________
> erlang-questions mailing list
> erlang-q...@erlang.org <mailto:erlang-q...@erlang.org>
> http://erlang.org/mailman/listinfo/erlang-questions

>
>
>
> _______________________________________________
> erlang-questions mailing list
> erlang-q...@erlang.org
> http://erlang.org/mailman/listinfo/erlang-questions
>

--
Loïc Hoguin
https://ninenines.eu

[Fred Hebert]

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.

[Jesper Louis Andersen]

On Wed, Jun 6, 2018 at 1:12 PM Loïc Hoguin <es...@ninenines.eu> 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.

[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.

[Loïc Hoguin]

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

[José Valim]

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

www.plataformatec.com.br

Skype: jv.ptec

Founder and Director of R&D

[Bryan Hunt]

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

[Fred Hebert]

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.

[José Valim]

 

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?