Rocket icon indicating copy to clipboard operation
Rocket copied to clipboard

Default impl for rocket_db_pools.Config not explicit

Open dfego opened this issue 10 months ago • 4 comments

What kind of documentation problem are you reporting?

Unclear Docs

Where is the issue found?

https://api.rocket.rs/v0.5/rocket_db_pools/struct.Config#fields

What's wrong?

Not sure if this is intended or a documentation issue, so I put it as as doc issue first.

According to the docs for rocket_db_pools.Config, there are a handful of specific defaults that I assume are referring to what's used in the absence of one of these structs. I only needed to update the url for my own purposes, so I set that field and then used ..Default::default(), which caused some weird issues. Then I realized that Config derives Default, but doesn't actually implement it, which means the defaults as specified in on that page are not applying.

I would expect, as a user, for either default() to be those documented defaults, or for the lack of this behavior to be explicitly documented (even if it's common practice).

System Checks

  • [x] I confirmed that the issue still exists on master on GitHub.
  • [x] I was unable to find a previous report of this problem.

dfego avatar Mar 16 '25 22:03 dfego

In this case, the docs are correct. Normally, these default values are actually set in the database derive macro, but they should (ideally) be set in the default value as well. Default::default() can't quite have identical behavior, since max_connections's default value is tied to another configuration value, but we can emulate this by assuming workers has the default value.

A PR to fix this issue would be welcome.

the10thWiz avatar Mar 28 '25 08:03 the10thWiz

I'm guessing it's a straightforward-enough fix, I'll take a look.

dfego avatar Mar 28 '25 16:03 dfego

I have some questions.

I'm looking at the contribution guide and in the instructions it mentions running test.sh and fixing any warnings. I just pulled the repository and ran this script (both on master and on v0.5.1, and either way the script doesn't run without lots of warnings and an eventual link error. Should I open an issue for this, or is this a common thing people run into and I'm doing something wrong?

I did:

  1. Clone the repo
  2. cargo build
  3. cd scripts/
  4. ./test.sh --contrib

I also tried no parameters to test.sh and doing this from v0.5.1.

The error that actually occurs is:

  = note: /usr/bin/ld: cannot find -lpq: No such file or directory

Is there a dependency that isn't handled by cargo? Is it documented somewhere?

dfego avatar Mar 28 '25 19:03 dfego

This is a system dependency (namely postgress). It's not super well documented, but for running the tests on db_pools, you have to install the native dependcies for every db type. That particualr error is for libpq, so you likely need to install libpq-devel (assuming you're running a linux system). I'm not sure how to go about

A few more: (note: names here are the fedora package names, yours may differ)

  • libpq-devel
  • sqlite-devel
  • mysql-devel

In general, when Rust has link errors, it means you need to install native dependencies. You can lookup what e.g. -lpq is online, and install the appropriate native dependencies.

Finally, you can mostly ignore the warnings that pop up. I get a bunch, but from what I can tell, they are new warnings, and only appear on recent compiler toolchains. We will likely need to open some new issues to fix them, but you shouldn't need to worry about it.

the10thWiz avatar Mar 29 '25 00:03 the10thWiz