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

[feature] #3383: Implement a macro that parses a socket address at compile time #3675

Merged
merged 7 commits into from
Jul 6, 2023

Conversation

DCNick3
Copy link
Contributor

@DCNick3 DCNick3 commented Jul 4, 2023

Linked issue

Closes #3383

Limitations

This can't replace all uses of the existing socket_addr! macro, as it parses only literal addresses. Some of the socket_addr! usages use an expression as port number:

peer.p2p_address = socket_addr!(127,0,0,1; port);
. Supporting this is possible, but would require a more complicated parser.

As 5/11 usages of the original macro use it, it probably doesn't make sense to merge it without support for variable ports

It also doesn't support hostname addresses, but they are not used anywhere except in the doc-test for the socket_addr! macro. Probably fine going without it, but should be relatively easy to implement if needed.

Checklist

  • Parse IPv4 and IPv6 addresses
  • Allow usage of arbitrary expressions in port position
  • Replace old usages of socket_addr! macro

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Jul 4, 2023
@DCNick3 DCNick3 marked this pull request as draft July 4, 2023 11:53
@DCNick3 DCNick3 changed the title [WIP] Implement a macro that parses a socket address at compile time [feature] #3383: Implement a macro that parses a socket address at compile time Jul 4, 2023
Copy link
Contributor

@Arjentix Arjentix left a comment

Choose a reason for hiding this comment

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

Congratulations with your first PR! Nice!

In PR description you say that custom port is not supported. But by looking at the code it looks like it is supported, no?

Cargo.toml Show resolved Hide resolved
primitives/derive/src/lib.rs Outdated Show resolved Hide resolved
primitives/derive/src/socket.rs Outdated Show resolved Hide resolved
primitives/derive/src/socket.rs Outdated Show resolved Hide resolved
primitives/derive/src/socket.rs Outdated Show resolved Hide resolved
primitives/derive/src/socket.rs Outdated Show resolved Hide resolved
primitives/derive/src/socket.rs Outdated Show resolved Hide resolved
@DCNick3
Copy link
Contributor Author

DCNick3 commented Jul 4, 2023

In PR description you say that custom port is not supported. But by looking at the code it looks like it is supported, no?

I forgot the link to an example..

peer.p2p_address = socket_addr!(127,0,0,1; port);

Basically, it uses an arbirary expression instead of an integer literal for port. This won't work with the "stringify and parse it" approach

primitives/derive/Cargo.toml Outdated Show resolved Hide resolved
primitives/derive/src/lib.rs Outdated Show resolved Hide resolved
@Arjentix
Copy link
Contributor

Arjentix commented Jul 4, 2023

So what blocks you to undraft this PR?

@DCNick3
Copy link
Contributor Author

DCNick3 commented Jul 4, 2023

So what blocks you to undraft this PR?

Using expressions as port numbers, which is done a lot throughout the codebase & replacing the uses of declarative sock_addr!. Will probably tackle this tomorrow

Copy link
Contributor

@0x009922 0x009922 left a comment

Choose a reason for hiding this comment

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

LGTM. Some comments:

  • I wonder, which syntax would you use for expressions interpolation? I have a couple of ideas:

    // format-style?
    socket_addr!("127.0.0.1:{}", port);
    
    // something custom?
    socket_addr!(127.0.0.1:~port);
  • Will you replace all usages of the old macro?

@0x009922
Copy link
Contributor

0x009922 commented Jul 5, 2023

Also, please make meaningful checklist in the PR's description (the current one is template for external contributors, AFAIC). It might make sense to move some TODOs from your description/code there.

@DCNick3
Copy link
Contributor Author

DCNick3 commented Jul 5, 2023

I wonder, which syntax would you use for expressions interpolation? I have a couple of ideas:

I think it's possible to parse the natural syntax: socket_addr!(127.0.0.1:port). Everything before the colon token is converted to string and parsed as an IP address, while everything after the colon would be parsed as an expression.

Will you replace all usages of the old macro?

Yes, this is what I am planning to do

@DCNick3 DCNick3 marked this pull request as ready for review July 5, 2023 10:16
@coveralls
Copy link
Collaborator

coveralls commented Jul 5, 2023

Pull Request Test Coverage Report for Build 5473214665

  • 183 of 195 (93.85%) changed or added relevant lines in 3 files are covered.
  • 329 unchanged lines in 11 files lost coverage.
  • Overall coverage increased (+0.1%) to 59.532%

Changes Missing Coverage Covered Lines Changed/Added Lines %
core/test_network/src/lib.rs 0 4 0.0%
primitives/derive/src/socket_addr.rs 166 174 95.4%
Files with Coverage Reduction New Missed Lines %
data_model/src/transaction.rs 1 47.66%
wasm_codec/derive/src/lib.rs 1 90.71%
tools/kagami/src/main.rs 3 2.26%
cli/derive/src/lib.rs 5 93.2%
cli/src/samples.rs 5 0%
ffi/derive/src/util.rs 6 94.34%
version/derive/src/lib.rs 11 91.44%
config/src/client.rs 13 70.42%
data_model/src/block.rs 30 42.21%
wasm_builder/src/lib.rs 95 41.67%
Totals Coverage Status
Change from base Build 5423219773: 0.1%
Covered Lines: 19889
Relevant Lines: 33409

💛 - Coveralls

@DCNick3
Copy link
Contributor Author

DCNick3 commented Jul 5, 2023

rustfmt wants socket_addr!(127.0.0.1: port); to be formatted as socket_addr!(127.0.0 .1: port);...

There is an option to exclude a certain macro invocations from being formatted, but

  1. the config option parsing seems broken skip_macro_invocations: Cannot configure from rustfmt.toml rust-lang/rustfmt#5816
  2. the toolchain used in CI doesn't support it

I guess we would have to live with this ugly formatting for some time?

…dress at compile time

Signed-off-by: Nikita Strygin <DCNick3@users.noreply.github.com>
- rename macro to socket_addr (for consistency with already existing decl macro)
- more backticks
- fix `SocketAddr` variant names (whoops)
- re-export the macro from `iroha_primitives` under a temporary name

Signed-off-by: Nikita Strygin <DCNick3@users.noreply.github.com>
- keep the "stringify and parse" approach for the IP part, but parse the rest using syn's parsing framework
- special-case brackets: they are used to wrap the IPv6 addresses
- use colon as a terminator for IPv4 address

Signed-off-by: Nikita Strygin <DCNick3@users.noreply.github.com>
…the new syntax

Signed-off-by: Nikita Strygin <DCNick3@users.noreply.github.com>
Signed-off-by: Nikita Strygin <DCNick3@users.noreply.github.com>
Signed-off-by: Nikita Strygin <DCNick3@users.noreply.github.com>
…hate it)

`rustfmt` wants `socket_addr!(127.0.0.1: port);` to be formatted as `socket_addr!(127.0.0 .1: port);`...

There is an option to exclude a certain macro invocations from being formatted, but

1) the config option parsing seems broken rust-lang/rustfmt#5816
2) the toolchain used in CI doesn't support it

I guess we would have to live with this ugly formatting for some time...

Signed-off-by: Nikita Strygin <DCNick3@users.noreply.github.com>
@DCNick3 DCNick3 merged commit f84599b into hyperledger:iroha2-dev Jul 6, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants