celestiaorg / celestia-node

Celestia Data Availability Nodes
Apache License 2.0
932 stars 928 forks source link

[Feature Request]: Win compatibility. #2189

Closed HoytRen closed 6 months ago

HoytRen commented 1 year ago

Implementation ideas

Currently we can't build node for Win because the fslock module isn't compatible with Win.

I resolved this problem by importing a third party module. How do you think about this? Is that ok, or should I try to copy the logic without import third party modules since they seems not well maintained now?

HoytRen commented 1 year ago

Another problem is here is a key file permission check in libs/keystore/fs_keystore.go, that stop restarting of the node.

Wondertan commented 1 year ago

I resolved this problem by importing a third party module. How do you think about this? Is that ok, or should I try to copy the logic without import third party modules since they seems not well maintained now?

Let's see the PR. We can consider importing the dependency or even copying the code if it's unmaintained but small and with a supported license.

HoytRen commented 1 year ago

I need further tests and code review before PR, it just looks work currently. Since these are not well known and proved modules, I must be more careful.

HoytRen commented 1 year ago

I believe it's ready for BSR now, before I create the PR, which devnet I should target for the test?

Because windows package of go isn't completed, it lacks functions and is buggy, I finally give up checking if only the current user has access to the key file, but check if everyone has access. I don't want to import a lot of win32 api by myself, because I could make it buggier than the package due to lacking a test environment.

Because windows isn't a multi-user environment, I believe it's not that necessary for checking the key file access. I checked openssh's approach but unfortunately, it doesn't work for go. I even tried with AI code, but it just created a fake story but not real working code. Maybe somebody could improve this later.

I find an active package for the lock logic, and I feel it works well. https://github.com/danjacques/gofslock

Wondertan commented 1 year ago

which devnet I should target for the test?

If we test Windows compatibility I think it doesn't matter which test network to test against. As long we can compile, run and the node syncs, we are good.

HoytRen commented 1 year ago

I merged the latest commits (6654cdf4994dbd381efd0d6a29688c731177c855) from main and tested against arabica-8, but I can't run lint now.

I installed goimports-reviser by:

go get github.com/incu6us/goimports-reviser
go install github.com/incu6us/goimports-reviser

now I can run goimports-reviser but when I try make lint it reports an error flag provided but not defined: -list-diff I don't familiar with goimports-reviser, then how to fix this?

HoytRen commented 1 year ago

BTW, I can't get the test through even without modification. The nodebuilder/tests always failed. but my modifications don't add new errors, then I don't think it's my fault.

ramin commented 1 year ago

big thing to do here is test the linked PR and evaluate github.com/danjacques/gofslock dependency that it introduces

HoytRen commented 1 year ago

Yes, I tested the modification against arabica-10 and mocha-4. The new dependency is lively maintained. I read their code and don't see anything wrong. Please review, thanks.

ramin commented 6 months ago

@HoytRen I believe we can close this issue now since https://github.com/celestiaorg/celestia-node/pull/3253

HoytRen commented 6 months ago

@ramin sure😊