fix cargo-pgrx and pgrx-tests on Windows#1934
fix cargo-pgrx and pgrx-tests on Windows#1934workingjubilee merged 8 commits intopgcentralfoundation:developfrom
Conversation
|
|
43cc5ca to
e07355c
Compare
|
It seems that https://patchwork.kernel.org/project/linux-hardening/patch/20180416175918.GA13494@beast/ breaks |
7a70740 to
8befc9f
Compare
The process is explained in the official documentation: https://www.postgresql.org/docs/16/install-windows.html Not a Windows user for years, but can help if you're stuck with something (: |
I'm curious if there are really any Windows developers. So, I'm planning to leave this feature here to see if anyone fixes it. |
|
👀 reviews? |
|
this branch works like a charm on windows with MSVC installed and the updated line in |
|
oh cool! |
|
That's basically the real review this thing needed, let me check the code |
workingjubilee
left a comment
There was a problem hiding this comment.
This looks good overall but it should use the std::env::consts instead of all these cfgs.
cargo-pgrx/src/command/install.rs
Outdated
| let so_ext = 'so_ext: { | ||
| if cfg!(target_os = "macos") { | ||
| break 'so_ext "dylib"; | ||
| } | ||
| if cfg!(target_os = "windows") { | ||
| break 'so_ext "dll"; | ||
| } | ||
| "so" | ||
| }; |
There was a problem hiding this comment.
| let so_ext = 'so_ext: { | |
| if cfg!(target_os = "macos") { | |
| break 'so_ext "dylib"; | |
| } | |
| if cfg!(target_os = "windows") { | |
| break 'so_ext "dll"; | |
| } | |
| "so" | |
| }; | |
| let so_ext = std::env::consts::DLL_EXTENSION; |
| if runas.is_some() { | ||
| eyre::bail!("`--runas` is not supported on Windows"); | ||
| } |
There was a problem hiding this comment.
Yeah, this seems better than trying to reimplement the equivalent for now.
pgrx-pg-config/src/cargo.rs
Outdated
| } | ||
| ("lib", "so") | ||
| }; | ||
| Ok(format!("{prefix}{}.{}", lib_name.replace('-', "_"), extension)) |
There was a problem hiding this comment.
| Ok(format!("{prefix}{}.{}", lib_name.replace('-', "_"), extension)) | |
| use std::env::consts::{DLL_PREFIX, DLL_SUFFIX}; | |
| Ok(format!("{DLL_PREFIX}{name}{DLL_SUFFIX}", name = lib_name.replace('-', "_"))) |
pgrx-pg-config/src/lib.rs
Outdated
| #[cfg(not(target_os = "windows"))] | ||
| path.push("dropdb"); | ||
| #[cfg(target_os = "windows")] | ||
| path.push("dropdb.exe"); |
There was a problem hiding this comment.
Instead of this, can we get format!("dropdb{EXE_SUFFIX}") and so on?
| #[cfg(not(target_os = "windows"))] | |
| path.push("dropdb"); | |
| #[cfg(target_os = "windows")] | |
| path.push("dropdb.exe"); | |
| #[cfg(not(target_os = "windows"))] | |
| path.push("dropdb"); | |
| #[cfg(target_os = "windows")] | |
| path.push("dropdb.exe"); |
|
Exactly how you thread in the consts I'm not inclined to argue... you'll note I used two different forms in the above suggestions, you can pick one you prefer or use both and add a third... just asking to use them when they're available. |
|
Windows CI fails. I'll see it later. |
workingjubilee
left a comment
There was a problem hiding this comment.
great! just needs to resolve conflicts.
| if bytes.starts_with(b"PK\x03\x04") | ||
| || bytes.starts_with(b"PK\x05\x06") | ||
| || bytes.starts_with(b"PK\x07\x08") |
There was a problem hiding this comment.
one question: is this magic a true byte string or is it a uint32_t, in true C style?
There was a problem hiding this comment.
it's copied from wikipedia directly, so this should be a true byte string?
There was a problem hiding this comment.
It's the zip based file format magic bits: 50 4B 03 04, 50 4B 05 06, and 50 4B 07 08. The last two bytes are designators for the archive type.
|
Windows CI fails due to rust-lang/rust#129582 (Rust 1.84) before. The pull request changes the behavior of "an @cAttte I’m really sorry to say that if you compiled the PGRX extension with a Rust version higher than 1.83 on my previous branch and ran into the 0xC0000409 error, please try recompiling it with Rust 1.83. |
cce6c68 to
e4c6c4e
Compare
time to make everything |
|
So I think it's ready to be merged? |
|
Thanks! |
|
yay 🚀 thank you both very much |
Fixes a known issue (cshim requires LTO) in #1934.
Notes: 1. postgres must run without administrator privileges, so switch calling `postgres` directly to starting postgres by `pg_ctl` in testing 2. Add Windows CI 3. known issue: `pgrx-cshim.c` is compiled with `-flto`, because, without this flag, `pgrx_embed.exe` is linked to `postgres.exe` 4. known issue: `cargo-pgrx` downloads, instead of building, postgres binaries from EDB website 5. known issue: `cargo pgrx run` and `cargo pgrx test` always print logs for `pg_ctl start` on Windows (pipes generated by `std::process::Command` are leaked to postgres, a command with `Stdio::piped()` hangs, so we use `Stdio::inherit()` on Windows) 6. breaking change: this pull request fixes `PgLwLock` and `PgAtomic` but introduces a breaking change about `PgLwLock` and `PgAtomic`: name must be provided at `PgLwLock/PgAtomic::new`, `PgLwLock::from_named` is removed and a parameter of `PgLwLock::attach` changes 7. breaking change: this pull request makes everything `C-unwind` and it's necessary for downstream to switch from `C` to `C-unwind` 8. regression: `cargo-pgrx` users need to execute `sudo sysctl fs.protected_fifos=0` before using `--runas` on Linux
…oundation#2006) Fixes a known issue (cshim requires LTO) in pgcentralfoundation#1934.
…gcentralfoundation#2014) `pgrx` is switched from "C" to "C-unwind" in pgcentralfoundation#1934 so it's fine to emit an error here Without this error, someone who updates dependencies without reading changelog, would break extensions silently, if their only usage of `#[pg_guard]` is `_PG_init`. It's bad.
Notes: 1. postgres must run without administrator privileges, so switch calling `postgres` directly to starting postgres by `pg_ctl` in testing 2. Add Windows CI 3. known issue: `pgrx-cshim.c` is compiled with `-flto`, because, without this flag, `pgrx_embed.exe` is linked to `postgres.exe` 4. known issue: `cargo-pgrx` downloads, instead of building, postgres binaries from EDB website 5. known issue: `cargo pgrx run` and `cargo pgrx test` always print logs for `pg_ctl start` on Windows (pipes generated by `std::process::Command` are leaked to postgres, a command with `Stdio::piped()` hangs, so we use `Stdio::inherit()` on Windows) 6. breaking change: this pull request fixes `PgLwLock` and `PgAtomic` but introduces a breaking change about `PgLwLock` and `PgAtomic`: name must be provided at `PgLwLock/PgAtomic::new`, `PgLwLock::from_named` is removed and a parameter of `PgLwLock::attach` changes 7. breaking change: this pull request makes everything `C-unwind` and it's necessary for downstream to switch from `C` to `C-unwind` 8. regression: `cargo-pgrx` users need to execute `sudo sysctl fs.protected_fifos=0` before using `--runas` on Linux
…oundation#2006) Fixes a known issue (cshim requires LTO) in pgcentralfoundation#1934.
…gcentralfoundation#2014) `pgrx` is switched from "C" to "C-unwind" in pgcentralfoundation#1934 so it's fine to emit an error here Without this error, someone who updates dependencies without reading changelog, would break extensions silently, if their only usage of `#[pg_guard]` is `_PG_init`. It's bad.
Notes:
postgresdirectly to starting postgres bypg_ctlin testingpgrx-cshim.cis compiled with-flto, because, without this flag,pgrx_embed.exeis linked topostgres.execargo-pgrxdownloads, instead of building, postgres binaries from EDB websitecargo pgrx runandcargo pgrx testalways print logs forpg_ctl starton Windows (pipes generated bystd::process::Commandare leaked to postgres, a command withStdio::piped()hangs, so we useStdio::inherit()on Windows)PgLwLockandPgAtomicbut introduces a breaking change aboutPgLwLockandPgAtomic: name must be provided atPgLwLock/PgAtomic::new,PgLwLock::from_namedis removed and a parameter ofPgLwLock::attachchangesC-unwindand it's necessary for downstream to switch fromCtoC-unwindcargo-pgrxusers need to executesudo sysctl fs.protected_fifos=0before using--runason Linux