Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ideas how to deal with warnings_as_errors in dependencies #1607

Closed
Licenser opened this issue Aug 14, 2017 · 5 comments
Closed

ideas how to deal with warnings_as_errors in dependencies #1607

Licenser opened this issue Aug 14, 2017 · 5 comments
Labels
enhancement new behaviour or additional functionality

Comments

@Licenser
Copy link
Contributor

Less a bug report (as it's not a bug) than an attempt to start a discussion for if this can be improved.

Disclaimer, writing this is partially based on Kyorai/riak_core#23 and a short discussion (more me asking a question;) with Fred on IRC about weather or not rebar3 errors on warnings_as_error for dependencies.

I think warnings_as_errors is a very good flag to use, it improves quality of code as it forces people to not be sloppy and ignore warnings. Personally I try set it on all my projects.

But with erlang moving faster recently, and quite aggressively depricating functions in every release this can be a problem.

To demonstrate I've created this minimal example project (abusing _checkouts please forgive me): https://github.com/Licenser/wae_test - tt demonstratses nicely what problem can arise here. When looking at it please imagine wae_offender to be a 3rd party library out of your control.

The fundamental issue is that today you can very quickly end up with dependencies that while working, will prevent your project from compiling. This is less of an issue for seasond Erlangers, but I imagine can be extremely frustrating for newcomers.

I can see a few (possible) solutions.

ask everyone to remove warnings_as_errors

I find this a horrible solution, it would solve the problem of warnings_as_errors but quite possibly decrease overall quality of code and even worse lead to libraries that just error (i.e. if we forget to update random->rand because we didn't get an error).

Me personally, for once, will refuse to do so.

make-someone-fix-it-solution

I am probably the most angry OSS developper ont his planet, so I really enjoy telling people "yes if you want that fixed go and make a PR that fixes it!", but, in this case fixing the compilation problems requires some real knowledge about erlang releases. For example just changing random to rand will not create new problems, and in the worst case (i.e. for riak_core for example) cause real problems and sneaky bugs. So while this sounds like a great way for the angry-me, the having-to-maintain-it-me disagrees, it sadly is not.

just-use-the-version-of-erlang-I-use

That kind of works until you run into the situation where multiple pelple employ it and there are conflicting versions. I.e. one library wants 18, another 19, and the project targets 20, oh my :( ...

overrides

This is the first thing that can be considered working I guess, add an override for each library that removes errors_as_warnings from the erl ops for the libraries in question. (see https://github.com/Licenser/wae_test/tree/override )

This is probably the solution I would pick today, but it again comes with quite a burden to get started and can easiley lead to problems down the road. As there is only an option to add or override the settings it requires to copy the erl_opts for each library copletely and overwrite the existing one with a slightly modified one. God forbid a update introduces a new option in erl_ops, that would esure days of fun debugging and hair pulling.

It also is not exactly a simple task for someone new to erlang "Find all the problematic libarires in _buidl/default/lib, look in their rebar.config, find the erl opts, make a table for them and change them" is really not a beginner friendly task.

ovberrides v2

Since overwrites are the closest thing to a real sollution I want to pick up here. This would be made a lot easyer by ofering 2 more options to overrides:

{overrides, [{add, app_name(), [{atom(), any()}]},
             {remove, app_name(), [{atom(), any()}]},
             {remove, [{atom(), any()}]},
             {override, app_name(), [{atom(), any()}]},
             {override, [{atom(), any()}]}]}.

With this addition it would now be possible to use:

{overrides, [{remove, [{erl_opts, [warnings}]}]}.

Remove wouls require to work the same way as add in it performing a 'deep unmerge' of the structure, but it would effectively solve the problem.

a global flag

Intoducing an global flag like {desp_supress_warnings_as_errors, true}. that will remove warnings in all depdendecies.

a local flag

Very much like adding remove but a local flag {supress_warnings_as_errors, true}. would allow overriding a dependencies rebar config w/o haivng to copy and keep in sync the erl_opts.

a commandline flag

rebar --no-deps-wae (name is just because I'm too lazy to type it all) that acts like the global flag described above would allow users to quickly disable warnings as errors for dependencies and compile their project.

@tsloughter
Copy link
Collaborator

I favor just removing it from the opts when compiling deps by default and we can have an override fro adding it back for deps or something.

@Licenser
Copy link
Contributor Author

Oh just removing it from deps also sounds like a solution! I forgot to list that :D

@ferd ferd added the enhancement new behaviour or additional functionality label Aug 30, 2017
@ghost
Copy link

ghost commented Jan 30, 2018

+1

{overrides, [{remove, [{erl_opts, [warnings}]}]}.

@ferd
Copy link
Collaborator

ferd commented Jun 22, 2018

This feature has been added to 3.6.0 as del for overrides.

@ferd ferd closed this as completed Jun 22, 2018
@vassilevsky
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new behaviour or additional functionality
Projects
None yet
Development

No branches or pull requests

4 participants