nim-lang / RFCs

A repository for your Nim proposals.
136 stars 26 forks source link

transfer cheatfate/asynctools in nim-lang/asynctools? #393

Closed timotheecour closed 3 years ago

timotheecour commented 3 years ago

https://github.com/cheatfate/asynctools seems un-maintained but is used by a number of packages (transitively or directly), can we transfer it to nim-lang (using same approach as https://github.com/nim-lang/RFCs/issues/365 for nimlime) ? /cc @cheatfate

I think this repo is not actively maintained anymore although it's really good work. I also got a few bugs fixed but I don't think I would submit any PR. I hope someone can fork it and take care of it so I can contribute over there.

requires "https://github.com/timotheecour/asynctools#pr_fix_compilation"


and it gets worse: see https://github.com/dom96/httpbeast/pull/26#issuecomment-502836890
and here, which uses another fork for httpbeast:

httpbeast.nimble:

Test dependencies

When https://github.com/cheatfate/asynctools/pull/28 is fixed,

change this back to normal asynctools

requires "https://github.com/iffy/asynctools#pr_fix_for_latest"

Araq commented 3 years ago

I doubt we can transfer it to nim-lang, as we don't have the manpower to maintain it over there either.

timotheecour commented 3 years ago

I expect maintenance burden to be: review PRs from external contributors merge PRS

the above situation is pretty bad, but alternative suggestions welcome, for cases like this where original maintainer has moved on and allowing more than 1 people to merge increases robustness against project abandonment

Araq commented 3 years ago

You're supposed to use https://github.com/status-im/nim-chronos/blob/master/chronos/asyncsync.nim instead. It is the more recent version of asynctools.

Araq commented 3 years ago

I now have write access and I am helping Cheatfate. Closing.

dom96 commented 3 years ago

You're supposed to use https://github.com/status-im/nim-chronos/blob/master/chronos/asyncsync.nim instead. It is the more recent version of asynctools.

It depends on chronos though, doesn't it?

Since you have write access now I assume you'll help merge any PRs that are necessary.

Araq commented 3 years ago

Correct.

bung87 commented 3 years ago

I dont understand , asynctools self contained asyncsync.nim already ?

timotheecour commented 3 years ago

I dont understand , asynctools self contained asyncsync.nim already ?

what do you mean?

the whole point of this issue was to improve the situation I described in top post; if @Araq can merge PRs in the backlog, so that other packages (eg jester, httpbeast etc) can use requires "asynctools" again instead of these:

requires "https://github.com/timotheecour/asynctools#pr_fix_compilation"
requires "https://github.com/iffy/asynctools#pr_fix_for_latest"

then the problem is solved

bung87 commented 3 years ago

You're supposed to use https://github.com/status-im/nim-chronos/blob/master/chronos/asyncsync.nim instead. It is the more recent version of asynctools.

I reply to araq's comment.

timotheecour commented 3 years ago

I've sent out https://github.com/dom96/jester/pull/279 and https://github.com/dom96/httpbeast/pull/49; what else needs to be updated? is there a nimble command to figure out dependencies on asynctools?

dom96 commented 3 years ago

Nope, we don't have reverse deps tracking anywhere (unless nimble.directory implements this, but I'm not aware that it does)

timotheecour commented 3 years ago

Nope, we don't have reverse deps tracking anywhere (unless nimble.directory implements this, but I'm not aware that it does)

actually a local tool would be good enough (and probably better because more flexible); seems like it's not hard to implement on top of nimble dump --json regex, eg:

nimble dump --json regex | jq .requires
[
  {
    "name": "nim",
    "str": ">= 1.0.0",
    "ver": {
      "kind": "verEqLater",
      "ver": "1.0.0"
    }
  },
  {
    "name": "unicodedb",
    "str": ">= 0.7.2",
    "ver": {
      "kind": "verEqLater",
      "ver": "0.7.2"
    }
  }
]

nimble could grow a nimble query <query params> or (easier to implement, more flexible and good enough) and API that can be called via: proc query(param: JSonNode): JsonNode to allow maximum flexibility

bung87 commented 3 years ago

@timotheecour @haxscramper already done something like that. https://github.com/nim-lang/nimble/issues/890#issuecomment-803595931

timotheecour commented 3 years ago

great; so the next step would be to either integrate into nimble, or make it available as a nimble package /cc @haxscramper (unless it already is, i couldn't tell from linked discussion?)

haxscramper commented 3 years ago

I will add this to my upcoming nimble RFC, and this topic was partially discussed yesterday on the discord chat. Right now I could say that I had to write a PNode-based manifest parser since nimble dump was kind slow and fragile for older packages. I have cleaned up parser I used for comment in #890 and it is now a part hnimast package, but it would be pretty easy to adopt. @bung87 have already used it to parse package info for their nimble fork I believe.

Note that I'm currently not up to discussion about this, and it looks like slightly off-topic for the current issue anyway.