-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fixes package name problem and locale problem #959
Conversation
d93e022
to
fc6c188
Compare
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.
Looks good overall. Will need to verify that the naming is still as expected after the comments are addressed.
scripts/fetch_and_build_rpm
Outdated
if [ "${osname}" == 'Oracle' ]; then | ||
locale='C' | ||
elif [ "${osname}" == 'Fedora' ] || [ "${osname}" == 'CentOS' ]; then | ||
elif [ "${osname}" == 'Fedora' ] || [[ "${osname}" == 'CentOS' && "${os_release}" == 8* ]]; 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.
I guess it is time that we remove all references to Fedora in our scripts. This hurts readability, and there are a lot of leftover code that do not really have any value.
Can we create an issue and track this please?
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.
scripts/fetch_and_build_rpm
Outdated
LC_ALL=${locale} | ||
rpmdev-bumpspec -c "${msg}" "${builddir}/${pkgname}.spec" |
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.
I think these two statements are meant to be on the same line.
LC_ALL=${locale} | |
rpmdev-bumpspec -c "${msg}" "${builddir}/${pkgname}.spec" | |
LC_ALL=${locale} rpmdev-bumpspec -c "${msg}" "${builddir}/${pkgname}.spec" |
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.
You're right fixed as you suggested
scripts/fetch_and_build_rpm
Outdated
LC_ALL=${locale} | ||
rpmdev-bumpspec -c "${msg}" "${builddir}/${pkgname}.spec" |
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.
I think these two statements are meant to be on the same line.
LC_ALL=${locale} | |
rpmdev-bumpspec -c "${msg}" "${builddir}/${pkgname}.spec" | |
LC_ALL=${locale} rpmdev-bumpspec -c "${msg}" "${builddir}/${pkgname}.spec" |
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.
You're right fixed as you suggested
Can we also trigger some nightly and regular image builds so that we can see everything works as expected? |
Images should be created after merge of the PR to be able to see nightlies with the new images |
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.
All good after we update a stale comment
scripts/fetch_and_build_rpm
Outdated
fi | ||
sed -i -E "1i %global pkginfix ${package_prefix}" "${builddir}/${pkgname}.spec" | ||
fi | ||
|
||
case "${1}" in | ||
release) | ||
# add minor/major version to package name if using fancy versioning |
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.
This comment no longer makes sense. Can we update that with a simple explanation on why we do not have any steps for release?
Currently, naming strucuture for rpm is as below
citus_13-11.1.0.citus-0.0.git.20220731.5490c85.el7.x86_64.rpm
When you try to install the package as instructed in citusdata download page, it doesn't work since package name does not include version just after citus as below
citus110_13-11.0.5.citus-1.el7.x86_64.rpm
With the new development, package name will be like below
citus110_13-11.1.0.citus-0.0.git.20220731.5490c85.el7.x86_64.rpm
Additionally I found a bug for centos 7 packages related to locale set. Since C.utf8 is not among supported locales for centos 7, the error below appears in the logs
"sh: warning: setlocale: LC_ALL: cannot change locale (C.utf8): No such file or directory"
When I check the commit history, I found out that the intention to add C.utf8 for Centos is just for el/8 , Not for el/7
Commit hash for this change: e864e96