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

Brushups for OTP27 #77

Merged
merged 6 commits into from
Jun 12, 2024

Conversation

dw-kihara
Copy link
Contributor

@dw-kihara dw-kihara commented May 25, 2024

When trying OTP27, I encountered several problems:

  • iso8601 cannot be compiled.
  • Some libraries used by rebar3_lint cannot be compiled.
  • A type mismatch in aws_credentials_httpc:start/1 is detected.

To resolve these issues:

  • For the libraries used by rebar3_lint, I updated rebar3_lint and addressed the new linter warnings.
  • For aws_credentials_httpc:start/1, I adjusted the code without changing its behavior.
  • For iso8601, we can wait for the existing PR to be merged.

Closes #78

@dw-kihara
Copy link
Contributor Author

dw-kihara commented May 25, 2024

Oops....
rebar3_lint 3.2.4 requires at least OTP23, so I get it back to 3.0.1 which is cofigured to OTP21.
However katana-code (which rebar3_lint depends on) requires OTP23 from 2.0.0 and therefore using revar3_lint 3.0.1 still rejects OTP22.
To be bad, older version katana-code (prior to 2.0.0) was the cause of OTP27 incompatibilty.

To resolve this situation, we might have to drop OTP22 in order to support OTP27 in the future.

@dw-kihara dw-kihara marked this pull request as draft May 28, 2024 05:14
@dw-kihara
Copy link
Contributor Author

I'm converting this to a draft because there doesn't seem to be an easy way to make both OTP22 and OTP27 work fine.

@onno-vos-dev
Copy link
Member

Feel free to drop OTP 22 👍

@dw-kihara
Copy link
Contributor Author

Dropped support for OTP 22.
iso8601 is pending, but it can be updated later.

(Also squashed some unnecessary trial and error commits.)

@dw-kihara dw-kihara marked this pull request as ready for review May 28, 2024 07:07
.github/workflows/build.yml Outdated Show resolved Hide resolved
@onno-vos-dev
Copy link
Member

Hmm ok ofcourse 🤦 iso8601 PR (erlsci/iso8601#62) wasn't merged yet. I'll leave this PR open for about a week and will try and chase the maintainer of iso8601 on Erlanger Slack/Forum if not merged by then so we can open up OTP27 👍

@josevalim
Copy link
Contributor

josevalim commented May 29, 2024

Perhaps it is worth considering inlining the iso8601 parsing here? ISO8601 can be trivial to parse. Here is a minimal implementation:

-module(foo).
-export([parse/1]).
-define(is_digit(D), D1 >= $0, D1 =< $9).

parse([Y1, Y2, Y3, Y4, $-, O1, O2, $-, D1, D2, $T, H1, H2, $:, M1, M2, $:, S1, S2 | Rest])
    when Rest =:= ""; Rest =:= "Z" ->
  {d4(Y1, Y2, Y3, Y4), d2(O1, O2), d2(D1, D2), d2(H1, H2), d2(M1, M2), d2(S1, S2)}.

d4(D1, D2, D3, D4) when ?is_digit(D1), ?is_digit(D2), ?is_digit(D3), ?is_digit(D4) ->
  (D1 - $0) * 1000 + (D2 - $0) * 100 + (D3 - $0) * 10 + (D4 - $0).

d2(D1, D2) when ?is_digit(D1), ?is_digit(D2) ->
  (D1 - $0) * 10 + (D2 - $0).

It doesn't support offsets, only no offset or Z, but that's probably an ok assumption make?

1> foo:parse("2020-04-24T00:12:34Z").
{2020,4,24,0,12,34}

@dw-kihara
Copy link
Contributor Author

@josevalim

Perhaps it is worth considering inlining the iso8601 parsing here? ISO8601 can be trivial to parse.

Unfortunately, ISO8601 is not so trivial. There are both the basic format like 20240530T015432Z and the extended format like 2024-05-30T01:54:32Z. While the extended format is more common, the basic format is also valid and some AWS services use the basic format. Example.
(Moreover, some minor notations like 2024-160T12:30:48 and 2024-W22-3T12:30:48 are defined in ISO8601. I personally think such notations are unnecessary, but they are still defined.)

It doesn't support offsets, only no offset or Z, but that's probably an ok assumption make?

I think assuming all Expirations do not have time offsets is risky. Although AWS services seem to use Z, I can't find evidence that this behavior is guaranteed. Considering aws_credentials allows users to add custom providers, the likelihood of Expiration having a time offset will increase, as humans tend to use local times.

Therefore, I think it is better to depend on an ISO8601 library for now rather than parse it ourselves.

@onno-vos-dev
Copy link
Member

Force pushed with a rebase just to make sure. If the pipelines go through, I'll merge and release a new tag 👍

Thank you @dw-kihara ❤️

@onno-vos-dev onno-vos-dev self-requested a review June 12, 2024 15:43
@onno-vos-dev onno-vos-dev merged commit 61cdc9e into aws-beam:master Jun 12, 2024
9 checks passed
@onno-vos-dev
Copy link
Member

hex: 0.3.1 and github: 0.3.1 both releases available 👌

@dw-kihara dw-kihara deleted the feature/brushups_for_otp27 branch June 12, 2024 23:15
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.

Error when compiling with OTP 27
3 participants