Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix cargo-pgrx and pgrx-tests on Windows #1934

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

usamoi
Copy link
Contributor

@usamoi usamoi commented Nov 1, 2024

Notes:

  1. postgres must run without administrator privileges, so switch calling postgres directly to starting postgres by pg_ctl in testing
  2. pgrx-cshim.c is compiled with -flto: without this flag, pgrx_embed.exe is linked to postgres.exe
  3. postgres binaries are download from EDB website, because I don't know how to compile postgres on Windows targeting msvc, and fix cargo-pgrx and pgrx-tests on Windows #1934 (comment)
  4. 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
  5. 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. fix cargo-pgrx and pgrx-tests on Windows #1934 (comment)
  7. Windows CI is added

@usamoi
Copy link
Contributor Author

usamoi commented Nov 1, 2024

raw syntax is stablized in 1.82 but it seems that CI does not know it fixed

@usamoi usamoi force-pushed the win branch 2 times, most recently from 43cc5ca to e07355c Compare November 1, 2024 13:06
@usamoi
Copy link
Contributor Author

usamoi commented Nov 1, 2024

It seems that https://patchwork.kernel.org/project/linux-hardening/patch/20180416175918.GA13494@beast/ breaks --runas on linux. I think the best solution is just telling user to execute sudo sysctl fs.protected_fifos=0 before using --runas.

@usamoi usamoi force-pushed the win branch 6 times, most recently from 7a70740 to 8befc9f Compare November 2, 2024 18:24
@YohDeadfall
Copy link
Contributor

postgres binaries are download from EDB website, because I don't know how to compile postgres on Windows targeting msvc

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 (:

@usamoi
Copy link
Contributor Author

usamoi commented Nov 4, 2024

postgres binaries are download from EDB website, because I don't know how to compile postgres on Windows targeting msvc

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.

@usamoi
Copy link
Contributor Author

usamoi commented Nov 29, 2024

👀 reviews?

@cAttte
Copy link

cAttte commented Feb 23, 2025

this branch works like a charm on windows with MSVC installed and the updated line in .cargo/config.toml, so thank you! would be great if this got reviewed + merged!

@workingjubilee
Copy link
Member

oh cool!

@workingjubilee
Copy link
Member

That's basically the real review this thing needed, let me check the code

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good overall but it should use the std::env::consts instead of all these cfgs.

Comment on lines +414 to +422
let so_ext = 'so_ext: {
if cfg!(target_os = "macos") {
break 'so_ext "dylib";
}
if cfg!(target_os = "windows") {
break 'so_ext "dll";
}
"so"
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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;

Comment on lines +130 to +132
if runas.is_some() {
eyre::bail!("`--runas` is not supported on Windows");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this seems better than trying to reimplement the equivalent for now.

}
("lib", "so")
};
Ok(format!("{prefix}{}.{}", lib_name.replace('-', "_"), extension))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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('-', "_")))

Comment on lines +364 to +367
#[cfg(not(target_os = "windows"))]
path.push("dropdb");
#[cfg(target_os = "windows")]
path.push("dropdb.exe");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this, can we get format!("dropdb{EXE_SUFFIX}") and so on?

Suggested change
#[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");

@workingjubilee
Copy link
Member

workingjubilee commented Feb 23, 2025

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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants