Skip to content
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

pgx-utils goes away! #911

Merged
merged 3 commits into from
Dec 5, 2022
Merged

pgx-utils goes away! #911

merged 3 commits into from
Dec 5, 2022

Conversation

eeeebbbbrrrr
Copy link
Contributor

@eeeebbbbrrrr eeeebbbbrrrr commented Dec 4, 2022

  • Move the sql_entity_graph module out of pgx-utils and into its own crate.
  • Move pgx-utils/src/rewriter.rs into pgx-macros/src and delete the pgx-utils crate entirely
  • Some dead code removal

From there, the changes here are largely mechanical to account for the crate changes. A few 3rd-party dependencies have been eliminated here and there as well.

pgx-pg-sys/build.rs now applies #[pg_guard] to bindgen extern "C" FFI blocks instead of automatically rewriting to its output directly. In testing this doesn't seem to hurt compilation performance and it removes a dependency in our build chain.

Relates to Issue #655.

- Move the `sql_entity_graph` module out of `pgx-utils` and into its own crate.
- Move `pgx-utils/src/rewriter.rs` into `pgx-macros/src` and delete the `pgx-utils` crate entirely
- Some dead code removal

From there, the changes here are largely mechanical to account for the crate changes.  A few 3rd-party dependencies have been eliminated here and there as well.

`pgx-pg-sys/build.rs` now applies `#[pg_guard]` to bindgen `extern "C"` FFI blocks instead of embedding it directly.  In testing this doesn't seem to hurt compilation performance and it removes a dependency in our build chain.
@workingjubilee
Copy link
Member

As you said yourself, we probably don't want to just rename pgx-utils if we can remove it instead, and this, despite the name, effectively constitutes renaming the crate because it just moves things around. @thomcc mentioned a plan to move most of the SQL entity graph code into cargo-pgx.

Copy link
Contributor

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

This is basically good -- admittedly I just skimmed stuff that looked like it's part of the rename/move.

A lot of this is stuff I wanted to do anyway after the JSON patch, which this will likely simplify (although it does mean that patch will need rewriting ⚰️).

There's some other follow-up stuff that I think we'll want -- (this will regress macro error messages slightly, and possibly compile-times for pgx users), but none of that is urgent or needs to block this (I'll try to get to those issues as part of a follow-up soon).

@@ -150,9 +140,9 @@ impl PgGuardRewriter {
arg_list.extend(quote! { #ident, });
}
} else {
eyre::bail!(
panic!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we'd use syn::Error here, since panic will give a pretty bad error message inside a macro/build script.

pgx-pg-sys/build.rs Show resolved Hide resolved
@eeeebbbbrrrr
Copy link
Contributor Author

eeeebbbbrrrr commented Dec 5, 2022

As you said yourself, we probably don't want to just rename pgx-utils if we can remove it instead, and this, despite the name, effectively constitutes renaming the crate because it just moves things around. @thomcc mentioned a plan to move most of the SQL entity graph code into cargo-pgx.

It is a rename, but at least it's clear what the code is used for now. It sounds like @thomcc has further ideas that might let us eliminate the crate entirely, which would be great, but I think this is still important just for clarity.

The big thing here was being able to move rewriter.rs over to pgx-macros. Obviously, it could have happened without essentially just rename pgx-utils, but again, things are a bit more clear now.

@workingjubilee
Copy link
Member

Fair enough, then!

yrashk pushed a commit to yrashk/pgx that referenced this pull request Dec 6, 2022
- Move the `sql_entity_graph` module out of `pgx-utils` and into its own crate.
- Move `pgx-utils/src/rewriter.rs` into `pgx-macros/src` and delete the `pgx-utils` crate entirely
- Some dead code removal

From there, the changes here are largely mechanical to account for the crate changes.  A few 3rd-party dependencies have been eliminated here and there as well.

`pgx-pg-sys/build.rs` now applies `#[pg_guard]` to bindgen `extern "C"` FFI blocks instead of automatically rewriting to its output directly.  In testing this doesn't seem to hurt compilation performance and it removes a dependency in our build chain.

Relates to Issue pgcentralfoundation#655.
@eeeebbbbrrrr eeeebbbbrrrr deleted the sql-entity-graph-crate branch June 20, 2023 18:00
workingjubilee pushed a commit that referenced this pull request Jan 8, 2024
#911 introduced using the wrong signature and
this UB was missed in review amidst the thousands of lines of changes.

Signed-off-by: usamoi <[email protected]>
workingjubilee pushed a commit that referenced this pull request Jan 24, 2024
#911 introduced using the wrong signature and
this UB was missed in review amidst the thousands of lines of changes.

Signed-off-by: usamoi <[email protected]>
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.

3 participants