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

PG17 Support #820

Closed
wants to merge 10 commits into from
Closed

PG17 Support #820

wants to merge 10 commits into from

Conversation

philkra
Copy link
Contributor

@philkra philkra commented Oct 23, 2024

Imported through #819 /cc @Jo0

@syvb
Copy link
Member

syvb commented Oct 23, 2024

The latest pgrx requires 1.82; you need to bump the Rust toolchain version.

@Jo0
Copy link
Contributor

Jo0 commented Oct 23, 2024

#821 to bump toolchain to 1.82.0

@graveland
Copy link
Contributor

graveland commented Oct 23, 2024

pgrx's upstream is using 1.81.0... are we sure 1.82.0 is required?

I'm going to push a commit on this branch to see if it helps, feel free to drop/modify it.

Nope, too bad :\

@Jo0
Copy link
Contributor

Jo0 commented Oct 23, 2024

Shame I can't make commits directly to the branch. Could you try with it bumped to 1.82?

@graveland
Copy link
Contributor

Shame I can't make commits directly to the branch. Could you try with it bumped to 1.82?

Pushed again with 82, It's the same error for 81 and 82:

error[E0658]: use of unstable library feature 'result_option_inspect'
   --> /usr/local/cargo/registry/src/index.crates.io-6f17d22bba15001f/pgrx-bindgen-0.12.6/src/build.rs:114:19
    |
114 |         enum_name.inspect(|name| match name.strip_prefix("enum").unwrap_or(name).trim() {
    |                   ^^^^^^^
    |
    = note: see issue #91345 <https://github.com/rust-lang/rust/issues/91345> for more information

@Jo0
Copy link
Contributor

Jo0 commented Oct 23, 2024

I'm curious now if we are even using the correct image for running the tests.

@graveland
Copy link
Contributor

I haven't actually tested it locally- if you have, and it works, it sounds like purely a CI issue unfortunately.

@graveland
Copy link
Contributor

Confirmed: CI is attempting to build with rustc: 1.74.0 (79e9716c9 2023-11-13)

@Jo0
Copy link
Contributor

Jo0 commented Oct 23, 2024

I think I see what's going on with the image build and tagging now. Seems the default tag (Example2) we are pushing to the CR for these runs are timescaledev/toolkit-builder-test, but the image tag being pulled for these CI Tests are tagged timescaledev/toolkit-builder Example

  workflow_dispatch:
    inputs:
      tag-base:
        description: 'Push image to DockerHub with this base tag (remove "-test" enable)'
        required: false
        # Repeating the default here for ease of editing in the github actions form.  Keep in sync with below.
        default: timescaledev/toolkit-builder-test

Do we need to do a manual run of Build CI Image / build against this branch but tagged timescaledev/toolkit-builder instead of timescaledev/toolkit-builder-test via the run parameters/variables?

@zilder
Copy link
Contributor

zilder commented Oct 23, 2024

Btw I was building this branch locally and was able to get past the pgrx-bindgen compilation. There is tons of warnings and errors though regarding the necessity to implement ArgAbi and RetBox. I will take a look at these.

This container is built in CI with the current release, so use that one
when testing.
@Jo0
Copy link
Contributor

Jo0 commented Oct 29, 2024

@zilder how are things going? I've been fighting WSL2 in the free time I've had trying to help out with the compilation errors.

@zilder
Copy link
Contributor

zilder commented Oct 30, 2024

Hey @Jo0, it took me some time to wrap my head around macros-based code generation in toolkit. With pgrx 0.12+ it seems most issues come from new requirements for PostgresType trait implementation. I'm making some progress now, though there are still things I haven't figured out yet.

@zilder
Copy link
Contributor

zilder commented Nov 6, 2024

Merged as part of #824

@zilder zilder closed this Nov 6, 2024
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.

5 participants