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

Add support for OTP 27 #585

Merged
merged 7 commits into from
Jun 26, 2024
Merged

Add support for OTP 27 #585

merged 7 commits into from
Jun 26, 2024

Conversation

kivra-pauoli
Copy link
Contributor

👋

I'm not sure what the support policy/maintenance range for brod is, but one could already start moving toward OTP 27 for systems that are adopting it.

I tested these changes in my fork and CI was Ok using Erlang/OTP 27 and rebar3 3.23: https://github.com/kivra-pauoli/brod/actions/runs/9564503494/job/26365813507

I'll add comments on top of the changes to ease review. I tried to do commit-by-commit so it's easier to do so too, in any case.

Approaches OTP 27 -based development
I noticed rebar.lock was outdated against rebar.config
format_status/1 was introduced in OTP 25

format_status/2 is now having brod compilation fail,
because of warnings_as_errors (in OTP 27)

This commit presents one possibility to solve
the issue: adopt the new format_status, which means
that effectively brod becomes >= OTP 25

Another possibility is to remove warnings_as_errors
and keep current behaviour until the function
is removed from OTP

I opted to remove format_status/2 from brod_supervisor/3
as it's doing mostly nothing relevant except "flagging"
state.module as "Callback"
rebar.lock Outdated Show resolved Hide resolved
@@ -36,7 +36,7 @@
, handle_info/2
, init/1
, terminate/2
, format_status/2
Copy link
Contributor

Choose a reason for hiding this comment

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

please keep format_status/2 for older version OTP

Copy link
Contributor Author

@kivra-pauoli kivra-pauoli Jun 24, 2024

Choose a reason for hiding this comment

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

To do this we need to remove warnings_as_errors from the compilation options, as Erlang/OTP complains then the process stops. I'll push it, reversing the change, so you can check what happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add an OTP_RELEASE exception, if that's what you mean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zmstone
Copy link
Contributor

zmstone commented Jun 23, 2024

Please add OTP 27 to the ci matrix.

@kivra-pauoli
Copy link
Contributor Author

Please add OTP 27 to the ci matrix.

CI presents two distinct scenarios:

  1. env variables determine a specific OTP/Rebar3 combo
  2. CI matrix determines a combo of stuff to test on

I'm gonna update the matrix, but I don't know how you prefer to deal with the env variables being fixed

@zmstone
Copy link
Contributor

zmstone commented Jun 24, 2024

sorry, it seems the erlef/setup-beam@v1 has rebar3 built on otp 26.
not sure how to fix, you can remove otp 27 from the ci yaml for now if you wish.

@kivra-pauoli
Copy link
Contributor Author

kivra-pauoli commented Jun 24, 2024

sorry, it seems the erlef/setup-beam@v1 has rebar3 built on otp 26.

It's typically build on top of current-2, so that'd be 25. If I didn't pin the rebar3 executable correctly I will (edit: 3.23.0 should do it - check 42a954b).

@kivra-pauoli kivra-pauoli requested a review from zmstone June 24, 2024 11:38
@zmstone zmstone merged commit 01ad88e into kafka4beam:master Jun 26, 2024
14 checks passed
@kivra-pauoli
Copy link
Contributor Author

Thanks much. Do you have an ETA on the next release that includes this change?

@kivra-pauoli kivra-pauoli deleted the chore/support-otp27 branch June 26, 2024 11:25
@zmstone
Copy link
Contributor

zmstone commented Jun 26, 2024

included in 3.19.1

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.

2 participants