> On 29 Mar 2018, at 12:32, Max Shonichev <mshon...@yandex.ru> wrote:
>
>
>
> 1. About packaging/package.sh
>
> 1.1. I personally dislike the use of bashism, e.g. like reserved keywords,
> double '[[', double '((', but that's ok
> for targeted distro (RHEL/Centos). The only requirement is please add more
> comments in tough places
> to improve code readability and maintanbility.
Will try to get rid of them as much as possible, but SH is really outdated
scripting language and most all of the systems now support BASH, even Windows :)
>
>
> 1.2. few code style comments:
>
>>> name=$(cat /etc/*release | grep ^NAME | sed -r 's|.*"(.*)".*|\1|')
>
> add
>
> | head -n 1
>
> or else it will fail in case more than one file with *release mask in /etc
I disagree. Script will not fail, just cat every file matched by mask and then
pass everything as one single stdout.
>
>
> 1.3. rework prepEnv
>
>>> if [ ! -z installCmd ]; then
>
> this will fail miserably, replace with
>
> if [ ! -z $installCmd ]; then
Fixed
>
> 1.3.1. however running package.sh under Ubuntu still would fail due to
> $installCmd is unpacked to 'apt --no-install-recommends'
> and -z 'apt --no-install-recommends' would always return false.
The main purpose of checking that variable ${installCmd} is not empty — because
it is instantiated empty and will stay empty if OS detection won’t be able to
define OS and pre-build software installation and environment preparation will
be skipped.
>
> 1.3.2. and still, when apt/yum is installed and rpm/rpm-build is not, the
> whole prepEnv would fail to ensure all
> build-time requirements are installed, because it only checks for apt/yum.
I suppose that apt / yum is always installed by default because it is default
package manager for corresponding OSes.
I did not get where is error here.
>
> 1.3.3. installing requires root privileges. However, no checks for $EUID nor
> running apt/yum with 'sudo' present.
> Please, add this check, otherwise running package.sh under unprivileged user
> with no installed build-time dependencies
> will fail with very undescriptive errors.
Check for ${EUID} will mean that whole build package process will run under
root, which is undesirable.
Added INFO message that explains necessity for super user privileges and added
sudo command to root-required commands.
>
>
> 1.4. getBin
>
> 1.4.1. I don't like detecting target apache ignite package version by
> parsing embedded changelog. This might fail
> when any package text description would contain '*' in the first char of the
> string. Either move changelog to separate
> Changelog file and %include it in .spec or parse for, e.g. '^Version:
> [0-9\.]+'
>
>>> igniteVersion=$(cat rpm/SPECS/apache-ignite.spec | grep '^*' | head -1 |
>>> sed -r 's|.*\ -\ (.*)-.*|\1|’)
Replaced with parsing of main version
>
> 1.4.2.
>
>>> ls ${binName} && binPreparedFlag=true || true
>
> why not use
>
> if [ ! -f $binName ];
Indeed this variant is much better, fixed.
>
> 1.4.3. 'curl' is used, but not checked as build-time dependency.
Added to build dependencies (via prepEnv function)
>
> 1.4.4. we try to find and download 'apache-ignite-fabric-$igniteVersion',
> that's ok for <2.4 releases, but in future
> we would need to find and download 'apache-ignite-$igniteVersion' too.
> Better add this check as a fallback option, e.g.
> try to use curl with both urls in correct order.
>
> if curl --output /dev/null --silent --head --fail "$url"; then
> echo "URL exists: $url"
> else
> echo "URL does not exist: $url"
> fi
1. Packages did not exist before 2.4.0.
2. Build script does not provide opportunity to build packages of different
from .spec versions and is considered to be part of packaging build process
(specifications and packaging script are modified together) — a shortcut, thus
relying on current (same commit) state for building.
>
> 1.4.5. also strange, but may be ok that we download
> apache-ignite-fabric-2.4.0
Fabric removal task is not yet merged to master.
Will be supported in its reimplementation
>
>
> 2. about apache-ignite.spec
>
> 2.1. fix typo
>
>>> Apache Ignite's optinal libs and integrations
Fixed
>
> 2.2. fix description
>
>>> C++ files necessary for using Apache Ignite
>
> platform files are not necessary for Ignite, only for integration with .NET,
> afaik?
Introduced alternative description
>
> 2.3. Tough place with 'Obsoletes: apache-ignite < 2.5.0'
>
> I understand the current logic behind splitting apache-ignite to several
> .rpm's and setting obsolete for the old
> apache-ignite.
>
> However, for future releases, e.g. if/when we handover this task to another
> engineer, it is not quite obvious that one
> must (or must not?) update version in 5 different places, including
> changelog. Please add a few lines about
> 'How to package next (new) version of Apache Ignite RPMs' to Confluence.
Everything that relies on package version is mapped to %{version} variable and
defined only once.
Other versions in dependencies directives will stay the same in future
specification updates.
Task for description of RPM specification update process, I guess, is out of
scope of current issue. However I agree with necessity of such documentation in
future and will create corresponding task in the nearest time.
>
> 2.4. firewalld / firewall-cmd is optionally used if installed, but not set
> in the dependency. It is OK, but may be we
> add this as a post-install note, like we do recommendation messages in node
> logs.
Firewall-CMD rules from package is an override for current installation rather
then necessity in some firewall implementations required for service usage.
>
>
> 3. service scripts : we'll should add a test suite for it, overall looks
> pretty solid.
Agree. Will create corresponding task in the nearest time
>
>
>
>
>
>
>
> --
> Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
