Skip to content
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

#957 RPM with killTimeout #960

Merged
merged 4 commits into from
Apr 11, 2017
Merged

#957 RPM with killTimeout #960

merged 4 commits into from
Apr 11, 2017

Conversation

mr-git
Copy link
Contributor

@mr-git mr-git commented Apr 10, 2017

No description provided.

@lightbend-cla-validator

Hi @mr-git,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

@muuki88
Copy link
Contributor

muuki88 commented Apr 10, 2017

Thanks for your lightning fast pull request 🚀

The rpm / sysvinit-rpm tests checks this line. Can you adjust the test as well?

For more information on how to run the tests locally see the developer guide: https://github.com/sbt/sbt-native-packager/wiki/Developer-Guide

@mr-git
Copy link
Contributor Author

mr-git commented Apr 10, 2017

hm.. it failed with (*:checkSpecFile) java.lang.AssertionError: assertion failed: rpm addService() scriptlet missing or incorrect - but I didn't affect the rpm-test.spec in any way!

My change is in start-rpm-template, and only src/sbt-test/rpm/sysvinit-rpm/target/rpm/buildroot/etc/init.d/rpm-test file should get affected by this change, isn't it?

How change in start template could affect RPM spec file which contains RPM setup instructions?

Could it be that src/sbt-test/rpm/sysvinit-rpm/build.sbt lines 75..87 is wrongly formatted? Seems that there are some issues with spaces and tabs, similar situation is for stopService() check too (lines: 112..125).

Test expects (there are tabs):

        |addService() {
        |    app_name=$1
        |    if hash update-rc.d >/dev/null 2>&1; then
        |		echo "Adding $app_name to service management using update-rc.d"
        |		update-rc.d $app_name defaults
        |    elif hash chkconfig >/dev/null 2>&1; then
        |		echo "Adding $app_name to service management using chkconfig"
        |		chkconfig --add rpm-test
        |		chkconfig $app_name on
        |    else
        |		echo "WARNING: Could not add $app_name to autostart: neither update-rc nor chkconfig found!"
        |    fi
        |}

But packager generates (no tabs):

addService() {
    app_name=$1
    if hash update-rc.d >/dev/null 2>&1; then
        echo "Adding $app_name to service management using update-rc.d"
        update-rc.d $app_name defaults
    elif hash chkconfig >/dev/null 2>&1; then
        echo "Adding $app_name to service management using chkconfig"
        chkconfig --add rpm-test
        chkconfig $app_name on
    else
        echo "WARNING: Could not add $app_name to autostart: neither update-rc nor chkconfig found!"
    fi
}

@mr-git
Copy link
Contributor Author

mr-git commented Apr 10, 2017

BTW, there are no tests that check the startup script, at least I didn't find any.
In 02519e1a7636489723518c4208f0ee6c03c6a3ad - com/typesafe/sbt/packager/archetypes/systemloader/systemv/loader-functions were changed - tabs were replaced with spaces.

Is there a way to run those CI tests on HEAD?

@muuki88 muuki88 added the rpm label Apr 11, 2017
@muuki88
Copy link
Contributor

muuki88 commented Apr 11, 2017

@mr-git you are awesome!

My change is in start-rpm-template, and only src/sbt-test/rpm/sysvinit-rpm/target/rpm/buildroot/etc/init.d/rpm-test file should get affected by this change, isn't it?

But packager generates (no tabs):

I removed all traces of tabs in the bash scripts this weekend, but didn't mind the tests. Thanks a lot for fixing my mess I did on the weekend. 02519e1 wasn't the best commit I ever made 😕

BTW, there are no tests that check the startup script, at least I didn't find any

That's very likely. I still haven't had time to figure out a good way to inspect these scripts. ATM I'm playing around with shellcheck. Because checking the contents of a script only helps to verify the correct variables are replaced, but not if it's correct.

@mr-git
Copy link
Contributor Author

mr-git commented Apr 11, 2017

I am still trying to figure out all places :( I hope last commit will make it better, though scripted rpm/systemd-rpm fails for me locally with exit code 1

@mr-git
Copy link
Contributor Author

mr-git commented Apr 11, 2017

https://travis-ci.org/sbt/sbt-native-packager/jobs/220865842:

javac 1.7.0_75
before_install
0.00s$ if [[ "$TRAVIS_OS_NAME" = "osx" ]]; then brew update; brew install xz; fi
128.18s$ ./sbt ++$SCALA_VERSION --warn update compile test:compile
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x00007f37f9dd8697, pid=2627, tid=139875981805312
#
# JRE version: OpenJDK Runtime Environment (7.0_75-b13) (build 1.7.0_75-b13)
# Java VM: OpenJDK 64-Bit Server VM (24.75-b04 mixed mode linux-amd64 compressed oops)
# Derivative: IcedTea 2.5.4
# Distribution: Ubuntu 12.04 LTS, package 7u75-2.5.4-1~precise1
# Problematic frame:
# V  [libjvm.so+0x426697]  CompactibleFreeListSpace::compact()+0x2c7
#
# Failed to write core dump. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again
#
# An error report file with more information is saved as:
# /home/travis/build/sbt/sbt-native-packager/hs_err_pid2627.log
# [ timer expired, abort... ]
/home/travis/.travis/job_stages: line 53:  2627 Aborted                 (core dumped) ./sbt ++$SCALA_VERSION --warn update compile test:compile
The command "./sbt ++$SCALA_VERSION --warn update compile test:compile" exited with 134.

What does it mean?

Is there a way to restart the build?

@mr-git
Copy link
Contributor Author

mr-git commented Apr 11, 2017

yay, it is green!

@muuki88
Copy link
Contributor

muuki88 commented Apr 11, 2017

Is there a way to restart the build?

Yes. Travis CI crashes 1 out of 20 times. Sorry, I didn't see this earlier.

yay, it is green!

✨ ✨ awesome! Thanks for fixing my mess I left on the weekend. You're awesome ❤️

@mr-git
Copy link
Contributor Author

mr-git commented Apr 11, 2017

You are welcome!
I have only one last question: when I can expect the next milestone build? 😄

@mr-git
Copy link
Contributor Author

mr-git commented Apr 11, 2017

This is my first pull request - how this will get merged? Will you do the merge or should I somehow init/request it?

@@ -86,7 +86,7 @@ start() {

stop() {
echo -n $"Stopping $prog: "
killproc -p $PIDFILE $prog
killproc -p $PIDFILE -d ${{kill_timeout}} $prog
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find any documentation of the -d parameter. Do you have any resources for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://gist.github.com/edouard-lopez/4d1c1d007156b662c64e#file-functions-centos-sh-L284 - this is for functions.centos.sh
https://bash.cyberciti.biz/guide//etc/init.d/functions - here search for killproc - it has line:

Usage: killproc [-p pidfile] [ -d delay] {program} [-signal]

Copy link
Contributor Author

@mr-git mr-git Apr 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on our centos servers in /etc/init.d/functions there is:

#...
# A function to stop a program.
killproc() {
        local RC killlevel= base pid pid_file= delay try binary=

        RC=0; delay=3; try=0
        # Test syntax.
        if [ "$#" -eq 0 ]; then
                echo $"Usage: killproc [-p pidfile] [ -d delay] {program} [-signal]"
                return 1
        fi
#... 
        if [ "$1" = "-d" ]; then
                delay=$(echo $2 | awk -v RS=' ' -v IGNORECASE=1 '{if($1!~/^[0-9.]+[smhd]?$/) exit 1;d=$1~/s$|^[0-9.]*$/?1:$1~/m$/?60:$1~/h$/?60*60:$1~/d$/?24*60*60:-1;if(d==-1) exit 1;delay+=d*$1} END {printf("%d",delay+0.5)}')
                if [ "$?" -eq 1 ]; then
                        echo $"Usage: killproc [-p pidfile] [ -d delay] {program} [-signal]"
                        return 1
                fi
                shift 2
        fi
#...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification. I guess this will be okay.

@muuki88
Copy link
Contributor

muuki88 commented Apr 11, 2017

I have only one last question: when I can expect the next milestone build?

I'll try to push it this week. I originally planned to fix one other issue, but won't have the time.

This is my first pull request - how this will get merged? Will you do the merge or should I somehow init/request it?

I'll do the merging :) Github has a pretty nice interface for doing this directly on the PR. No work from you side required :)

@muuki88 muuki88 merged commit 68e95f7 into sbt:master Apr 11, 2017
@muuki88
Copy link
Contributor

muuki88 commented Apr 11, 2017

Thanks for all your work and the positive communication :) Hope you'll continue doing great open source stuff 👍

@muuki88
Copy link
Contributor

muuki88 commented Apr 13, 2017

@mr-git release in 1.2.0-M9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants