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

Add a default cshim feature flag #958

Merged
merged 6 commits into from
Dec 18, 2022
Merged

Add a default cshim feature flag #958

merged 6 commits into from
Dec 18, 2022

Conversation

eeeebbbbrrrr
Copy link
Contributor

@eeeebbbbrrrr eeeebbbbrrrr commented Dec 13, 2022

This adds a cshim feature flag to pgx, enabled by default, and pushes it down to pgx-pg-sys and pgx-tests.

With this turned off we lose low-level access to a few things:

  • pgx_list* functions
  • pgx_planner_rt_fetch()
  • pgx_SpinLock* functions

From pgx' perspective, this means a few modules are just not available without cshim:

  • hooks (uses PgList)
  • list (impl for PgList which uses pgx_list* functions)
  • namespace (uses PgList)
  • spinlock (uses pgx_SpinLock* functions)

This feature flag will hopefully make it easier to cross-compile pgx with only a small bit of compromises. This feature flag was initially designed with plrust user functions in mind, and they won't need access to any of the above.

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

I think this is ready for merge.

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.

Strictly speaking this should probably be a cshim feature enabled by default (enabling a cargo feature is never supposed to remove functionality).

But in practice I don't think this matters much, and it might be a hassle to disable otherwise.

@eeeebbbbrrrr
Copy link
Contributor Author

eeeebbbbrrrr commented Dec 13, 2022

Strictly speaking this should probably be a cshim feature enabled by default (enabling a cargo feature is never supposed to remove functionality).

But in practice I don't think this matters much, and it might be a hassle to disable otherwise.

Hmm. I think that would be incredibly invasive, code wise. Would that not mean basically putting a #[cfg] on nearly everything, or otherwise a major reorg?

@thomcc
Copy link
Contributor

thomcc commented Dec 13, 2022

I think it would mostly be a matter of changing #[cfg(not(feature = "no-cshim"))] to #[cfg(feature = "cshim")], no?

@thomcc
Copy link
Contributor

thomcc commented Dec 13, 2022

That said this isn't really a feature we expect users to enable, so it's probably fine.

@eeeebbbbrrrr
Copy link
Contributor Author

#[cfg(feature = "cshim")]

Would it not mean adding this to everything that currently doesn't have this feature flag at all? Am I missing something.

@workingjubilee
Copy link
Member

@eeeebbbbrrrr No, it would involve adding the feature flag only to things enabled specifically by the cshim. Currently the flag strategy is a "double negative": "if the no-cshim feature is NOT enabled, then include this code". Thus it's fine to simply reduce the double negatives to positives. Then, we'd add default = ["cshim"] to the Cargo.toml.

@eeeebbbbrrrr
Copy link
Contributor Author

default = ["cshim"]

Do we like that? Sometimes using cargo across the entire pgx repo (cargo test --all) requires setting specific features and also specifying --no-default-features.

I can do it this way, of course, if it's preferred and otherwise idiomatic.

@workingjubilee
Copy link
Member

Yeah, I don't mind setting features like that. We can prevent settings creep by making sure at least one feature includes all default features that aren't a Postgres version.

@eeeebbbbrrrr
Copy link
Contributor Author

Okay, I'll rearrange this.

@eeeebbbbrrrr eeeebbbbrrrr changed the title Add a no-cshim feature flag Add a default cshim feature flag Dec 14, 2022
@eeeebbbbrrrr
Copy link
Contributor Author

I don't think this PR is going to survive in this state after our discord discussions around cargo test. I'm switching gears to work on that, then I'll come back to this.

This adds a `no-cshim` feature flag to `pgx` and pushes it down to `pgx-pg-sys` and `pgx-tests`.

With this turned on we lose low-level access to a few things:

   - `pgx_list*` functions
   - `pgx_planner_rt_fetch()`
   - `pgx_SpinLock*` functions

From `pgx`' perspective, this means a few modules are just not available under `no-cshim`:

   - `hooks` (uses `PgList`)
   - `list` (impl for `PgList` which uses `pgx_list*` functions)
   - `namespace` (uses `PgList`)
   - `spinlock` (uses `pgx_SpinLock*` functions)

This feature flag will hopefully make it easier to cross-compile pgx with only a small bit of compromises.  This feature flag was initially designed with [plrust](https://github.com/tcdi/plrust) user functions in mind, and they won't need access to any of the above.
… due to rust-lang/cargo#7160 and how if it were default we wouldn't be able to turn it off from a top-level `cargo test --all --no-default-features --features pg14`.
@eeeebbbbrrrr
Copy link
Contributor Author

This PR is slightly complicated by rust-lang/cargo#4753.

The "solution", if you can call it that, is for our pgx-tests/Cargo.toml to not have a default = [ "cshim" ] feature. And if we want to run the tests with the cshim turned on, we'll have to specify it explicitly.

9646bdf does that, assuming it passes CI.

@eeeebbbbrrrr
Copy link
Contributor Author

I'm content with this now. Merging.

@eeeebbbbrrrr eeeebbbbrrrr merged commit ae7fb7a into develop Dec 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants