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

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

Closed
timotheecour opened this issue Jun 27, 2021 · 15 comments
Closed

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

timotheecour opened this issue Jun 27, 2021 · 15 comments

Comments

@timotheecour
Copy link
Member

timotheecour commented Jun 27, 2021

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 #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.

  • the workarounds are ugly, eg because of those stale PRs, jester depends on my private fork of a PR, which is fragile:
# jester.nimble:

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

and it gets worse: see dom96/httpbeast#26 (comment)
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
Copy link
Member

Araq commented Jun 27, 2021

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

@timotheecour
Copy link
Member Author

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
Copy link
Member

Araq commented Jun 28, 2021

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
Copy link
Member

Araq commented Jun 28, 2021

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

@Araq Araq closed this as completed Jun 28, 2021
@dom96
Copy link
Contributor

dom96 commented Jun 28, 2021

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
Copy link
Member

Araq commented Jun 28, 2021

Correct.

@bung87
Copy link

bung87 commented Jun 28, 2021

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

@timotheecour
Copy link
Member Author

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
Copy link

bung87 commented Jun 28, 2021

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
Copy link
Member Author

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

@dom96
Copy link
Contributor

dom96 commented Jun 29, 2021

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

@timotheecour
Copy link
Member Author

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
Copy link

bung87 commented Jun 29, 2021

@timotheecour @haxscramper already done something like that. nim-lang/nimble#890 (comment)

@timotheecour
Copy link
Member Author

timotheecour commented Jun 29, 2021

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
Copy link

haxscramper commented Jun 29, 2021

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.

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

No branches or pull requests

5 participants