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

cargo-pgx cross compilation support #981

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

Conversation

jaskij
Copy link
Contributor

@jaskij jaskij commented Dec 24, 2022

Work in progress for adding cross compilation support to cargo-pgx, as discussed in #955

@jaskij
Copy link
Contributor Author

jaskij commented Dec 24, 2022

Now, the questions is, how do I test it? Since cargo-pgx must be in the same version as pgx required by the plugin being built.

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 will need some work (well, it's a draft, so I think you know that), but is a really good start. Here's some preliminary feedback.

cargo-pgx/src/command/install.rs Outdated Show resolved Hide resolved
cargo-pgx/src/command/package.rs Outdated Show resolved Hide resolved
cargo-pgx/src/command/schema.rs Outdated Show resolved Hide resolved
cargo-pgx/src/command/schema.rs Outdated Show resolved Hide resolved
cargo-pgx/src/command/install.rs Outdated Show resolved Hide resolved
cargo-pgx/src/command/install.rs Outdated Show resolved Hide resolved
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.

Ah, two super minor idiom nits too.

cargo-pgx/src/command/cross_options.rs Outdated Show resolved Hide resolved
cargo-pgx/src/command/cross_options.rs Outdated Show resolved Hide resolved
@jaskij
Copy link
Contributor Author

jaskij commented Jan 14, 2023

@thomcc I primarily marked this as a draft to discuss the interface - mostly around what is passed as an argument, and what is passed as an environmental variable.

Meanwhile, I'm going to document some caveats here:

  • host and target need compatible versions of PostgreSQL (not sure how much they need to much, if it must be the exact same version, or just same minor)
  • PG_CONFIG needs to be an absolute path
  • setting the path to pg_config is an absolute mess - there's PGX_PG_CONFIG, PG_CONFIG, --pg-config and I added --host-pg-config. My testing so far indicates that PG_CONFIG does not need to be set, but it's still early.

@jaskij jaskij marked this pull request as ready for review January 14, 2023 21:36
@jaskij
Copy link
Contributor Author

jaskij commented Jan 14, 2023

I have absolutely no clue why that one test is failing, but it seems to be misconfigured, especiall since on my PC (with PG15 installed via cargo pgx init) that same command does not fail.

  [shim for PG v15] [stderr] Makefile:26: /usr/lib/postgresql/15/lib/pgxs/src/makefiles/pgxs.mk: No such file or directory
  [shim for PG v15] [stderr] make: *** No rule to make target '/usr/lib/postgresql/15/lib/pgxs/src/makefiles/pgxs.mk'.  Stop.

@jaskij
Copy link
Contributor Author

jaskij commented Jan 15, 2023

A quick note on how to actually cross-compile with cargo-pgx, seems I forgot to post it yesterday.

  1. Read Thom's guide
  2. Additional requirements:
    -- host and target PostgreSQL (including server dev stuff and pg_config)
    -- user space qemu for your target architecture (eg qemu-aarch64)
  3. Make pg_config wrapper (below)
  4. Call PGX_PG_CONFIG_PATH=~/pg_config_wrapper cargo pgx package --sysroot=/usr/local/oecore-x86_64/sysroots/cortexa53-crypto-poky-linux --pg-config=/home/jaskij/pg_config_wrapper --target=aarch64-unknown-linux-gnu

pg_config wrapper:

#!/usr/bin/env sh
set -x
SDK_DIR="/usr/local/oecore-x86_64"
SYSROOT_DIR="$SDK_DIR/sysroots"
OE_HOST="x86_64-poky-linux"
OE_TARGET="cortexa53-crypto-poky-linux"
HOST_SYSROOT="$SYSROOT_DIR/$OE_HOST"
TARGET_SYSROOT="$SYSROOT_DIR/$OE_TARGET"

$HOST_SYSROOT/usr/bin/qemu-aarch64 -L $TARGET_SYSROOT  $TARGET_SYSROOT/usr/bin/pg_config "$@"

@jaskij
Copy link
Contributor Author

jaskij commented Jan 15, 2023

As is, schema generation builds the extension for the host so that it can generate the bindings.
This isn't a good solution, but is a workable one. Sadly, schema generation feels like
black magic to me.

The current solution has two, huge and obvious, drawbacks:

  • requiring host PostgreSQL, at the very least in similar version to target's
  • doubling the already long build times

I'm not sure if it's in-scope for this MR though, and I would need a lot of guidance to implement it.

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.

I think running the schema generation on the host is reasonable. It's not ideal, surely, but it seems unlikely that SQL will vary between targets (this is also something we can document).

Overall this mostly looks good, but I'm testing it out today and that might dig something up.

I think it needs some docs, but those don't have to block things, since this is a pretty niche use case (and I can handle them, when I find the time).

Do you mind resolving the conflicts?

@jaskij
Copy link
Contributor Author

jaskij commented Jan 28, 2023

@thomcc I see that your work on cross compilation was merged, that's what created the conflict. Do you think there is something I should be looking into or integrating with? Or can I just simply resolve the conflict and be done with it?

@thomcc
Copy link
Contributor

thomcc commented Feb 1, 2023

You don't need to integrate with it really. There's some redundancy, but it largely handles a few cases that this completes (specifically the no cshim case where you don't care about cargo-pgx — it's basically for https://github.com/tcdi/plrust).

Resolving the conflict is sufficient, thanks.

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 great. It will need some further documentation, but I'm working on an update/completion to CROSS_COMPILE.md anyway, and I can integrate it afterwards.

@jaskij
Copy link
Contributor Author

jaskij commented Feb 1, 2023

@thomcc conflict resolved.

If you're working on update to CROSS_COMPILE.md already, my instructions are condensed in this comment.

A cleaned up version of pg_config_wrapper:

#!/usr/bin/env sh
set -x
TARGET_SYSROOT="/usr/local/oecore-x86_64/sysroots/cortexa53-crypto-poky-linux"

qemu-aarch64 -L $TARGET_SYSROOT  $TARGET_SYSROOT/usr/bin/pg_config "$@"

@thomcc
Copy link
Contributor

thomcc commented Feb 2, 2023

Can you rustfmt and push an update?

@jaskij
Copy link
Contributor Author

jaskij commented Feb 2, 2023

Damnit, thought it was already formatted. Will do as soon as I'm at my PC.

@jaskij
Copy link
Contributor Author

jaskij commented Feb 3, 2023

@thomcc done.

@thomcc
Copy link
Contributor

thomcc commented Feb 3, 2023

Hmm, can you check the CI error? I didn't see this locally when I tested, but I think I was testing on pg14 rather than 15.

@eeeebbbbrrrr
Copy link
Contributor

Putting this PR on "pause" until we have some extra cycles over here in pgxland to make sure this doesn't actually break anything and is otherwise complete enough to accomplish the stated goal.

@workingjubilee
Copy link
Member

If this gets rebased I think we can take a look at landing it?

@jaskij
Copy link
Contributor Author

jaskij commented Jul 25, 2023

I don't have the time or energy to get on this now, but might have it at the beginning of September, feel free to ping me then if I don't show up.

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.

4 participants