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

Further adapt to OTP 24 + adopt GHA #539

Merged
merged 5 commits into from
Feb 25, 2021
Merged

Further adapt to OTP 24 + adopt GHA #539

merged 5 commits into from
Feb 25, 2021

Conversation

paulo-ferraz-oliveira
Copy link
Contributor

@paulo-ferraz-oliveira paulo-ferraz-oliveira commented Feb 7, 2021

This pull request builds on top of my previous one.

On the one hand, I prefer erl_anno:line/1 to matching on an "opaque" structure, since this eases maintenance.

On the other hand, with a more recent version of OTP (from master), I got new warnings (introduced by new behaviour from erlang/otp#2995), now fixed.

Note: this doesn't fundamentally change the way we handle the location, but it meant I was assuming that the format for that might not change, which might not be true (since OTP 24 isn't out yet).


line_from_loc({Line, _Col}) -> Line;
line_from_loc(Line) -> Line.
make_varname(Prefix, CallAnno) ->
Copy link
Contributor Author

@paulo-ferraz-oliveira paulo-ferraz-oliveira Feb 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many more variables should be called Anno instead of Line, within this file, but I didn't want to pollute the update.

@paulo-ferraz-oliveira
Copy link
Contributor Author

I'm pushing a hack "fix" to the tests, since I keep getting unexpected results (discussed here) that mislead me into thinking I added/changed something that start causing an issue.

@paulo-ferraz-oliveira paulo-ferraz-oliveira marked this pull request as draft February 8, 2021 01:30
@paulo-ferraz-oliveira
Copy link
Contributor Author

Moving to draft, since I'm going to try and replace Travis CI with GitHub Actions (running here, for testing purposes).

@paulo-ferraz-oliveira
Copy link
Contributor Author

I've removed gen_fsm from the scope of OTP 22+ tests.

@paulo-ferraz-oliveira paulo-ferraz-oliveira changed the title Further adapt to OTP 24 Further adapt to OTP 24 + adopt GHA Feb 24, 2021
@paulo-ferraz-oliveira
Copy link
Contributor Author

After many a experiment with Github Actions and failing tests, I found that I was missing container option --user 1001. All tests are now passing, as evidenced by Embrace the future at https://github.com/paulo-ferraz-oliveira/lager/actions.

@paulo-ferraz-oliveira paulo-ferraz-oliveira marked this pull request as ready for review February 24, 2021 00:10
rebar.config Outdated
@@ -21,7 +21,7 @@

{erl_opts, [
{lager_extra_sinks, ['__lager_test_sink']},
{platform_define, "^(19|20|21|22)", test_statem},
{platform_define, "^(19|20|21|22|23|24)", test_statem},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not the best approach. Suggestions are welcome.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguably we'd remove 19 and 20 from this list since we only support the last two stable releases.

Possibly we can also just kill this check entirely now.

@michaelklishin
Copy link

@paulo-ferraz-oliveira beat me to some of these changes. This seems a bit more urgent now that OTP 24 has reached a public RC.

@mrallen1 merging this would help team RabbitMQ test on OTP 24 (and indirectly test Lager on it).

@paulo-ferraz-oliveira
Copy link
Contributor Author

@michaelklishin: cool. The main goal was to prepare for OTP 24, but I thought I could fix the tests too, while moving to GitHub Actions, so I did.

We'd appreciate a release too, since MANY of our lib.s still depend on lager, though I'd really like to have #540 included in that release too. 😄

@Vagabond
Copy link
Member

I don't know much about GHA, but as long as it's more reliable than Travis I don't object.

The changes look fine. I think we can probably just remove the platform_define entirely at this point.

@mrallen1 any thoughts?

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Feb 24, 2021

@mrallen1 already told me (before) we don't need to keep supporting stuff prior to OTP 21, which is why the GHA element doesn't make a reference to it.

I'll remove it , and also propose further cleanup (I can see references to OTP 18, for example).

There you go: f2ae8fe.

@paulo-ferraz-oliveira
Copy link
Contributor Author

@Vagabond, should we get rid of appveyor also?

@jadeallenx
Copy link
Member

@paulo-ferraz-oliveira @lukebakken built the appveyor stuff to test lager on Windows Erlang builds (which I guess was something that RabbitMQ needed) but if it's bit rotted and broken, remove it

@jadeallenx
Copy link
Member

@Vagabond @paulo-ferraz-oliveira Kill anything prior to OTP 20 with extreme prejudice - it's on the TODO list, just haven't TODONE it...

@paulo-ferraz-oliveira
Copy link
Contributor Author

@mrallen1: appveyor is failing, but I'm not sure it's broken, which is why I asked 😄. I don't know what versions it runs on, or what's the target (the yml is not very informative).

Regarding OTP 20, I cleaned pre OTP-21 stuff from the tests, but didn't go after anything else.

I'm OK for merge if you are.

@lukebakken
Copy link
Contributor

If I have time I'll re-visit a Windows build but yeah feel free to nuke it for now.

@michaelklishin
Copy link

@michaelklishin: cool. The main goal was to prepare for OTP 24, but I thought I could fix the tests too, while moving to GitHub Actions, so I did.

We'd appreciate a release too, since MANY of our lib.s still depend on lager, though I'd really like to have #540 included in that release too. 😄

No objections from me. I just hope we will have an OTP 24-compatible release in the next couple of weeks so that all Lager users can begin testing on the OTP 24 RC1, even if they build every dependency from source :)

@jadeallenx jadeallenx merged commit d525470 into erlang-lager:master Feb 25, 2021
@jadeallenx
Copy link
Member

Thank you very much. I know this was a lot of work - really appreciate it.

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

Successfully merging this pull request may close these issues.

None yet

5 participants