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.

Sign up for GitHub

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

Jump to bottom

chore: introduce libp2p-test-utils #5725

Merged
merged 20 commits into from
Dec 26, 2024
Merged

chore: introduce libp2p-test-utils #5725

merged 20 commits into from
Dec 26, 2024

Conversation

kamuik16
Copy link
Contributor

@kamuik16 kamuik16 commented Dec 8, 2024

Description

Fixes #4992

Based on the conversation in #4992.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@drHuangMHT drHuangMHT mentioned this pull request Dec 9, 2024
Closed
4 tasks
jxs
jxs reviewed Dec 9, 2024
View reviewed changes
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Thanks for this.
Overall looks good to me, left some minor comments. We also need CHANGELOG.md entries.
I think when Thomas mentioned the publish = false wasn't accounting what he said later on #4992 (comment)

misc/logging/Cargo.toml Outdated Show resolved Hide resolved
misc/logging/src/lib.rs Outdated Show resolved Hide resolved
@jxs jxs mentioned this pull request Dec 9, 2024
Merged
4 tasks
@kamuik16
Copy link
Contributor Author

kamuik16 commented Dec 9, 2024

Thanks for this. Overall looks good to me, left some minor comments. We also need CHANGELOG.md entries. I think when Thomas mentioned the publish = false wasn't accounting what he said later on #4992 (comment)

We also need CHANGELOG.md entries.

Which ones do you suggest? Since the logging is only used in testing part and is not the part of public API, I don't think it needs CHANGELOG.md entries.

@jxs
Copy link
Member

jxs commented Dec 9, 2024

Thanks for this. Overall looks good to me, left some minor comments. We also need CHANGELOG.md entries. I think when Thomas mentioned the publish = false wasn't accounting what he said later on #4992 (comment)

We also need CHANGELOG.md entries.

Which ones do you suggest? Since the logging is only used in testing part and is not the part of public API, I don't think it needs CHANGELOG.md entries.

the ones we publish, libp2p-perf and libp2p-server

@kamuik16
Copy link
Contributor Author

Thanks for this. Overall looks good to me, left some minor comments. We also need CHANGELOG.md entries. I think when Thomas mentioned the publish = false wasn't accounting what he said later on #4992 (comment)

We also need CHANGELOG.md entries.

Which ones do you suggest? Since the logging is only used in testing part and is not the part of public API, I don't think it needs CHANGELOG.md entries.

the ones we publish, libp2p-perf and libp2p-server

added CHANGELOG.md entries.

@kamuik16
Copy link
Contributor Author

@jxs would appreciate your further review here :)

@kamuik16 kamuik16 requested a review from jxs December 13, 2024 04:24
@kamuik16
Copy link
Contributor Author

semver check is just failing because the libp2p-logging crate has not been published yet.

jxs
jxs approved these changes Dec 13, 2024
View reviewed changes
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Krishang, thanks for the ping!
Giving more thought about this one taking into account we'll have to release it and be yet another crate to maintain in exchange for duplicating a couple lines across the repo it's not clear its value, but I am willing to move forward, wdyt @elenaf9 @dariusc93 @guillaumemichel @drHuangMHT

@kamuik16
Copy link
Contributor Author

kamuik16 commented Dec 13, 2024 •
edited
Loading

LGTM, thanks Krishang, thanks for the ping! Giving more thought about this one taking into account we'll have to release it and be yet another crate to maintain in exchange for duplicating a couple lines across the repo it's not clear its value, but I am willing to move forward, wdyt @elenaf9 @dariusc93 @guillaumemichel @drHuangMHT

yeah, that's why I was suggesting to keep the publish = false and remove its usage from the packages that are released.

Majority of the usage of this is in the tests, so no need to publish it.

@jxs
Copy link
Member

jxs commented Dec 13, 2024

LGTM, thanks Krishang, thanks for the ping! Giving more thought about this one taking into account we'll have to release it and be yet another crate to maintain in exchange for duplicating a couple lines across the repo it's not clear its value, but I am willing to move forward, wdyt @elenaf9 @dariusc93 @guillaumemichel @drHuangMHT

yeah, that's why I was suggesting to keep the publish = false and remove its usage from the packages that are released.

Majority of the usage of this is in the tests, so no need to publish it.

but then we still need to have duplicated log initialization code on the published crates (libp2p-perf and libp2p-server) right? Or how would you address it? Nonetheless thanks for your patience bearing with the process Krishang.

elenaf9
elenaf9 reviewed Dec 14, 2024
View reviewed changes
Copy link
Contributor

@elenaf9 elenaf9 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

yeah, that's why I was suggesting to keep the publish = false and remove its usage from the packages that are released.

Majority of the usage of this is in the tests, so no need to publish it.

I am also leaning towards not publishing libp2p-logging, and instead duplicating the lines in the two published crates.
It's not ideal, but it still:

  • avoids the verbose let _ = tracing_subscriber::fmt()... in all other tests/ examples
  • doesn't require us to maintain yet another published crate.

misc/logging/src/lib.rs Outdated Show resolved Hide resolved
misc/logging/src/lib.rs Outdated Show resolved Hide resolved
misc/logging/src/lib.rs Outdated Show resolved Hide resolved
@kamuik16
Copy link
Contributor Author

yeah, that's why I was suggesting to keep the publish = false and remove its usage from the packages that are released.
Majority of the usage of this is in the tests, so no need to publish it.

I am also leaning towards not publishing libp2p-logging, and instead duplicating the lines in the two published crates. It's not ideal, but it still:

  • avoids the verbose let _ = tracing_subscriber::fmt()... in all other tests/ examples
  • doesn't require us to maintain yet another published crate.

yep, I do agree on this, it's better to have 2-3 lines of duplicated code rather than maintaining a whole crate.

I have reverted back the changes on the published crates and made libp2p-logging with publish = false.

elenaf9
elenaf9 reviewed Dec 14, 2024
View reviewed changes
Copy link
Contributor

@elenaf9 elenaf9 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the follow-up @kamuik16!

I just noticed that the new libp2p_logging crate is not used by any crates in examples/. Was that on purpose / is there a reason why it can't be used there?

misc/logging/CHANGELOG.md Outdated Show resolved Hide resolved
misc/multistream-select/src/dialer_select.rs Outdated Show resolved Hide resolved
protocols/stream/tests/lib.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Outdated Show resolved Hide resolved
misc/logging/src/lib.rs Outdated Show resolved Hide resolved
@kamuik16
Copy link
Contributor Author

kamuik16 commented Dec 14, 2024 •
edited
Loading

Thanks for the follow-up @kamuik16!

I just noticed that the new libp2p_logging crate is not used by any crates in examples/. Was that on purpose / is there a reason why it can't be used there?

yeah, that was on purpose, examples are meant for user to copy paste and run. If we don't publish the crate, examples would compile in our repository but users who are still new and just want to copy paste and run them, won't be able to do so.

does this make sense?

@elenaf9 elenaf9 mentioned this pull request Dec 14, 2024
Merged
4 tasks
@kamuik16 kamuik16 requested review from elenaf9 and jxs December 16, 2024 11:47
@elenaf9
Copy link
Contributor

elenaf9 commented Dec 17, 2024

yeah, that was on purpose, examples are meant for user to copy paste and run. If we don't publish the crate, examples would compile in our repository but users who are still new and just want to copy paste and run them, won't be able to do so.

does this make sense?

Yes, makes sense, although it's a bit unfortunate. I didn't think about that before and based on the discussion in #4992 assumed we would be using it for both tests and examples. My bad.

I would still go ahead with the current PR. Wdyt @jxs?

Another thought I just had is that if this is only used in tests, we could consider generalizing libp2p-logging into test-utils / test-common or sth like that, so next time we have another function or logic that is commonly reused in tests we can just add it there.

@kamuik16
Copy link
Contributor Author

yeah, that was on purpose, examples are meant for user to copy paste and run. If we don't publish the crate, examples would compile in our repository but users who are still new and just want to copy paste and run them, won't be able to do so.
does this make sense?

Yes, makes sense, although it's a bit unfortunate. I didn't think about that before and based on the discussion in #4992 assumed we would be using it for both tests and examples. My bad.

I would still go ahead with the current PR. Wdyt @jxs?

Another thought I just had is that if this is only used in tests, we could consider generalizing libp2p-logging into test-utils / test-common or sth like that, so next time we have another function or logic that is commonly reused in tests we can just add it there.

seems reasonable

@jxs
Copy link
Member

jxs commented Dec 18, 2024

yeah, that was on purpose, examples are meant for user to copy paste and run. If we don't publish the crate, examples would compile in our repository but users who are still new and just want to copy paste and run them, won't be able to do so.
does this make sense?

Yes, makes sense, although it's a bit unfortunate. I didn't think about that before and based on the discussion in #4992 assumed we would be using it for both tests and examples. My bad.

I would still go ahead with the current PR. Wdyt @jxs?

Another thought I just had is that if this is only used in tests, we could consider generalizing libp2p-logging into test-utils / test-common or sth like that, so next time we have another function or logic that is commonly reused in tests we can just add it there.

yeah makes sense to me, can we then update this PR Krishang and do it? Again, thanks for the patience! ❤️

@kamuik16
Copy link
Contributor Author

yeah, that was on purpose, examples are meant for user to copy paste and run. If we don't publish the crate, examples would compile in our repository but users who are still new and just want to copy paste and run them, won't be able to do so.
does this make sense?

Yes, makes sense, although it's a bit unfortunate. I didn't think about that before and based on the discussion in #4992 assumed we would be using it for both tests and examples. My bad.
I would still go ahead with the current PR. Wdyt @jxs?
Another thought I just had is that if this is only used in tests, we could consider generalizing libp2p-logging into test-utils / test-common or sth like that, so next time we have another function or logic that is commonly reused in tests we can just add it there.

yeah makes sense to me, can we then update this PR Krishang and do it? Again, thanks for the patience! ❤️

Hey @jxs, no worries. I updated the PR to move the libp2p-logging to libp2p-test-utils. Also changing the PR title now.

@kamuik16 kamuik16 changed the title chore: introduce libp2p-logging chore: introduce libp2p-test-utils Dec 18, 2024
elenaf9
elenaf9 approved these changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

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

Thank you for your patience @kamuik16! LGTM

jxs
jxs approved these changes Dec 25, 2024
View reviewed changes
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Thanks Krishang! LGTM Can you do one last chore and address the merge conflicts? 😁

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 25, 2024

This pull request has merge conflicts. Could you please resolve them @kamuik16? 🙏

@kamuik16
Copy link
Contributor Author

Thanks Krishang! LGTM Can you do one last chore and address the merge conflicts? 😁

resolved @jxs 😄

@jxs jxs added the send-it label Dec 26, 2024
@mergify mergify bot merged commit 644d7d0 into libp2p:master Dec 26, 2024
71 checks passed
@jxs
Copy link
Member

jxs commented Dec 26, 2024

Thanks!

@kamuik16 kamuik16 deleted the libp2p-logging branch December 27, 2024 05:14
jxs pushed a commit to jxs/rust-libp2p that referenced this pull request Jan 6, 2025
@kamuik16 @jxs
chore: introduce libp2p-test-utils
e8a97ec 
Fixes libp2p#4992

Based on the conversation in libp2p#4992.

Pull-Request: libp2p#5725.
jxs added a commit to jxs/rust-libp2p that referenced this pull request Jan 13, 2025
@jxs
chore(test-utils): revert libp2p#5725, with it we cannot publish the …
af07278 
…crates using test-utils
mergify bot pushed a commit that referenced this pull request Jan 13, 2025
@jxs
chore(test-utils): revert #5725
0177ddf 
We cannot publish the crates using `libp2p-test-utils`, as cargo seems to required `dev-dependencies` to also be published

Pull-Request: #5810.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elenaf9 elenaf9 elenaf9 approved these changes

@dariusc93 dariusc93 dariusc93 approved these changes

@jxs jxs jxs approved these changes

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tracing_subscriber should log to stderr
4 participants
@kamuik16 @jxs @elenaf9 @dariusc93