Opened 11 years ago

Closed 11 years ago

#578 closed defect (fixed)

rasdl fails on CentOS 6.4

Reported by: Dimitar Misev Owned by: Alireza
Priority: major Milestone: 9.0
Component: rasdl Version: development
Keywords: Cc: ungarj, Peter Baumann, abeccati
Complexity: Medium

Description

service rasdaman initdb fails with:

Using error text file: /usr/share/rasdaman//errtxts
rasdl: rasdaman schema and database manipulation tool, rasdaman - on
base DBMS pgsql -- generated on 10.12.2013 10:55:34.
Creating base RASBASE...terminate called after throwing an instance of
'std::out_of_range'
  what():  basic_string::substr
- -bash: line 1: 10832 Aborted                 /usr/bin/rasdl -c
- --connect RASBASE
inserting type definitions to rasdaman with rasdl failed

Change History (28)

comment:1 by Dimitar Misev, 11 years ago

Owner: changed from Dimitar Misev to Alireza
Status: newassigned

Looks like an issue of version check, for some reason the version in the RPMs is not set.

Joachim, how are you building the RPMs? Is maybe git missing on the system where you build them, or is the .git dir missing from the repo? Do you git clone/pull rasdaman before building RPMs?

I have just compiled rasdaman on CentOS 6.5 and the version is properly output v9.0.0beta2-gfecde34:

$ ./rasdl --help
Using error text file: /home/rasdaman/install/share/rasdaman//errtxts
No error texts file found at: /home/rasdaman/install/share/rasdaman//errtxts

rasdl: rasdaman schema and database manipulation tool, rasdaman v9.0.0beta2-gfecde34 on base DBMS pgsql -- generated on 11.12.2013 17:11:01.
...

However doing this on the RPM rasdl gives no version:

$ rasdl --help
Using error text file: /usr/share/rasdaman//errtxts
rasdl: rasdaman schema and database manipulation tool, rasdaman - on base DBMS pgsql -- generated on 10.12.2013 10:55:34.

comment:2 by Dimitar Misev, 11 years ago

Reassigning this to Alireza, as he needs to fix the DatabaseIf::rmanverToLong() method in reladminif/databaseif.pgc to take into consideration that the version may not be defined, in which case either some default version should be used. E.g. in the beginning of the method

#ifndef RMANVERSION
#define RMANVERSION "v9.0.0-unknown"
#endif

Also Joachim may provide further details on how is it possible that the version is not set at all. The version should be defined in the rasdaman source tree after make, in $RMANSRC/version.h. It is set by the $RMANSRC/Makefile.am

comment:3 by Dimitar Misev, 11 years ago

Cc: abeccati added

comment:4 by ungarj, 11 years ago

Thanks Dimitar for looking into this. I'm using the following git commands to get the source code:

git fetch origin <TAG>
git checkout <TAG>
git archive --format=tar --prefix=rasdaman-<VERSION>/ <TAG> | gzip > ../rasdaman-<VERSION>.tar.gz

then I get the spec and init file from the source:

cp packaging/rpm/rasdaman.init.in ../
cp packaging/rpm/rasdaman.spec ../../SPECS/

For the beta versions however I had to rename the source tar.gz as well as the included folder from rasdaman-9.0.0-beta2 to rasdaman-9.0.0 because building would fail otherwise.

Then I boot into the building VM. Git is not installed on the VM. Should it? Why?

comment:5 by Dimitar Misev, 11 years ago

Can you please do it like this:

git fetch origin <TAG>
git checkout <TAG>
cd ..
cp -a rasdaman rasdaman-<VERSION>
tar cfz rasdaman-<VERSION>.tar.gz rasdaman-<VERSION>

comment:6 by Dimitar Misev, 11 years ago

And yes git needs to be installed where you build the packages, it's used to compute that version.

comment:7 by Dimitar Misev, 11 years ago

@Alireza you should fix the default version setting in source:Makefile.am when git is not present on the system.

comment:8 by ungarj, 11 years ago

Dimitar, I would put the entire git history as well in the tar.gz if not using "git archive".

Furthermore, what is git required for on the building machine? If so, it should be stated somewhere.

I'll try a different naming scheme proposed by OpenStack to prevent renaming the source folder in the tar.gz

comment:9 by Dimitar Misev, 11 years ago

Yes, you should put the git history in the tar.gz (are you trying to save some insignificant network bandwith, or? :))

git is used in source:Makefile.am to determine the rasdaman version when compiling it, with this command

echo '#define RMANVERSION "'`(git for-each-ref --sort='*authordate' --format='%(tag)' refs/tags 2>/dev/null | tail -n 1 2>/dev/null || echo v9.0.0) | sed s/-[0-9]*//1`-`git describe | awk -F '-' '{ print $$NF; }' || echo 0`'"' > version.h.tmp

comment:10 by ungarj, 11 years ago

I'm just trying to keep it clean and simple with as few dependencies and data as possible :)

./git currently has 86MB, the whole directory without .git ~150MB. There is no reason why we should blow up the size with insignificant data.

The git command in the Makefile would be ok if you have nightly builds but not for full or beta releases. Isn't there any other way to grab this information?

in reply to:  10 comment:11 by Dimitar Misev, 11 years ago

Replying to ungarj:

There is no reason why we should blow up the size with insignificant data.

It's not insignificant, we need it to compute the version. If you want you can try to set the version into version.h manually before making the tar.gz so that you can remove .git..

The git command in the Makefile would be ok if you have nightly builds but not for full or beta releases.

Why is it ok for nightly builds but not for other releases? We should try to be consistent in the versions reported by rasql, rasdl, rasserver, etc.

comment:12 by ungarj, 11 years ago

Sorry, but just to compute the version string (the first of these three lines)

#define RMANVERSION "v9.0.0beta2-beta2"
#define GCCVERSION "gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-4)"
#define GCCTARGET "x86_64-redhat-linux"

I'm somewhat hesitant in putting the whole git history (in this case 86MB).

BTW the version string looks a bit odd. Is it ok? Can't this be simply extracted from the source archive or the spec file?

comment:13 by Dimitar Misev, 11 years ago

On my machine version.h has:

#define RMANVERSION "v9.0.0beta2-gb8d1db1"
#define GCCVERSION "gcc (Debian 4.7.2-5) 4.7.2"
#define GCCTARGET "x86_64-linux-gnu"

The part after the dash is the git commit, so that every executable can be associated to a git commit from which it was compiled.

comment:14 by Dimitar Misev, 11 years ago

Why are you concerned by the 86MB? It takes too much CPU time to compress them?

comment:15 by ungarj, 11 years ago

Why including an extra 86MB (which adds ~50% of the package size) to get one single line of information? Whatever, it remains highly dubious but probably I am missing something here.

What concerns me is that by running it manually, our RMANVERSION is different…

Anyway, I'll try it for now with including everything.

One more comment: we should have the git requirements on the building machine documented somewhere, I might not be the last one getting caught in this.

comment:16 by Dimitar Misev, 11 years ago

Relatively yes, absolutely it's just another second or two for compression. And this is not the final package, but just the source archive that rpmbuild uses to build the packages. The RPM packages will NOT contain any .git.

I'm not sure about the different RMANVERSION, how did you run it manually?

comment:17 by ungarj, 11 years ago

It's included in the src.rpm as well I think.

We created the Makefile and ran "make version".

comment:18 by Dimitar Misev, 11 years ago

What does this give:

git describe

comment:19 by Dimitar Misev, 11 years ago

I have submitted patch to fix version generation when git is not available.

comment:20 by ungarj, 11 years ago

Thanks dimitar, now I've got working builds. However, for the next builds I'll try out your patch to avoid git.

initdb works, so I'm ok with closing the ticket.

comment:21 by Dimitar Misev, 11 years ago

My patch is not meant to be used as an excuse to remove the .git dir, I submitted it to make sure that there is a meaningful fallback in case git is not installed or .git is corrupted/missing.

This ticket is not completely fixed yet, we are waiting for Alireza to fix the rasdaman code (comment:2)

comment:22 by Alireza, 11 years ago

I checked DatabaseIf::rmanverToLong() method with two input "v9.0.0-unknown" (suggested by comment 2) and "v9.0.0beta2-gfecde34" (from version.h file), for both case it returns the same value 9000.

if we use the preprocessors to define the rmanversion (in case it is not defined), then for testing we need to remove/rename it in version.h (or from Makefile.am). Leaving it empty will not activate the #ifndef directive. If we change it then the directive needs to be added to other files beside databaseif.pgc.

comment:23 by Dimitar Misev, 11 years ago

Ok so I suggest to have a default of "v9.0.0-unknown" when the RMANVERSION is empty or not defined.

The goal is to make rmanverToLong() resistant to potentially unexpected strings, so that segfaults and other errors are prevented. So please add appropriate checks in the code, e.g. for NULL pointers before doing anything with them.

comment:24 by Peter Baumann, 11 years ago

looking at the rmanverToLong() I find that itmakes assumption which may not be true: in case find_first_of() does not find anything the result is undefined. This should be corrected so that there is always a sane fallback.

Same thing in the Makefile: target "version" assumes a few things, like git being available. Should be corrected to have a fallback value as well:

  • if git then proceed as given
  • else set RMANVERSION to default (beware: "-unknown" will make rmanverToLong() fail - may be what we want, or not, depending on the fix above).

So in both cases we need to introduce defensive programming.

comment:25 by Dimitar Misev, 11 years ago

Peter, I submitted a patch for the Makefile which fixes those assumptions.

comment:26 by Alireza, 11 years ago

rmanverToLong() does not fail with "v9.0.0-unknown" and return 9000.

I checked "git for-each-ref —sort='*authordate' —format='%(tag)' refs/tags 2>/dev/null | tail -n 1
echo v9.0.0" in the Makefile.am when git is not installed and it does not return "v9.0.0" as default value instead it returns empty. Maybe that is the problem.

comment:27 by Dimitar Misev, 11 years ago

Yes, my patch (see on the patchmanager "[PATCH] ticket:578 - fix generation of version when git is not installed") fixes that issue.
The Makefile is _fixed_.
We are talking about making the code more foolproof, see

The goal is to make rmanverToLong() resistant to potentially unexpected strings, so that segfaults and other errors are prevented. So please add appropriate checks in the code, e.g. for NULL pointers before doing anything with them.

and

looking at the rmanverToLong() I find that itmakes assumption which may not be true: in case find_first_of() does not find anything the result is undefined. This should be corrected so that there is always a sane fallback.

comment:28 by Dimitar Misev, 11 years ago

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.