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

[WIP] fix https://github.com/nitely/nim-regex/issues/60 #13868

Closed
wants to merge 2 commits into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Apr 4, 2020

[DO NOT REVIEW YET]

fix nitely/nim-regex#60

nim-regex (after an awesome performance update) is now using import pkg/foo instead of import foo which broke nim CI because it was using nim c src/regex and for some reason src isn't in the path (maybe because the logic for important_packages doesn't install foo ?)

EDIT: doesn't work yet... but i've now found the root cause and it's a bug in important_packages logic, not in regex package:
nimble install regex installs the latest release tag (13.1), as it should (we should only test latest releases, HEAD would be too unstable)
but then git clone regex clones HEAD (which is different from latest release tag), and it fails with import pkg/foo since that refers to the installed regex, not the cloned one.

the fix

either use nimble develop after the clone (but that has the drawback of testing HEAD instead of latest release), or (preferred), after git clone we must git reset --hard rev where rev is the revision used in nimble install

there are other (preexisting) issues though:
if nimble install bar installs foo at a specific version, (say regex at 10.0) that is different from the one implied by nimble install foo, it can cause issues

the obvious short term workaround is to disable regex, but the underlying issue is still there and will keep popping up unless it is fixed

EDIT

upstream regex was updated to remove import pkg/foo so the bug disappeared but underlying issue is still there.
if we're disallowing import pkg/foo (btw we should have a linter to issue warnings when this is detected), that means we're forced to use import foo which can cause ambiguities; maybe what we need is:

import thispkg/foo to refer to current nimble package (implementation is easy, built on top of getNimbleFile from #13223). I will explain this in more details later.

@timotheecour timotheecour changed the title [WIP] fix https://github.com/nitely/nim-regex/issues/60 fix https://github.com/nitely/nim-regex/issues/60 Apr 4, 2020
@timotheecour timotheecour changed the title fix https://github.com/nitely/nim-regex/issues/60 [WIP] fix https://github.com/nitely/nim-regex/issues/60 Apr 4, 2020
@Araq
Copy link
Member

Araq commented Apr 5, 2020

Once again the root cause is Nimble with it's stupid $nimbleDir.

@genotrance
Copy link
Contributor

Every package should be testable without installation. Especially important packages should have a cfg with --path:src or equivalent if not already.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 24, 2020

@Araq

Once again the root cause is Nimble with it's stupid $nimbleDir.

That's not the root cause, and NIMBLE_DIR is essential for sandboxing in particular on user's machine eg export NIMBLE_DIR=$HOME/.nimble_alt && nimble install foo && nimble test

The root cause is what I described above or here timotheecour#167, and keeps breaking important_packages, as just happened again today with inim (inim-repl/INim#74), see my explanation here: inim-repl/INim#74 (comment)

  • important_packages.nim does this:
  • nimble install inim => this installs inim at latest release tag
  • git clone inim https://github.com/inim-repl/INim && cd INim && nimble test => we've installed inim at latest release tag but test inim at HEAD, which is obviously a bug: latest commit added requires "https://github.com/PMunch/ansiparse" so depends on ansiparse, but that package won't be installed by nimble install since that dependency wasn't added after latest release tag. Or maybe that dependency is there, for totally unrelated ephemeral reasons, because some other important_package installed it.

the short term fix is simple:
after git clone foo, we need to also do git checkout rev where rev is whatever revision got installed by nimble install foo

@genotrance

Every package should be testable without installation.

that can't work if package foo depends on bar.
What should work is nimble test foo where installation still happens, but under-the-scene.

note: testing head vs testing latest release tag

IMO important_packages should only be concerned with testing packages at their latest release tag (eg 1.2.3), not at HEAD. That's what my proposed solution would enable.
Testing at HEAD would just break a lot more often especially wrt depenencies.

Especially important packages should have a cfg with --path:src or equivalent if not already.

I'm on the fence on this. should be discussed further.

@Araq
Copy link
Member

Araq commented Apr 24, 2020

IMO important_packages should only be concerned with testing packages at their latest release tag (eg 1.2.3), not at HEAD. That's what my proposed solution would enable.
Testing at HEAD would just break a lot more often especially wrt depenencies.

Ok, that is an acceptable design. However, this PR does nothing like that and stalled.

@Araq Araq closed this Apr 24, 2020
@Araq Araq added the stale Staled PR/issues; remove the label after fixing them label Apr 24, 2020
@timotheecour
Copy link
Member Author

and yet another instance of this: c-blake/cligen#142

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nim CI important_packages failure
3 participants