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

Add a script to CI to check dynamic symbols in Android binary #8351 #9981

Merged
merged 1 commit into from Mar 19, 2016

Conversation

@cengizIO
Copy link
Contributor

cengizIO commented Mar 14, 2016

Tries to fix #8351.

This is meaningful only with the PR to servo/saltfs

Cross PR: servo/saltfs#249


This change is Review on Reviewable

@@ -0,0 +1,12 @@
#!/bin/bash

ACTUAL_SYMBOLS=$(arm-linux-androideabi-objdump -T target/arm-linux-androideabi/debug/libservo.so | grep "D " | grep "UND" | awk '{ print $NF; }' | tr '\n' ';')

This comment has been minimized.

Copy link
@cengizIO

cengizIO Mar 14, 2016

Author Contributor

I'm not sure about the path of arm-linux-androideabi-objdump tool

This comment has been minimized.

Copy link
@cengizIO

cengizIO Mar 14, 2016

Author Contributor

According to buildbot/master/master.cfg#L219, libservo.so is built in debug mode, with default mach target path.

@Ms2ger
Copy link
Contributor

Ms2ger commented Mar 14, 2016

I would much rather see this written in python.

@cengizIO
Copy link
Contributor Author

cengizIO commented Mar 14, 2016

@Ms2ger actually I just tried to follow the pattern in etc/ci directory.

But if you think python is more maintainable, I can convert it.

@aneeshusa
Copy link
Member

aneeshusa commented Mar 14, 2016

To make this more robust, we should be agnostic as to the order of the symbols, i.e. for bash adding a call to sort in the pipeline after the awk command, or in Python, using sets instead of lists.

@wafflespeanut wafflespeanut removed their assignment Mar 15, 2016
@cengizIO
Copy link
Contributor Author

cengizIO commented Mar 17, 2016

@Ms2ger @aneeshusa I've converted it to a python script which is sort order agnostic and much more readable.

@cengizIO
Copy link
Contributor Author

cengizIO commented Mar 17, 2016

@larsbergstrom This doesn't have an assignee at the moment.

#/usr/bin/env python

import sys, re, subprocess
from sets import Set

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Mar 17, 2016

Member

Sets are built-in since Python 2.6 and you don't need to import them; the sets module isn't even available in Python 3. See https://docs.python.org/2/library/stdtypes.html#set and https://docs.python.org/3/library/stdtypes.html#set.

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Mar 17, 2016

Member

Also, I think a frozenset would be appropriate for the allowed_symbols variable.

if m != None:
actual_symbols.add(m.group(2))

difference = allowed_symbols.difference(actual_symbols)

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Mar 17, 2016

Member

I believe we need to do actual_symbols.difference(allowed_symbols), which can written with syntax sugar as actual_symbols - allowed_symbols. This will take the actual symbols and remove the symbols we know are OK, leaving the symbols that are not OK.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Mar 17, 2016

@aneeshusa Thanks for handling the python parts of the review! I would also add that there is some output from our tidy tool:

Checking files for tidiness...

./etc/ci/check_dynamic_symbols.py:1: E265 block comment should start with '# '

./etc/ci/check_dynamic_symbols.py:3: E401 multiple imports on one line

./etc/ci/check_dynamic_symbols.py:11: E126 continuation line over-indented for hanging indent

./etc/ci/check_dynamic_symbols.py:14: E123 closing bracket does not match indentation of opening bracket's line

./etc/ci/check_dynamic_symbols.py:18: E711 comparison to None should be 'if cond is not None:'

./etc/ci/check_dynamic_symbols.py:1: incorrect license

@cengizIO I apologize for not shepherding this review! I didn't notice that it was not assigned and will make sure that sort of thing doesn't slip through the cracks again. Thanks also for your patience in rewriting what was originally supposed to be a cheesy shell script into a much more robust python script.

@cengizIO
Copy link
Contributor Author

cengizIO commented Mar 17, 2016

@aneeshusa and @larsbergstrom thanks for the feedback!

Please check latest revision.

This is probably my 3rd python script ever.

And I didn't know about ./mach test-tidy until you mentioned. It's cool 😎

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Mar 19, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Mar 19, 2016

📌 Commit 135429f has been approved by larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Mar 19, 2016

Testing commit 135429f with merge 1db70db...

bors-servo added a commit that referenced this pull request Mar 19, 2016
Add a script to CI to check dynamic symbols in Android binary #8351

Tries to fix #8351.

This is meaningful only with the PR to servo/saltfs

Cross PR: servo/saltfs#249

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9981)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 19, 2016

@bors-servo bors-servo merged commit 135429f into servo:master Mar 19, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
bors-servo added a commit to servo/saltfs that referenced this pull request Mar 22, 2016
Call a new script to check dynamic symbols in Android binary #8351

Tries to fix #8351.

This is meaningful only with the PR to servo/servo

Cross PR: servo/servo#9981

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/249)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.