Skip to content

chore: Fix warnings for custom postgres_## cfg flags #3950

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

iamjpotts
Copy link
Contributor

@iamjpotts iamjpotts commented Jul 27, 2025

Resolves build warnings related to custom cfg conditions that are present in the code because they are set for various CI runs defined in a matrix in the GitHub workflow.

Example warning that disappears with this change:

warning: unexpected `cfg` condition name: `postgres_15`
   --> tests/postgres/types.rs:691:24
    |
691 | #[cfg(any(postgres_14, postgres_15))]
    |                        ^^^^^^^^^^^
    |
    = help: consider using a Cargo feature instead
    = help: or consider adding in `Cargo.toml` the `check-cfg` lint config for the lint:
             [lints.rust]
             unexpected_cfgs = { level = "warn", check-cfg = ['cfg(postgres_15)'] }
    = help: or consider adding `println!("cargo::rustc-check-cfg=cfg(postgres_15)");` to the top of the `build.rs`
    = note: see <https://doc.rust-lang.org/nightly/rustc/check-cfg/cargo-specifics.html> for more information about checking conditional configuration

This changes the postgres cfg conditions to be similar to how cfg conditions are setup for mariadb - version is a value of the postgres cfg parameter rather than having a separate parameter/condition for each postgres version.

Additional changes

Since Postgres 12 and prior are now EOL and CI was already adjusted In December to exclude Postgres 12 and earlier after Postgres 12 went EOL in November, some code comments and docstrings are adjusted to reflect that.

Docker compose file - version: 3 was removed since the version field has been deprecated for some time and generates a warning.

Github workflow:

  • Job steps which had multiple docker commands are split into one step per docker command (easier to review logs)
  • Steps which ran ssl auth tests are split into their own job - this prevents conflicts on host port 5432, which were happening even though the docker container was torn down (using docker compose rm -f did not help either)

#3879 will need modification after this is merged.

Does your PR solve an issue?

  • reduces build warnings related to --cfg flags.
  • eliminates warning related to deprecated docker compose version field

Is this a breaking change?

No - informs linter of an expected set of possible custom cfg parameter values, and adjusts test code, but not production code (other than comments)

@iamjpotts iamjpotts marked this pull request as ready for review July 27, 2025 01:38
@iamjpotts iamjpotts marked this pull request as draft August 10, 2025 17:21
@iamjpotts iamjpotts force-pushed the jp/cfg-postgres branch 6 times, most recently from 3d250e0 to 40c749e Compare August 10, 2025 18:45
@iamjpotts iamjpotts marked this pull request as ready for review August 10, 2025 19:00
@iamjpotts iamjpotts force-pushed the jp/cfg-postgres branch 2 times, most recently from c605bd5 to 2d1a5b1 Compare August 15, 2025 22:31
@iamjpotts
Copy link
Contributor Author

The CI failure is a transient database connection error:

error: error communicating with database: Connection reset by peer (os error 104)

I do not see a way to rerun the failed job.

@iamjpotts
Copy link
Contributor Author

I do not see a way to rerun the failed job.

Resigned last commit and forced push to rerun jobs.

@iamjpotts
Copy link
Contributor Author

Same transient failure as before, on same Postgres (13, async-std, native-tls) job:

Run cargo test --no-default-features --features any,postgres,macros,_unstable-all-types,runtime-async-std,tls-native-tls
warning: /home/runner/work/sqlx/sqlx/Cargo.toml: file `/home/runner/work/sqlx/sqlx/tests/sqlite/macros.rs` found to be present in multiple build targets:
  * `integration-test` target `sqlite-macros`
  * `integration-test` target `sqlite-unbundled-macros`
warning: /home/runner/work/sqlx/sqlx/sqlx-cli/Cargo.toml: `default-features` is ignored for sqlx, since `default-features` was not specified for `workspace.dependencies.sqlx`, this could become a hard error in the future
   Compiling sqlx v0.9.0-alpha.1 (/home/runner/work/sqlx/sqlx)
error: error communicating with database: Connection reset by peer (os error 104)
    --> tests/postgres/postgres.rs:1149:5
     |
1149 |     sqlx::query!("SELECT 1 AS one")
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: this error originates in the macro `$crate::sqlx_macros::expand_query` which comes from the expansion of the macro `sqlx::query` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `sqlx` (test "postgres") due to 1 previous error
warning: build failed, waiting for other jobs to finish...
Error: Process completed with exit code 101.

@abonander
Copy link
Collaborator

It could be a race condition with Postgres starting up. I think we normally sleep a certain amount of time to make sure it's up first.

@iamjpotts
Copy link
Contributor Author

Same failure, different postgres 13 job.

@iamjpotts
Copy link
Contributor Author

iamjpotts commented Aug 16, 2025

I found two more places where the old cfg flags needed to get removed (docs).

@iamjpotts iamjpotts force-pushed the jp/cfg-postgres branch 2 times, most recently from 4f20b96 to 6a90586 Compare August 16, 2025 01:12
@iamjpotts
Copy link
Contributor Author

time="2025-08-16T01:17:41Z" level=warning msg="Found orphan containers ([postgres_13]) for this project. If you removed or renamed this service in your compose file, you can run this command with the --remove-orphans flag to clean it up."

This error from the step near the end that does docker compose -f after docker stop may be related.

@iamjpotts iamjpotts force-pushed the jp/cfg-postgres branch 2 times, most recently from e4e19e6 to a92236f Compare August 16, 2025 02:06
@iamjpotts
Copy link
Contributor Author

It could be a race condition with Postgres starting up. I think we normally sleep a certain amount of time to make sure it's up first.

@abonander the ultimate cause of the failure was a bind failure when docker was trying to use host port 5432 for the job step that stopped the non-ssl container and created the ssl container.

Job step with failure:

# client SSL authentication
- run: |
docker stop postgres_${{ matrix.postgres }}
docker compose -f tests/docker-compose.yml run -d -p 5432:5432 --name postgres_${{ matrix.postgres }}_client_ssl postgres_${{ matrix.postgres }}_client_ssl
docker exec postgres_${{ matrix.postgres }}_client_ssl bash -c "until pg_isready; do sleep 1; done"

I've amended this pr to split the postgres job into two. That change could be a standalone pr if you wish.

@iamjpotts
Copy link
Contributor Author

@abonander I'm done with my flurry of branch updates used to test CI changes.

I also cherry picked the last commit which is the fix into its own PR if you prefer that: #3977.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants