search

c-blake / cligen

Nim library to infer/generate command-line-interfaces / option / argument parsing; Docs at
https://c-blake.github.io/cligen/
ISC License
501 stars 23 forks source link

cligen break nim important_packages #170

Closed timotheecour closed 3 years ago

timotheecour commented 3 years ago

https://github.com/nim-lang/Nim/pull/15695/checks?check_run_id=1299438437

PASS: status-im/nim-chronicles C                (27.06 sec)
  FAIL: c-blake/cligen.git C
  Test "c-blake/cligen.git" in category "nimble-packages-1"
  Failure: reBuildFailed
  package test failed
  $ nim c -o:cligenn -r cligen.nim
  Hint: used config file '/Users/runner/work/Nim/Nim/config/nim.cfg' [Conf]
  Hint: used config file '/Users/runner/work/Nim/Nim/config/config.nims' [Conf]
  ..............................
  /Users/runner/work/Nim/Nim/pkgstemp/cligen/cligen/mslice.nim(8, 14) Error: cannot open file: cligen/prefetch

  PASS: PMunch/combparser C                       ( 2.55 sec)

wouldn't cligen's own CI prevent this?

c-blake commented 3 years ago

There is no actual bug other than some kind of staleness thing going on in the nim CI. I added a new module to the git VC head in the very same git commit as adding that import for mslice. It works fine locally.

Somehow the nim CI is partially updating mslice (presumably via git pull maybe indirectly via some broken nimble thing since I have not punched a new release) but did not pull the new module. Shrug. If it will fix the borked Nim CI, I can punch a new cligen release. Personally, it seems a Bad Property not under my own control is afoot here.

c-blake commented 3 years ago

It does not only work locally on merely nim-devel either..but on my whole usual panel of nim-(devel, 1.4, 1.2, 1.0, 0.20.2, 0.19.2).

timotheecour commented 3 years ago

I'm wondering if it's https://github.com/timotheecour/Nim/issues/167 all over again; I thought https://github.com/nim-lang/Nim/pull/14770 was supposed to fix that? /cc @Clyybber

c-blake commented 3 years ago

I am not sure how publicly visible this is, but I can also see all recent commits passing with green checks (under my Actions tab) which runs sh test.sh in the cligen CI, far more extensive (though yes, still incomplete/only help messages) than nim c -o:cligenn cligen. I'm not really in a mood to debug the Nim CI or nimble, but am happy to do whatever you tell me to in order to unblock packages testing.

c-blake commented 3 years ago

Personally, if it is that test-against-different-than-install then it should really be fixed. Otherwise no one can add anything new without punching releases under pretty heavy constraints which is bad, not good, for the package ecosystem, but then I think that is also true of nimble's src/ defaults. Maybe the Nim CI should just bypass nimble completely? My nimp is like 300 lines of code. It just does '#head' all the way down which would test the transitive closure of all VC heads. (Not sure if that is better or worse..pros & cons.)

Anyway, let me know if you want me to punch some patch release or if you want to debug the broken CI "as-is".

timotheecour commented 3 years ago

would be good to have a reproducing bug though, if you punch a patch release the problem will once again disappear under the rug without underyling issue fixed in nim CI; maybe disabling cligen in important_packages might be a better fix (temporarily ) so we can always come back to that broken state by uncommenting it

c-blake commented 3 years ago

I guess it's up to @Clyybber what he wants to do, if he's going to work on that again. It's no big deal to me to either punch a release, nor be un-auto-tested-against-Nim-core-shenanigans for a while. Let us know, @Clyybber.

Clyybber commented 3 years ago

Hmm, sorry I don't know exactly what caused this issue, possibly a nimble bug? I created https://github.com/nim-lang/Nim/pull/15701, which should make only cligen's latest release get tested until I fix the root cause (I have a suspicion its the combination of nimble pkgs having cligen as a dependency and us cloning cligen but not doing nimble develop, I'll follow up with a proper fix tomorrow hopefully). Even if this doesn't fix it, its clear that its not an issue on your side, so I this can be closed

timotheecour commented 3 years ago

@Clyybber indeed, that's what I observed in https://github.com/timotheecour/Nim/issues/167

the bug is that there's a mismatch between what's installed by nimble (latest git tag, or perhaps another version if installed already by another package [that's another bug! should be isolated from other packages and not order dependant]) and what's tested via nimble test or the test cmd provided (which uses git HEAD)

can you try adding nimble develop ?

c-blake commented 3 years ago

Closing as not really my issue. Leave a comment if you want me to punch a release.

Clyybber commented 3 years ago

Just a small update, this can be reproduced by fresh cloning cligen and running nim c -r cligen.nim; the failure stems from import cligen/prefetch referencing the cligen package itself, and since we didn't use nimble develop on the package this didn't work.

c-blake commented 3 years ago

Well, I did an import ./prefetch. Seems like that is how various other modules import. So, there is a good chance this fixes it.

cligen.nim itself does do a big import cligen/[...], but the new prefetch is not in that. So, that import along with various other cligen/ references may be immune to version skew problems.

c-blake commented 3 years ago

FWIW, I guess a cligen.nim.cfg containing path = "." could also have fixed this. (EDIT: I am not sure if that is a general substitute for nimble develop. Maybe. Unless the package itself already has such a .cfg, of course.)

Clyybber commented 3 years ago

Yeah, I fixed this issue by making CI use --path:. for cligen.

c-blake commented 3 years ago

Well, fixed two ways now, then..one for the old CI and one forward looking to similar future import mistakes that I may make. ;-) I may even add that cligen.nim.cfg, fixing it 3 ways.

I do think once cligen is installed in any ordinary way (including just git clone + add to path like nimp) that the import would be resolved.

So, this could maybe also impact other packages in some testing-outside-the-usual-install-rules sense. I.e., maybe all packages (or all packages with a pkgname/ subdir) should get that --path=., not merely cligen. I haven't gone through the list to see which might need it.

timotheecour commented 3 years ago

this is exactly why we need https://github.com/nim-lang/RFCs/issues/267

import ./prefetch is the best we can do without https://github.com/nim-lang/RFCs/issues/267 but it has its own issues. IIRC it's treated like import prefetch, and this is obviously brittle since it brings the whole world in the same scope (eg if there's a prefetch nimble package and prefetch file moves); the main problem is it brings everything on the same scope, and we have no idea whether it refers to an external or internal import without knowing the installed nimble packages, the local filesystem and --path