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

tcp: fix build on riscv64 #2590

Merged
merged 2 commits into from
Oct 17, 2023
Merged

tcp: fix build on riscv64 #2590

merged 2 commits into from
Oct 17, 2023

Conversation

marten-seemann
Copy link
Contributor

No description provided.

@marten-seemann marten-seemann requested a review from Jorropo October 1, 2023 15:47
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

Can you add comments linking to marten-seemann/tcp#1 there is no reason this can't work on riscv, just we need theses constants defined.

@@ -1,4 +1,4 @@
//go:build !windows
//go:build !windows && !riscv64
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//go:build !windows && !riscv64
// riscv64 see: https://github.com/marten-seemann/tcp/pull/1
//go:build !windows && !riscv64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure this works? Doesn't the go:build need to come first?

Copy link
Contributor

@Jorropo Jorropo Oct 7, 2023

Choose a reason for hiding this comment

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

I don't think so, go:build line has to come before package:

> cat a.go 
// test

//go:build doNotBuild

package aaa

const "test" + 42
> cat b.go 
package aaa

func A() {}
> go build .
> echo $status
0

@@ -1,4 +1,4 @@
//go:build !linux && !darwin && !windows
//go:build !linux && !darwin && !windows && !riscv64
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//go:build !linux && !darwin && !windows && !riscv64
// riscv64 see: https://github.com/marten-seemann/tcp/pull/1
//go:build !linux && !darwin && !windows && !riscv64

p2p/transport/tcp/metrics_none.go Show resolved Hide resolved
@Jorropo
Copy link
Contributor

Jorropo commented Oct 11, 2023

Could this be included in v0.31.1 ?

@marten-seemann
Copy link
Contributor Author

Could this be included in v0.31.1 ?

@Jorropo Can you elaborate on the sudden urgency please? The Kubo issue has been open for years.

@Jorropo
Copy link
Contributor

Jorropo commented Oct 12, 2023

I'm now running Kubo on some riscv64 hardware and I'm lazy, so I would rather not have to repatch it every time but that is subjective.
We now have a patch that you are not in-comfortable merging due to the extra maintenance it would bring and it is free given it's stubbing statistics values to be 0 and the stubs must exists anyway for the windows platform which you want to support.

Imo the best time to fix this would have been fixed in december 2021 or january 2022 shortly after marten-seemann/tcp#1 was sent but it didn't happend for reasons unknown to me and I don't have a time machine to do anything about it, so the next best time to do it is now.

I can wait for v0.32.0 if that is your question.

@marten-seemann
Copy link
Contributor Author

This doesn't seem like a kind of patch that justifies cutting a patch release. This will be included in v0.32.

@Jorropo
Copy link
Contributor

Jorropo commented Oct 12, 2023

I miss-understood there would be a v0.31.1 for the resource manager desync issue.

Co-authored-by: Jorropo <jorropo.pgm@gmail.com>
@marten-seemann marten-seemann mentioned this pull request Oct 17, 2023
25 tasks
@marten-seemann
Copy link
Contributor Author

I added one of the comments for the build flags. I don't think we need to repeat in 3 files.

@marten-seemann marten-seemann merged commit 99f7611 into master Oct 17, 2023
11 checks passed
sukunrt pushed a commit that referenced this pull request Oct 18, 2023
* tcp: fix build on riscv64

* explain why riscv64 doesn't work

Co-authored-by: Jorropo <jorropo.pgm@gmail.com>

---------

Co-authored-by: Jorropo <jorropo.pgm@gmail.com>
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