chore(postgres): not loading the libpq library by default & better user feedback #2028
Conversation
|
You can find the image built from this PR at Built from 2ed497e |
Ivansete-status
requested review from
jm-clius,
vpavlin,
SionoiS and
NagyZoltanPeter
waku/waku_archive/driver/builder.nim
Outdated
| else: | ||
| ## Error variable to be used if the user is trying to mount Store with a Postgres database | ||
| var errPgMsg = "ERROR:" | ||
| errPgMsg &= "The POSTGRES flag should be passed to the make command, e.g. " |
There was a problem hiding this comment.
It sounds me that the message is more like targeting developers not node operators.
It's lucky when it is met.
I would rather say something "This build does not support store with PostgresSQL."
There was a problem hiding this comment.
LGTM! Thanks for this. :) I add one comment re the content of the error message - it should be more generic to not assume knowledge of build environment and wakunode2 in the driver. Will this also fix the requirement for libpq when running make test in a clean environment? I think we might have to hide postgres tests behind the same compiler definition too.
waku/waku_archive/driver/builder.nim
Outdated
|
|
||
| else: | ||
| ## Error variable to be used if the user is trying to mount Store with a Postgres database | ||
| var errPgMsg = "ERROR:" |
There was a problem hiding this comment.
Ah, the error message here (i.e. the builder.nim) can have no knowledge of the encapsulating application, wakunode2, or make commands, etc. It's good that we're trying to be helpful, but at most the driver knows something like: "Postgres has been configured but not been compiled. Check compiler definitions."
I think it's OK to link to the install guide here, even though strictly speaking this module should not even know it's part of nwaku. :)
(Not for this PR, but the correct way to do it is to enumerate the error types here, rather than just returning error strings. That way the application wakunode2 receives an error result when setting up the archive and the application can then log more instructions as it has the context to do so. For now, this will be overkill.)
Super good catch re I'll also make the message more generic as you both suggested :) |
…n builder.nim * We only run the postgres tests when explicitly willing to, i.e. make POSTGRES=1 test. The reason is that the postgres tests expect a database instance to be running locally. * waku/waku_archive_driver/builder.nim: more generic message to also address node operators.
|
Weekly Update
|
Description
Before this change, when a new
nwakuuser wanted to start a node, the next error appeared if the libpq library wasn't installed on his/her system.This is indeed confusing and very little information is given. Especially confusing because it appeared even the user not mounting the Store protocol.
This change aims to enhance that and, by default, if the user doesn't mount the Store protocol, the node will start as usual.
Changes
waku/waku_archive/archive.nim(not directly related to the issue being enhanced in this PR.)How to test
wakunode2without Store mounted. The node should start properly, even if thelibpqis not being installed.wakunode2mounting Store with SQLite. The node should start properly.wakunode2mounting Store with Postgres and nolibpqis present in the system. In this case, the next message should appear and thewakunode2ends immediately:"Postgres has been configured but not been compiled. Check compiler definitions."
i.
make POSTGRES=1 wakunode2 -j11( In this case, the user has postgres in his/her mindset )ii. Run
wakunode2mounting Store with Postgres and nolibpqis present in the system.iii. The next message should appear, which is a little bit more descriptive than the one that appeared in all the cases:
libpq.so.5: cannot open shared object file: No such file or directorylibpq.so: cannot open shared object file: No such file or directorycould not load: libpq.so(.5|)libpq. The node should start properly.