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

Otp24 #136

Merged
merged 12 commits into from
Jul 15, 2021
Merged

Otp24 #136

merged 12 commits into from
Jul 15, 2021

Conversation

NelsonVides
Copy link
Collaborator

@NelsonVides NelsonVides commented May 12, 2021

Waiting for esl/amoc_rest#8

@NelsonVides NelsonVides marked this pull request as ready for review May 12, 2021 22:31
@gustawlippa
Copy link
Contributor

gustawlippa commented May 14, 2021

The tests are run for OTP 21.3 but pg was introduced in OTP 23.0. This is a welcome change but it should somehow work with the CI.

@NelsonVides
Copy link
Collaborator Author

NelsonVides commented May 14, 2021

The tests are run for OTP 21.3 but pg was introduced in OTP 23.0. This is a welcome change but it should somehow work with the CI.

And that explains the failure, but that makes it all a bit more complicated, because pg2 was completely removed with otp24, which means we could only support the last two major releases, or not support the last. Or do some hacks to compile different things with different versions 😕

@DenysGonchar @chrzaszcz

@DenysGonchar
Copy link
Collaborator

yeah, that is known issues, and that's exactly the reason why we didn't have CI testing against OTP23. pg2 became deprecated since that release, but we didn't want to relax dialyzer/xref verification rules.

@DenysGonchar
Copy link
Collaborator

I guess now, having 2 major OTP release with pg module, we can replace pg2 with pg.
Of course, we would have to make a new release with the comment that only OTP23 and OTP24 are supported and those who need to use pre OTP23 version can still use the previous Amoc release.

@@ -181,7 +181,6 @@ handle_call({change_rate_gradually, Name, LowRate, HighRate,
handle_call({stop, Name}, _From, State) ->
case all_processes(Name, stop) of
ok ->
pg2:delete(Name),
Copy link
Collaborator

Choose a reason for hiding this comment

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

is group removed in this case? can we recreate the group with the same name after this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/erlang/otp/blob/4d2a17302a79a86b6c74c769b9fafd9fbd9be516/lib/kernel/src/pg.erl#L21-L53 Here's an explanation, non-existent and empty groups are treated the same in this new implementation, the reason being agreement in the case of coming back from a network split.

@NelsonVides
Copy link
Collaborator Author

Github actions have some issue with nifs for a wxWidget dep not being loaded, no idea why, but that doesn't happen locally on my system. Needs investigation.

Use erlef/setup-beam@v1
Use rebar3 given by erlef/setup-beam@v1
Test only OTP23 and OTP23
This makes sure that attributes and compile flags aren't removed from
the beam files and module_info() can still find them. Latest relx
versions enforced stripping prod releases, so we need to explicitly
say otherwise.
Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

Looks good! There is one minor remark from me.

test/amoc_api_scenarios_handler_SUITE.erl Outdated Show resolved Hide resolved
Loading everything reachable is an overkill, and sometimes means that
some completely unneeded modules that cannot be loaded will introduce
errors to the pipeline.
Copy link
Collaborator

@DenysGonchar DenysGonchar left a comment

Choose a reason for hiding this comment

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

It was surprisingly hard, but I really like the changes.
MB we should make a follow-up ticket to add a note regarding {debug_info, keep} relx flag in amoc documentation. I think it should be added here, near to -required_variable(...) attribute explanation.

@DenysGonchar DenysGonchar merged commit bec620d into master Jul 15, 2021
@DenysGonchar DenysGonchar deleted the otp24 branch July 15, 2021 11:24
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.

4 participants