Remove dependency on hut #267

saleyn · 2021-06-26T14:47:35Z

Removed logging dependency on hut.

cw789 · 2021-06-26T16:37:23Z

Thank you so much. 💛
This is / was the last thing that does depend on rebar in my deps.

saleyn · 2021-06-27T20:31:46Z

The OTP log macros expect either a text string or a map with an error report.
Implementation was adjusted accordingly.

seriyps · 2021-06-27T23:57:30Z

Oh, but why do you need a new header file? I think it might be better to just use OTP ?LOG_* macros directly?
Also, it might be a good idea to add the domain meta-key https://erlang.org/doc/man/logger_filters.html (say, [gen_smtp, client], [gen_smtp, server]) or maybe just [gen_smtp])

saleyn · 2021-06-28T03:31:04Z

The kernel's ?LOG_* macros prefer log reports as opposed to formatted log messages, so, to use them directly would require to convert all calls to pass log reports (i.e. a hash table of key/value pairs). This is doable but not a backward-compatible change for logging in this project.

saleyn · 2021-06-28T03:57:54Z

Regarding the failing CI run - in OTP 24 the erlang:get_stacktrace/0 was removed, and the rebar3 included in the project is failing when run on a system with the latest Erlang/OTP. Probably it's best not to include the rebar3 in the project, but to use the externally installed rebar3?

seriyps · 2021-06-28T06:57:44Z

?LOG_INFO("hello ~s", ["world"]) works just as you expect...

saleyn · 2021-06-28T14:34:47Z

?LOG_INFO("hello ~s", ["world"]) works just as you expect...

For some reason when running in OTP 24 this was causing some crash errors in the logger. Maybe it was something related to my environment. I'll double check.

saleyn · 2021-06-28T18:17:04Z

The issue was related to my local environment. It seems to be good now.

seriyps

Thanks! Looks good, but not sure about Ranch upgrade

seriyps · 2021-06-28T19:24:46Z

rebar.config

@@ -26,8 +26,7 @@
  {smtp_rfc822_parse, return_error, 2}
 ]}.

-{deps, [{ranch, ">= 1.7.0"},
-        {hut, "1.3.0"}]}.
+{deps, [{ranch, "2.0.0"}]}.


This version change seems unrelated. Is it necessary?
I think we have some code to support both 1.x and 2.x. So, if you really want to drop 1.x support, it's better to do as a separate PR.

saleyn · 2021-06-28T20:16:12Z

Is this really necessary to include rebar3 binary in the repos? This is generally not considered a good practice. The CI environment can get a binary of rebar3 independently of this project. Note that running the build with the old rebar3 in the master on the system with OTP 24 installed crashes with:

$ make
...
escript: exception error: undefined function erlang:get_stacktrace/0
  in function  rebar3:main/1 (/opt/rebar3-3.11.0/src/rebar3.erl, line 72)
  in call from escript:run/2 (escript.erl, line 750)
  in call from escript:start/1 (escript.erl, line 277)
  in call from init:start_em/1 
  in call from init:do_boot/3 
make: *** [Makefile:4: compile] Error 127

seriyps · 2021-06-28T21:23:04Z

I honestly don't have an opinion on whether include it or not. But I think it might be nice to at least have a fixed version of rebar3 in CI (by, eg, downloading it not from https://s3.amazonaws.com/rebar3/rebar3 but from, say, github releases: https://github.com/erlang/rebar3/releases/download/3.16.1/rebar3; not sure which exact version would work here though)

mworrell · 2021-06-29T07:00:01Z

The newest rebar3 only works with the last three OTP versions (it doesn't run anymore on OTP20).

With Zotonic we tried to download a fixed version from GH, but we ran into build issues where our downloads were failing. We reverted back to using the newest rebar3 version.

saleyn · 2021-06-29T11:00:35Z

The newest rebar3 only works with the last three OTP versions (it doesn't run anymore on OTP20).

With Zotonic we tried to download a fixed version from GH, but we ran into build issues where our downloads were failing. We reverted back to using the newest rebar3 version.

@mworrell, wouldn't it make sense though to make the version-specific rebar3 download to be part of the CI build for a given environment rather than including it in the project? Github provides a template example for setting this up.

cw789 · 2021-09-03T16:48:49Z

Is there anything I (as noob) can help to get this pull request ready?

seriyps · 2021-09-03T21:42:17Z

I think we should merge #273 and then rebase this PR on top of it. Not sure if any help is needed besides maybe reviewing #273.

mworrell · 2021-09-21T18:45:48Z

#273 has been merged, so this can now be rebased on top of master

seriyps

Hey, may you, please, revert your changes in rebar3 file? We already updated it in #273

Remove extra brackets Revert ranch version change Revert rebar3

seriyps · 2021-09-21T21:18:48Z

Guess this line should be removed as well:

gen_smtp/rebar.config

Line 47 in d012840

hut

seriyps · 2021-09-21T23:20:25Z

Also please consider adding the domain to the log metadata. This way it would be much easier to filter the logs created by gen_smtp
#267 (comment)

smth like

?LOG_INFO("smth smth ~p", [Param], #{domain => [gen_smtp]}) or ?LOG_INFO("smth smth ~p", [Param], #{domain => [gen_smtp, client]}) / ?LOG_INFO("smth smth ~p", [Param], #{domain => [gen_smtp, server]}) etc.

so later some special treatment can be done for the logs originating from from the gen_smtp, say:

 {handler, default, logger_std_h,
   #{
         level => debug,
         filters => [{stop_gen_smtp, {fun logger_filters:domain/2, {stop, sub, [gen_smtp]}}}],
...

but I won't insist if you have no time for that.

cw789 · 2022-02-12T07:46:18Z

May I kindly ask if there is a chance to get this into the next release.

seriyps · 2022-02-12T17:02:41Z

Created a new PR #309 that does more or less the same

mworrell · 2022-02-12T23:03:55Z

Merged #309

mworrell · 2022-02-12T23:04:49Z

Closing, as #309 has the same effect of removing hut.

mworrell approved these changes Jun 26, 2021

View reviewed changes

saleyn force-pushed the logger branch from 1407f8a to 7012f3d Compare June 28, 2021 17:52

seriyps reviewed Jun 28, 2021

View reviewed changes

seriyps approved these changes Jun 28, 2021

View reviewed changes

mworrell mentioned this pull request Sep 21, 2021

Add OTP-24 to CI #273

Merged

saleyn force-pushed the logger branch from 0323b59 to 2824456 Compare September 21, 2021 20:22

seriyps approved these changes Sep 21, 2021

View reviewed changes

seriyps requested changes Sep 21, 2021

View reviewed changes

Remove dependency on hut

8d74bf3

Remove extra brackets Revert ranch version change Revert rebar3

saleyn force-pushed the logger branch from 2824456 to 8d74bf3 Compare September 21, 2021 21:02

seriyps mentioned this pull request Feb 12, 2022

Use OTP logger directly; drop hut dependency #309

Merged

mworrell closed this Feb 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dependency on hut #267

Remove dependency on hut #267

saleyn commented Jun 26, 2021 •

edited

Loading

cw789 commented Jun 26, 2021

saleyn commented Jun 27, 2021 •

edited

Loading

seriyps commented Jun 27, 2021 •

edited

Loading

saleyn commented Jun 28, 2021

saleyn commented Jun 28, 2021

seriyps commented Jun 28, 2021

saleyn commented Jun 28, 2021 •

edited

Loading

saleyn commented Jun 28, 2021

seriyps left a comment •

edited

Loading

seriyps Jun 28, 2021

saleyn commented Jun 28, 2021 •

edited

Loading

seriyps commented Jun 28, 2021

mworrell commented Jun 29, 2021

saleyn commented Jun 29, 2021 •

edited

Loading

cw789 commented Sep 3, 2021

seriyps commented Sep 3, 2021 •

edited

Loading

mworrell commented Sep 21, 2021

seriyps left a comment

seriyps commented Sep 21, 2021

seriyps commented Sep 21, 2021

cw789 commented Feb 12, 2022

seriyps commented Feb 12, 2022

mworrell commented Feb 12, 2022

mworrell commented Feb 12, 2022

Remove dependency on hut #267

Remove dependency on hut #267

Conversation

saleyn commented Jun 26, 2021 • edited Loading

cw789 commented Jun 26, 2021

saleyn commented Jun 27, 2021 • edited Loading

seriyps commented Jun 27, 2021 • edited Loading

saleyn commented Jun 28, 2021

saleyn commented Jun 28, 2021

seriyps commented Jun 28, 2021

saleyn commented Jun 28, 2021 • edited Loading

saleyn commented Jun 28, 2021

seriyps left a comment • edited Loading

Choose a reason for hiding this comment

seriyps Jun 28, 2021

Choose a reason for hiding this comment

saleyn commented Jun 28, 2021 • edited Loading

seriyps commented Jun 28, 2021

mworrell commented Jun 29, 2021

saleyn commented Jun 29, 2021 • edited Loading

cw789 commented Sep 3, 2021

seriyps commented Sep 3, 2021 • edited Loading

mworrell commented Sep 21, 2021

seriyps left a comment

Choose a reason for hiding this comment

seriyps commented Sep 21, 2021

seriyps commented Sep 21, 2021

cw789 commented Feb 12, 2022

seriyps commented Feb 12, 2022

mworrell commented Feb 12, 2022

mworrell commented Feb 12, 2022

saleyn commented Jun 26, 2021 •

edited

Loading

saleyn commented Jun 27, 2021 •

edited

Loading

seriyps commented Jun 27, 2021 •

edited

Loading

saleyn commented Jun 28, 2021 •

edited

Loading

seriyps left a comment •

edited

Loading

saleyn commented Jun 28, 2021 •

edited

Loading

saleyn commented Jun 29, 2021 •

edited

Loading

seriyps commented Sep 3, 2021 •

edited

Loading