search

vacp2p / nim-libp2p

libp2p implementation in Nim
https://vacp2p.github.io/nim-libp2p/docs/
MIT License
240 stars 52 forks source link

Update go-libp2p-daemon build version. #1111

Closed AlejandroCabeza closed 3 weeks ago

AlejandroCabeza commented 3 weeks ago

Fix build errors by updating the used version to a working one. Also, it's the one used in the CI for testing.

kaiserd commented 3 weeks ago

Thank you! Is there a reason why we do not used the tagged versions? v0.35.0 is only 14 days old. Seems to be recent enough. We could update the CI to that, too.

diegomrsantos commented 3 weeks ago

@AlejandroCabeza could please explain what build errors this is fixing?

@kaiserd If I remember correctly, I tried to use a newer version and it didn't work. It's not clear to me why this daemon is even necessary. I'd suggest removing it altogether.

AlejandroCabeza commented 3 weeks ago

Thank you! Is there a reason why we do not used the tagged versions? v0.35.0 is only 14 days old. Seems to be recent enough. We could update the CI to that, too.

@kaiserd I'm fine having a look to see whether we can update the daemon to a more recent version. In any case I'd merge this PR regardless, and take that as a follow-up task.

could please explain what build errors this is fixing?

@diegomrsantos Just build errors, I'm not a go expert so I can't elaborate. But it's as simple as running the build script as it is, you'll get those errors.

It's not clear to me why this daemon is even necessary. I'd suggest removing it altogether.

@diegomrsantos The p2pd binary is used for running tests. I'm positive that's mentioned in the README.

diegomrsantos commented 3 weeks ago

@AlejandroCabeza I meant when those build errors occur. As I've done this a long time ago, I don't remember. Using the current script to install it locally doesn't work?

I meant it's not clear to me why we need p2pd to run tests as the vast majority of them don't need it. I think it's required only for 4 or so. I suggest investigating it and either removing all those tests or removing the dependency on the daemon.

AlejandroCabeza commented 3 weeks ago

I meant when those build errors occur. As I've done this a long time ago, I don't remember. Using the current script to install it locally doesn't work?

@diegomrsantos Correct, running it fails due to some xerrors library (or something like that).

I meant it's not clear to me why we need p2pd to run tests as the vast majority of them don't need it. I think it's required only for 4 or so. I suggest investigating it and either removing all those tests or removing the dependency on the

@diegomrsantos No clue, but I think that's a different job altogether. If you think it's worth considering, would you mind writing an issue for it so we can discuss it during next weekly?

codecov-commenter commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.50%. Comparing base (02c96fc) to head (ec3c710). Report is 14 commits behind head on master.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1111/graphs/tree.svg?width=650&height=150&src=pr&token=M88zHaQffJ&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p)](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1111?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p) ```diff @@ Coverage Diff @@ ## master #1111 +/- ## ========================================== - Coverage 84.53% 84.50% -0.04% ========================================== Files 91 91 Lines 15517 15531 +14 ========================================== + Hits 13118 13124 +6 - Misses 2399 2407 +8 ``` [see 18 files with indirect coverage changes](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1111/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p)