Remove the `pg_sql_graph_magic!()` macro. #1591

eeeebbbbrrrr · 2024-02-29T16:38:27Z

The pg_sql_graph_magic!() did some shenanigans related to loading the extension ".control" file at compile-time and embedded it in the compiled artifact. This was convoluted and actually unnecessary as the location where that information was used (in schema.rs) can easily access the ".control" file at runtime.

This PR removes that macro and its usages.

The fn __pgrx__marker() function is still required, however. It's necessary to ensure that the rustc build we do for the pgrx_embed binary for schema generation properly links so that the other (dynamically referenced) __pgrx_internals_XXX symbols can be found. The function, however, is now a noop.

This is related to #1546, but is not a solution. It should just make solving that request easier.

The `pg_sql_graph_magic!()` did some shenanigans related to loading the extension ".control" file at compile-time and embedded it in the compiled artifact. This was convoluted and actually unnecessary as the location where that information was used (in `schema.rs`) can easily access the ".control" file at runtime. This PR removes that macro and its usages. The `fn __pgrx__marker()` function is still required, however. It's necessary to ensure that the `rustc` build we do for the `pgrx_embed` binary for schema generation properly links so that the other (dynamically referenced) `__pgrx_internals_XXX` symbols can be found. The function, however, is now a noop.

pgrx-tests/src/framework.rs

pgrx/src/lib.rs

…ol_file.rs`

pgrx-sql-entity-graph/src/control_file.rs

…binary as our own

workingjubilee · 2024-02-29T17:58:00Z

For the audience's benefit: This cannot be landed as-is because it breaks cargo pgrx schema, which is somewhat necessary for diagnostic purposes.

… be in the ".control" file, we need to pluck the version number out of the manifest and pass it through to the `pgrx_embed` binary as an envvar. Also add a CI test for running `cargo pgrx schema`, which is how I manually discovered this problem.

eeeebbbbrrrr · 2024-02-29T18:09:56Z

For the audience's benefit: This cannot be landed as-is because it breaks cargo pgrx schema, which is somewhat necessary for diagnostic purposes.

ee63bf9 fixes that. ;)

cargo-pgrx/src/command/schema.rs

…ess frustration.

eeeebbbbrrrr · 2024-02-29T20:13:00Z

It’s green!

workingjubilee · 2024-02-29T20:19:37Z

Holy shit!

workingjubilee

Just some comments for future reference if I ever have to come back to this PR. It looks good! Moving build complexity out of the library is great.

workingjubilee · 2024-02-29T20:30:07Z

pgrx-sql-entity-graph/src/control_file.rs

+            // endeavor to not require external values if they're not used by the input
+            if input.contains(CARGO_VERSION) {
+                input = input.replace(
+                    CARGO_VERSION,
+                    &std::env::var("CARGO_PKG_VERSION").map_err(|_| {
+                        ControlFileError::MissingEnvvar("CARGO_PKG_VERSION".to_string())
+                    })?,
+                );
+            }


This does introduce a slight behavior divergence, but that seems fine, since the main validation effort is done via cargo-pgrx.

workingjubilee · 2024-02-29T20:35:14Z

cargo-pgrx/src/command/schema.rs

+            // call the marker.  Primarily this ensures that rustc will actually link to the library
+            // during the "pgrx_embed" build initiated by `cargo-pgrx schema` generation
+            #lib_name_ident::__pgrx_marker();


Perhaps we should use pub extern "C" fn on this marker? rustc is more reluctant to play around with obviously-for-external-consumption interfaces. It's not something we need to fix now, however, just musing aloud.

workingjubilee · 2024-02-29T20:39:08Z

cargo-pgrx/src/command/schema.rs

            let mut entities = Vec::new();
-            let control_file_entity = ::pgrx::pgrx_sql_entity_graph::SqlGraphEntity::ExtensionRoot(::#lib_name_ident::__pgrx_marker());
+            let control_file_path = std::path::PathBuf::from(#control_file_path);


I wonder if there's some cursed set of cases that results in this being a malformed UTF-8 string? ...but if someone seriously asks for supporting non-UTF-8 paths in our greenfield extension build system, I'm probably going to tell them exactly where they can shove that request. Maybe we should be simplifying our code by using camino.

workingjubilee · 2024-02-29T20:40:36Z

pgrx-sql-entity-graph/src/control_file.rs

+    /// # // arrays.control chosen because it does **NOT** use the @CARGO_VERSION@ variable
+    /// let context = include_str!("../../pgrx-examples/arrays/arrays.control");


Yeah this seems fair. We have other tests to guarantee this works.

workingjubilee · 2024-02-29T20:44:52Z

pgrx/src/lib.rs

-#[macro_export]
-macro_rules! pg_sql_graph_magic {
-    () => {
-        // A marker which must exist in the root of the extension.
-        #[doc(hidden)]
-        pub fn __pgrx_marker() -> $crate::pgrx_sql_entity_graph::ControlFile {
-            use ::core::convert::TryFrom;
-            let package_version = env!("CARGO_PKG_VERSION");
-            let context = include_str!(concat!(
-                env!("CARGO_MANIFEST_DIR"),
-                "/",
-                env!("CARGO_CRATE_NAME"),
-                ".control"
-            ))
-            .replace("@CARGO_VERSION@", package_version);
-
-            let control_file =
-                $crate::pgrx_sql_entity_graph::ControlFile::try_from(context.as_str())
-                    .expect("Could not parse control file, is it valid?");
-            control_file
-        }
-    };
-}


@usamoi

Welcome to pgrx 0.12.0-alpha.1! Say the magic words with me! ```shell cargo install cargo-pgrx --locked --version 0.12.0-alpha.1 ``` # Breaking Changes ## No more dlopen! Perhaps the most exciting change this round is @usamoi's contribution in #1468 which means that we no longer perform a `dlopen` in order to generate the schema. The cost, such as it is, is that your pgrx extensions now require a `src/bin/pgrx_embed.rs`, which will be used to generate the schema. This has much less cross-platform issues and will enable supporting things like `cargo binstall` down the line. It may be a bit touchy on first-time setup for transitioning older repos. If necessary, you may have to directly add a `src/bin/pgrx_embed.rs` and add the following code (which should be the only code in the file, though you can add comments if you like?): ```rust ::pgrx::pgrx_embed!(); ``` Your Cargo.toml will also want to update its crate-type key for the library: ```toml [lib] crate-type = ["cdylib", "lib"] ``` ## Library Code - pgrx-pg-sys will now use `ManuallyDropUnion` thanks to @NotGyro in #1547 - VARHDRSZ `const`s are no longer `fn`, thanks to @workingjubilee in #1584 - We no longer have `Interval::is_finite` since #1594 - We translate more `*_tree_walker` functions to the same signature their `*_impl` version in Postgres 16 has: #1596 - Thanks to @eeeebbbbrrrr in #1591 we no longer have the `pg_sql_graph_magic!()` macro, which should help with more things in the future! # What's New We have quite a lot of useful additions to our API: - `SpiClient::prepare_mut` was added thanks to @XeniaLu in #1275 - @usamoi also contributed bindings subscripting code in #1562 - For `#[pg_test]`, you have been able to use `#[should_panic(expected = "string")]` to anticipate a panic that contains that string in that test. For various reasons, `#[pg_test(error = "string")]` is much the same. Now, you can also use `#[pg_test(expected = "string")]`, in the hopes that is easier to stumble across, as of #1570 ## `Result<composite_type!("..."), E>` support - In #1560 @NotGyro contributed support for using `Result<composite_type!("Name"), E>`, as a case that had not been handled before. ## Significantly expanded docs Thanks to @rjuju, @NotGyro, and @workingjubilee, we now have significantly expanded docs for cargo-pgrx and pgrx in general. Some of these are in the API docs on https://docs.rs or the READMEs, but there's also a guide, now! It's not currently published, but is available as an [mdbook](https://github.com/rust-lang/mdBook) in the repo. Some diagnostic information that is also arguably documentation, like comments and the suggestion to `cargo install`, have also been improved, thanks to @workingjubilee in - #1579 - #1573 ## `#[pg_cast]` An experimental macro for a `CREATE CAST` was contributed by @xwkuang5 in #1445! ## Legal Stuff Thanks to @the-kenny in #1490 and @workingjubilee in #1504, it was brought to our attention that some dependencies had unusual legal requirements. So we fixed this with CI! We now check our code included into pgrx-using binaries is MIT/Apache 2.0 licensed, as is common across crates.io, using `cargo deny`!. The build tools will have more flexible legal requirements (partly due to the use of Mozilla Public License code in rustls). # Internal Changes Many internal cleanups were done thanks to - @workingjubilee in too many PRs to count! - @thomcc found a needless condition in #1501 - @nyurik in too many PRs to count! In particular: - we now actually `pfree` our `Array`s we detoasted as-of #1571 - creating a `RawArray` is now low-overhead due to #1587 ## Soundness Fixes We had a number of soundness issues uncovered or have added more tests to catch them. - Bounds-checking debug assertions for array access by @NotGyro in #1514 - Fix unsound `&` and `&mut` in `fcinfo.rs` by @workingjubilee in #1595 ## Less Deps Part of the cleanup by @workingjubilee was reducing the number of deps we compile: * cargo-pgrx: reduce trivial dep usages in #1499 * Update 2 syn in #1557 Hopefully it will reduce compile time and disk usage! ## New Contributors * @the-kenny made their first contribution in #1490 * @xwkuang5 made their first contribution in #1445 * @rjuju made their first contribution in #1516 * @nyurik made their first contribution in #1533 * @NotGyro made their first contribution in #1514 * @XeniaLu made their first contribution in #1275 **Full Changelog**: v0.12.0-alpha.0...v0.12.0-alpha.1

eeeebbbbrrrr requested a review from workingjubilee February 29, 2024 16:38

woah. need to get that envvar at runtime, not compile time!

8dd9469

eeeebbbbrrrr commented Feb 29, 2024

View reviewed changes

pgrx-tests/src/framework.rs Show resolved Hide resolved

eeeebbbbrrrr commented Feb 29, 2024

View reviewed changes

pgrx/src/lib.rs Show resolved Hide resolved

eeeebbbbrrrr added 4 commits February 29, 2024 11:59

code review cleanup

90098e7

code review cleanup

0317831

minor refactor to move .control file variable substitutions to `contr…

7216d02

…ol_file.rs`

come on, cargo fmt. seriously

e845056

eeeebbbbrrrr commented Feb 29, 2024

View reviewed changes

pgrx-sql-entity-graph/src/control_file.rs Show resolved Hide resolved

carry cargo's CARGO_PKG_VERSION value through to the pgrx_embed …

f720ef2

…binary as our own

eeeebbbbrrrr marked this pull request as draft February 29, 2024 17:53

eeeebbbbrrrr mentioned this pull request Feb 29, 2024

FR: Support extension names not matching the crate names #1546

Open

workingjubilee reviewed Feb 29, 2024

View reviewed changes

cargo-pgrx/src/command/schema.rs Outdated Show resolved Hide resolved

eeeebbbbrrrr added 2 commits February 29, 2024 13:44

more cleanups around CARGO_PKG_VERSION

5581c40

seriously clippy.

e4a5e1d

eeeebbbbrrrr marked this pull request as ready for review February 29, 2024 18:53

eeeebbbbrrrr added 2 commits February 29, 2024 14:14

CI: UGH

6a258b6

I swear. Something about CI causes my brain to shut off, causing endl…

0611431

…ess frustration.

workingjubilee approved these changes Feb 29, 2024

View reviewed changes

workingjubilee merged commit 5f0db58 into pgcentralfoundation:develop Feb 29, 2024
11 checks passed

workingjubilee mentioned this pull request Mar 1, 2024

Release pgrx 0.12.0-alpha.1 #1598

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the `pg_sql_graph_magic!()` macro. #1591

Remove the `pg_sql_graph_magic!()` macro. #1591

eeeebbbbrrrr commented Feb 29, 2024 •

edited

Loading

workingjubilee commented Feb 29, 2024

eeeebbbbrrrr commented Feb 29, 2024

eeeebbbbrrrr commented Feb 29, 2024

workingjubilee commented Feb 29, 2024

workingjubilee left a comment

workingjubilee Feb 29, 2024

workingjubilee Feb 29, 2024

workingjubilee Feb 29, 2024

workingjubilee Feb 29, 2024

workingjubilee Feb 29, 2024

		/// # // arrays.control chosen because it does NOT use the @CARGO_VERSION@ variable
		/// let context = include_str!("../../pgrx-examples/arrays/arrays.control");

Remove the pg_sql_graph_magic!() macro. #1591

Remove the pg_sql_graph_magic!() macro. #1591

Conversation

eeeebbbbrrrr commented Feb 29, 2024 • edited Loading

workingjubilee commented Feb 29, 2024

eeeebbbbrrrr commented Feb 29, 2024

eeeebbbbrrrr commented Feb 29, 2024

workingjubilee commented Feb 29, 2024

workingjubilee left a comment

Choose a reason for hiding this comment

workingjubilee Feb 29, 2024

Choose a reason for hiding this comment

workingjubilee Feb 29, 2024

Choose a reason for hiding this comment

workingjubilee Feb 29, 2024

Choose a reason for hiding this comment

workingjubilee Feb 29, 2024

Choose a reason for hiding this comment

workingjubilee Feb 29, 2024

Choose a reason for hiding this comment

Remove the `pg_sql_graph_magic!()` macro. #1591

Remove the `pg_sql_graph_magic!()` macro. #1591

eeeebbbbrrrr commented Feb 29, 2024 •

edited

Loading