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

fix(report/handlers): accept more semver versions #111

Merged
merged 10 commits into from
Sep 26, 2017

Conversation

bassosimone
Copy link
Contributor

The current problem we have, specifically, is that since v1.2.0 the
mobile apps emit full semver including also the build.

This requires several parts of our infrastructure to improve the
regexp being used so to validate also this kind of input.

See also: measurement-kit/measurement-kit#1388

An earlier version of this diff was blessed by @hellais on Slack and
since then I just changed comments.

The current problem we have, specifically, is that since v1.2.0 the
mobile apps emit full semver including also the build.

This requires several parts of our infrastructure to improve the
regexp being used so to validate also this kind of input.

See also: measurement-kit/measurement-kit#1388

An earlier version of this diff was blessed by @hellais on Slack and
since then I just changed comments.
@bassosimone
Copy link
Contributor Author

bassosimone commented Sep 26, 2017

The build is failing like this:

function killitwithfire () {
    trap - ALRM
    kill -ALRM $prog 2>/dev/null
    kill -9 $! 2>/dev/null && exit 0
}

function waitforit () {
    trap "killitwithfire" ALRM
    sleep $1& wait
    kill -ALRM $$
}

waitforit $1& prog=$! ; shift ;
+prog=6047
+shift
+waitforit
trap "killitwithfire" ALRM INT
+trap killitwithfire ALRM
+trap killitwithfire ALRM INT
"$@"& wait $!
+wait
+sleep
+wait 6049
sleep: missing operand
Try 'sleep --help' for more information.
RET=$?
+RET=0
if [[ "$(ps -ef | awk -v pid=$prog '$2==pid{print}{}')" != "" ]]; then
    kill -ALRM $prog
    wait $prog
fi
+kill -ALRM 6046
ps -ef | awk -v pid=$prog '$2==pid{print}{}'
killitwithfire
++killitwithfire
++trap - ALRM
++kill -ALRM 6047
++kill -9 6049
++ps -ef
++awk -v pid=6047 '$2==pid{print}{}'
/home/travis/.travis/job_stages: line 57:  6046 Segmentation fault      (core dumped) ./.travis.test.sh 30 ./bin/oonib

The command "chmod +x .travis.test.sh && ./.travis.test.sh 30 ./bin/oonib" exited with 139.

I don't understand very well what it could be, and it seems like this is something related to the way in which travis is handling the test. I am going to restart it to see if it's deterministic (I actually hope so).

@bassosimone
Copy link
Contributor Author

I am experiencing more errors with the build, like that the keyserver doesn't know the key. 😡

@bassosimone
Copy link
Contributor Author

Alright, it seems it's time to play golf.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 78.469% when pulling 9e0eca4 on fix/version_regexp into 03d738b on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 78.527% when pulling ec91e00 on fix/version_regexp into 03d738b on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 78.469% when pulling 9bb6db5 on fix/version_regexp into 03d738b on master.

@bassosimone
Copy link
Contributor Author

I really hate these coveralls annoying comments that don't serve any purpose.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 78.527% when pulling d09c1c5 on fix/version_regexp into 03d738b on master.

@bassosimone
Copy link
Contributor Author

In the end, I decided to rewrite the test that caused a segfault.

I believe the problem appeared now that travis has upgraded its infrastructure to 14.04: the previous travis build was for 03d738b and occurred on Apr, 6 when travis was still using 12.04.

The original script was brilliant: it did two nested waits to make sure the running process was either killed gracefully or with fire. But probably was too brilliant and triggered some edge case.

I did not want to wrestle too much with travis. Also, reading the script, it seems to me it's fine to rewrite it such that, if we cannot kill the background process, the build will hang and then fail (on travis).

I guess having the local build hang and the travis build fail is good enough for our purposes.

@bassosimone
Copy link
Contributor Author

Alright, I have read the diff once more. I am going to self bless this as good, given that @hellais already blessed the diff improving the regexp on Slack and that the test changes "look good to me".

theres-no-place-like-127 0 0 1_www fullhdwpp com_

@bassosimone bassosimone merged commit 8ccede1 into master Sep 26, 2017
@bassosimone bassosimone deleted the fix/version_regexp branch September 26, 2017 11:21
bassosimone added a commit to ooni/probe-ios that referenced this pull request Sep 26, 2017
bassosimone added a commit to ooni/probe-android that referenced this pull request Sep 26, 2017
bassosimone added a commit to ooni/probe-android that referenced this pull request Sep 26, 2017
bassosimone added a commit to ooni/probe-ios that referenced this pull request Sep 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants