[TODO] regression: Error: Locks requires --threads:on option

[timotheecour]

D20191001T210000
/cc @rayman22201 #12231 introduced a regression, making some projects not compile anymore; eg any project depending on nimongo

Example

nim c -r main.nim

# main.nim
import pkg/nimongo/mongo

Current Output

core/locks.nim(14, 10) Error: Locks requires --threads:on option.

(unlike what's mentioned in #12231 there is not necessarily a link error)

Expected Output

should compile and run

Additional Information

this is a regression introduced in #12231

note that forcing user to add --threads:on is not an option, it causes other errors when adding that flag (eg in jester routes that work without --threads:on) that are really hard to fix

• Your Nim version (output of nim -v).

Nim Compiler Version 1.0.99 [MacOSX: amd64]
Compiled at 2019-10-02
Copyright (c) 2006-2019 by Andreas Rumpf
git hash: c20778d

• Was it working in the previous Nim releases?
  yes

note

would be nice to add nimongo to the set of tested packages in testsuite to prevent such future regressions

[rayman22201]

Nimmongo is obviously relying on broken behavior. Locks require threads. This means my linker bug has now been reintroduced in #12331
😕

Edit:
See here: https://github.com/SSPkrolik/nimongo/blob/master/nimongo/mongo.nim#L2

Nimmongo is completely bypassing the standard idiomatic nim way of turning on threads in Nim, and instead essentially forcing pthreads to be included directly on linux. This probably explains why there is no linker error for Nimmongo. That really seems like a hack, and the wrong way to handle this....

[rayman22201]

I might propose making an RFC with a transition period and compiler warning. Then make a breaking change to reintroduce the compiler error.

Edit: just to clarify

• I agree that reverting this change was the right thing to do
• I agree that nimmongo should be added to important packages
• I think there is a bigger issue here with packages relying on subtly undefined or broken behavior that probably needs to be resolved.

[rayman22201]

note that forcing user to add --threads:on is not an option, it causes other errors when adding that flag (eg in jester routes that work without --threads:on) that are really hard to fix

@timotheecour can you elaborate more on this? What you are describing sounds more like a jester bug than a Nim bug?

[timotheecour]

Nimmongo is completely bypassing the standard idiomatic nim way of turning on threads in Nim, and instead essentially forcing pthreads to be included directly on linux

@rayman22201 maybe (so we don't forget) could you please create an issue in https://github.com/SSPkrolik/nimongo/issues to track this?

/cc @SSPkrolik

@timotheecour can you elaborate more on this? What you are describing sounds more like a jester bug than a Nim bug?

I will follow up on that EDIT: exactly this: dom96/jester#108 or dom96/jester#110 except it now gives errors instead of warnings IIRC, when you're passing --threads; that means code that works without --threads would give CT error when passing --threads, and there's no easy way to fix these errors; maybe what we need is a way to ignore these gcsafe errors, eg: --ignoreGcsafeErrors as a mitigation workaround