tcdi / postgrestd

The most peculiar std you have ever seen
Other
37 stars 4 forks source link

Audit and overhaul postgrestd internals #41

Closed thomcc closed 1 year ago

thomcc commented 1 year ago

This fixes many, many things that were unintentionally exposed, broken, needing reconsidering, or that should have been fine but weren't[^1]. This is mostly a consequence of the fact that we gained a much better idea of what we need from postgrestd between when it was written and now.

Sadly, it's an enormous single PR, which is mostly due to time constraints.

[^1]: I mean, really, why is is there so much low level networking code in wierd places? libc usage in std::sys_common::net is one thing, but std::os::unix::net should not be so large, complex, and full of syscalls!

Removed APIs and bugfixes

Alright there's a lot here. Almost everything should not impact public API, only the runtime functionality. A single exception to this exists: std::os::unix::{ucred,net} are currently removed (rather than just having their functionality stubbed out). These should be re-added once we're confident they're fully disabled, but in the short term (see below for more details).

Where possible, the changes were either done in a way that should minimize likelihood of merge conflicts, although in cases where we'd fail to notice a problematic addition, I went with the merge conflict route.

Stuff needing futher work/thought

None of this is a blocker, but here are some things I don't love and would like to get back to.

Bonus: Includes finished macOS support

Added bonus: macOS is sort of supported now in a mostly experimental configuration. That is, this replaces #40, since while working on it I noticed these issues (and the cfg fixes here make macOS support nearly automatic. Not to mention, my only fast machine is a macbook, so I'd still be working if I hadn't added it)

One notable wonkiness about the support is that we can't use the same target vendor convention we've been using on apple for two reasons: target_vendor = "apple" is used a fair bit in the wild to detect any(macos, ios, tvos, watchos), and the stdlib (and folks in the wild) has build-time code which assumes env::var("TARGET").unwrap().contains("apple-darwin") is the way to check for macOS. Honestly I don't blame either, so I've stuck it at the end for now, as <arch>-apple-darwin-postgres, which seems to work.

eeeebbbbrrrr commented 1 year ago

Considering the size and scope of the PR, this doesn't seem overly complex. It boils down to having the experience and knowledge to know if the right parts of the stdlib have received the right treatment.

To that end, I want @workingjubilee to review and approve this one.