-
Notifications
You must be signed in to change notification settings - Fork 13
[PKG-1258]: Add OIDC packaging files for PG18 #1026
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
Conversation
dutow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pg_oidc_builder.sh seems like a copy pasted and modified script for many components, in it's current state that seems difficult to maintain and impossible to properly review. It probably needs some serious cleanup/refactoring, but that's not the point of this PR.
| cd $WORKDIR | ||
| RHEL=$(rpm --eval %rhel) | ||
| ARCH=$(echo $(uname -m) | sed -e 's:i686:i386:g') | ||
| if [ -f /opt/rh/devtoolset-7/enable ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this script has lots of old copy-paste leftovers, like devtoolset-7/llvm-toolset-7 are ancient and I am not sure why we would want to enable them, we definitely should clean these up at some point, but that probably shouldn't be as part of this pull requets, so I'm just adding this as a note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in my previous comment, we can address such cleanups of code after Q1'26 releases.
| if [[ "${RHEL}" -eq 9 ]]; then | ||
| if [[ "$COMPONENT" == "postgresql" || "$COMPONENT" == "pg_repack" ]]; then | ||
| if [[ "$COMPONENT" == "postgresql" || "$COMPONENT" == "pg_repack" || "$COMPONENT" == "pg_oidc" ]]; then | ||
| INSTALL_LIST+="gcc-toolset-14 " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again not part of this change, but I do wonder why some components require different gcc toolsets on different distros, that seems strange
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These components' builds are failing without this dependency. Maybe we can dive deeper in it later, like whether these components be build with a common gcc toolset.
| INSTALL_LIST+="gdal311-devel proj96-devel geos313-devel pcre2-devel " | ||
| fi | ||
| if [[ "$COMPONENT" == "pg_oidc" ]]; then | ||
| INSTALL_LIST+="libstdc++-static " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed? I thought this is part of the devtoolset for the build machine, and it has no runtime dependency when the package is installed on the system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is not a runtime dependency but a build time dependency. pg_oidc build is failing without it.
Yes, you are right. This same script format is being used across all the components. However, I had cleaned up it a bit before Q4'25 release, but it still needs some more refining. We can address this after Q1'26 releases. |
https://perconadev.atlassian.net/browse/PKG-1258