ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Petr Ivanov <mr.wei...@gmail.com>
Subject Re: Apache Ignite RPM packaging (Stage II)
Date Mon, 02 Apr 2018 14:00:05 GMT


> On 29 Mar 2018, at 12:32, Max Shonichev <mshonich@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/

Mime
View raw message