Opened 9 years ago

Closed 8 years ago

#875 closed defect (fixed)

properties files get unduly modified by installation procedure

Reported by: Peter Baumann Owned by: Bang Pham Huu
Priority: minor Milestone: 9.1.x
Component: build system Version: development
Keywords: Cc: Dimitar Misev
Complexity: Medium

Description

the "make install" script for handling the petascope.properties and log4j.properties file can create a new file. While it attempts to copy over old settings some stuff, like comments, may get lost. The preocedure is meant for getting in new keys/values, which per se is good. A better approach, however, is to keep the old file and just append any newly introduced variables.

PS: in the script, this text is unclear:

"service existing service file, will not modify it"

Attachments (2)

0001-ticket-875-properties-files-get-unduly-modified-by-i.patch (1.9 KB ) - added by Bang Pham Huu 8 years ago.
0001-ticket-875-properties-files-get-unduly-modified-by.patch (1.8 KB ) - added by Bang Pham Huu 8 years ago.
New update

Download all attachments as: .zip

Change History (14)

comment:1 by Dimitar Misev, 8 years ago

Component: undecidedautotools
Milestone: Future9.1.x
Owner: changed from Peter Baumann to Bang Pham Huu
Status: newassigned

Bang, please make sure the existing petascope.properties file is backed up before it's modified.

comment:2 by Bang Pham Huu, 8 years ago

Dimitar: agree (I can see you make backup petascope.properties before 9 in make install), but I don't know what is called "new" variable in here (new with one but old with others) (so the best way is compare with user's file, if they don't have property then I will add). I hope understand correctly (in fact, I also did it with Jetty properties (start_embedded_petascope and java_server)).

comment:3 by Dimitar Misev, 8 years ago

As discussed today, it's probably best to take that logic for updating the configuration file out of the Makefile.am, and put it in a script which will give us more power (e.g. python or bash).

comment:4 by Bang Pham Huu, 8 years ago

ok, I got it, user will configured everything in petascope.properties (log4j.properties) in Rasdaman source code first and my script will get old configuration from user and modify the new petascope.properties last time before overwrite (with backup) to $RMANHOME/etc.

comment:5 by Bang Pham Huu, 8 years ago

Resolution: fixed
Status: assignedclosed

The patch has been submitted as it has been validated carefully and accepted both by Dimitar, I will close this ticket here.

comment:6 by Dimitar Misev, 8 years ago

Resolution: fixed
Status: closedreopened

There is a slight bug in the update_properties.sh script: it creates petascope.properties backups on every make install. So I end up with a bunch of petascope.properties*.bak in $RMANHOME/etc, even though the petascope.properties file has not changed at all.

No backup should be left in etc/ if nothing has been updated.

Also the permissions of the petascope.properties are not right, they should be same like the other config files (-rw-r—r—):

-rw-r--r-- 1 dimitar dimitar  711 Nov 29 09:18 log-server.conf
-rwxr-xr-x 1 dimitar dimitar 7441 Dec  2 17:37 petascope.properties
-rwxr-xr-x 1 dimitar dimitar 7441 Dec  2 17:37 petascope.properties.12-02-2015_17-37-14.bak

comment:7 by Bang Pham Huu, 8 years ago

Ok, Dimitar, actually a lot of things has added from the original purpose so I did not take care to check both file are identical first. A patch has been submitted and also fix the permission to 644 as you want. When the patch is accepted I will close this again.

comment:8 by Dimitar Misev, 8 years ago

Thanks for the quick fix! Please submit for review first next time, I had to go to the patchmanager to see the code.

+#2.3 Check if NEW and OLD file are the same (no need to create a backup and do anything)
+logn "Checking the difference between Old and New configuration files..."
+diff=$(diff "$NEW" "$OLD")
+if [[ $diff == "" ]]; then
+   echo "Your old configuration is already up to date."
+   ok
+fi
+echo "Done."

$diff should be in quotes in the if statement; "Checking the difference bet…" is not needed log output. It's better like this:

+#2.3 Check if NEW and OLD file are the same (no need to create a backup and do anything)
+cmp --quiet "$NEW" "$OLD"
+if [ $? -eq 0 ]; then
+   log "The existing configuration is already up to date."
+   ok
+fi

comment:9 by Bang Pham Huu, 8 years ago

Sorry for inconvenience as I thought this is too small so don't push to "codereview", so I upload the patch manager to here (please check, if it is passed then I will upload to patch manager tomorrow - so you can delete the earlier patch before. Thanks).

http://rasdaman.org/attachment/ticket/875/0001-ticket-875-properties-files-get-unduly-modified-by-i.patch

comment:10 by Dimitar Misev, 8 years ago

Ok Bang, please test your fix first, it doesn't look like you did that at all. And remove the

logn "Checking the difference between Old and New configuration files..."

No rush, it doesn't need to be done right now in the night of course.

Last edited 8 years ago by Dimitar Misev (previous) (diff)

by Bang Pham Huu, 8 years ago

New update

comment:11 by Bang Pham Huu, 8 years ago

Hi Dimitar,

it should not be rush of course, but I also run update_petascope.properties few times before submitting a patch (and work in good behavior). I could not get your idea about it did not work as you want (it just compare Old and New configuration and if it is the same then exit NO_ERROR - do you want to add something for more detail?).

I submit a new patch without the line you don't want to see and of course run with "make install" inside petascope directory few times alo, please download and recheck.

http://www.rasdaman.org/attachment/ticket/875/0001-ticket-875-properties-files-get-unduly-modified-by.patch

comment:12 by Bang Pham Huu, 8 years ago

Resolution: fixed
Status: reopenedclosed

I will close as Dimitar has accepted and allow me to submit a patch. Thanks.

Note: See TracTickets for help on using tickets.