search

tahoe-lafs / zfec

zfec -- an efficient, portable erasure coding tool
Other
373 stars 44 forks source link

Remove `unsafePerformIO` #84

Open exarkun opened 1 year ago

exarkun commented 1 year ago

It's not clear that these uses are safe and it doesn't seem like much of a hardship to just drop them and force the caller to operate in IO.

sajith commented 9 months ago

Looks like a nice change, but I'm merely eyeballing. :-)

It would be nice to have this checked by CI, and update documentation with the correct steps for running Haskell tests because simply running runhaskell haskell/test/FECTest.hs as mentioned in the README does not work.

sajith commented 9 months ago

Well, the README has some notes about installing the library, but it is both outdated (because fec.cabal has a test suite definition now, so cabal test is what should be recommended) and incorrect (because cabal install-ing a library globally based on vague wording is likely to turn out to be a bad idea).

Can we also add some README updates to this PR?

sajith commented 9 months ago

Incidentally, https://hackage.haskell.org/package/fec is bothersome too. The package there is outdated, and there's just one maintainer. I'm guessing that it doesn't bother Nix people so much. :-)

(Also, the Haskell package probably should be called zfec to be more accurate, not fec? But then the module is Codec.FEC...)

exarkun commented 9 months ago

Looks like a nice change, but I'm merely eyeballing. :-)

It would be nice to have this checked by CI, and update documentation with the correct steps for running Haskell tests because simply running runhaskell haskell/test/FECTest.hs as mentioned in the README does not work.

I think those are good ideas. Since I seem to have discovered the cause of the corrupt results (#89 / #91) I don't know if I'll push forward this change much more in the near future but I will try to put some CI / documentation improvements into another PR.

exarkun commented 9 months ago

Looks like a nice change, but I'm merely eyeballing. :-)

It would be nice to have this checked by CI, and update documentation with the correct steps for running Haskell tests because simply running runhaskell haskell/test/FECTest.hs as mentioned in the README does not work.

Ah - CI does run the Haskell tests already, eg https://app.circleci.com/pipelines/github/tahoe-lafs/zfec/5/workflows/e1f141a4-cc88-49eb-aafb-f43d76171448/jobs/5

sajith commented 9 months ago

Ah, I wasn't paying attention. It doesn't seem to run cabal test though, or does it?

sajith commented 9 months ago

Also, it doesn't seem that CircleCI status checks appear on GitHub PRs?

exarkun commented 9 months ago

Ah, I wasn't paying attention. It doesn't seem to run cabal test though, or does it?

Indirectly it does - but it's true that it drives that process with nix build. (This builds the library and, perhaps surprisingly, runs the test suite.

Also, it doesn't seem that CircleCI status checks appear on GitHub PRs?

They do but it looks like CircleCI was actually set up after this PR was created so it didn't show up on this PR's CI runs.