Fix strip in bootstrap by j-devel · Pull Request #1607 · kivy/python-for-android

j-devel · 2019-01-25T05:14:40Z

Due to hardcoding, the arch-dependent strip command is not invoked when building for arch other than --arch=armeabi-v7a (e.g. strip not detected when --arch=x86).

This PR fixes

the problem above, and
the case of applying strip erroneously when the file name is '' (an empty string).

… to hardcoding

ghost · 2019-01-25T11:25:06Z

Nice 👍 I just checked, STRIP indeed is set in pythonforandroid/archs.py making this a much nicer solution rather than hardcoding it.

However, archs.py already adds --strip-unneeded into the STRIP command right now. But I'm not sure that's nice behavior to rely on, so I think it's not bad to bake it in addition - but it might result in the option being specified twice. If that doesn't actually break things in practice I don't think that is a problem, but that should be tested

j-devel · 2019-01-26T07:40:49Z

Thanks for reviewing code. I further considered and tested the following point:

it might result in the option being specified twice.

As a result, I think the possibility of doubly applying STRIP is none. Here's my findings.

The STRIP of our concern is part of strip_libraries() and this function takes care of only python-core and android-jni related shared objects, which are specifically,

DIST/_python_bundle/_python_bundle/modules/*.so
DIST/libs/ARCH/*.so

And these shared objects are NOT stripped devoid of this function.

On the other hand, shared objects belonging to regular recipes are independent of
strip_libraries() because they reside under DIST/_python_bundle/site-packages/ (e.g. DIST/_python_bundle/site-packages/jnius/jnius.so).

I performed a test build with --arch=x86, got stripped shared objects (that are stripped only once by strip_libraries()), and verified Python running without errors on Android.

ghost · 2019-01-26T09:30:06Z

As a result, I think the possibility of doubly applying STRIP is none.

Sure, but that's not what I suggested. I think the --strip-unneeded option might be specified/added twice in the single STRIP command that runs. Probably not an issue, just something I noticed

j-devel · 2019-01-26T11:21:58Z

Excuse me, I was talking about different thing 😓

Now, for example, when env['STRIP'] is 'i686-linux-android-strip --strip-unneeded', the following line ignores all the flags originally existing in env['STRIP'].

strip = env['STRIP'].split(' ')[0]  # strip is now 'i686-linux-android-strip'

So we won't get --strip-unneeded option twice.

ghost · 2019-01-26T11:54:39Z

@j-devel regarding split(' ')[0]: I oppose that idea, that seems very problematic to me since it's not unthinkable the strip command contains other options that are needed. (e.g. I'm pretty sure it's not completely unthinkable a more unusual compiler toolchain might require something like compiler-name --strip to invoke a strip tool) So that seems like a bad idea to me.

I was more thinking of considering where --strip-unneeded should actually be added. After all, the problem appears to be that this isn't completely obvious, hence (possibly, I just guessed from the code) done twice. But this shouldn't hold back this pull request, since it's more of a nitpick. But please don't add split(' ')[0], that seems like far too much into the other direction than is good for anything

pythonforandroid/bootstrap.py

ghost · 2019-01-26T21:34:46Z

pythonforandroid/bootstrap.py

-            return
-        strip = sh.Command(strip)
+        tokens = env['STRIP'].split(' ')
+        strip = reduce(lambda cmd, t: cmd.bake(t), tokens, sh.Command(tokens.pop(0)))


Ok I played around with this, and it appears this can be written as:

tokens = env['STRIP'].split(' ') strip = sh.Command(tokens[0]).bake(tokens[1:])

I would suggest that would be a little more readable

(This also works when len(token) == 1 and consequently tokens[:1] == [], I tested that special case)

Sorry I really didn't want to go on about this forever from my initial duplicate option nitpick, I hope I'm not wearing you out too much 😬 but the reduce/lambda variant is a little hard to read

Additional note: as written above I just noticed the space char splitting could break things.

tokens = shlex.split(env['STRIP']) strip = sh.Command(tokens[0]).bake(tokens[1:])

This should work correctly (requires import shlex at the beginning of the file, which is a core/stdlib module as far as I'm aware)

I really appreciate your support 😄

I like your simpler version and tested it fine. (But, in

(This also works when len(token) == 1 and consequently tokens[:1] == [], I tested that special case)

I am sure you are meaning tokens[1:] == [] not [:1]).

I am pushing a new commit...

inclement

Looks like there are some pep8 issues breaking travis, would you be able to fix that too? It shouldn't be anything complicated.

inclement · 2019-01-30T13:27:53Z

pythonforandroid/bootstrap.py

-            return
-        strip = sh.Command(strip)
+        tokens = shlex.split(env['STRIP'])
+        strip = sh.Command(tokens[0]).bake(tokens[1:])


It looks like this still assumes that that env['STRIP'] will split to a list of length at least two, which seems not necessarily true. Could you add a check to only bake the extra tokens if they exist?

Added the check in 7ad0207.

Looks like there are some pep8 issues breaking travis

I would like to look into this issue soon and come back.

@j-devel @inclement .bake works fine with an empty list. the if condition is not necessary, the above code works fine even for a single argument. (array slices outside of the array's range don't result in an error either, it just results in an empty list)

ghost · 2019-01-30T19:45:45Z

pythonforandroid/bootstrap.py

-        strip = sh.Command(strip)
+        tokens = shlex.split(env['STRIP'])
+        strip = sh.Command(tokens[0])
+        if len(tokens) > 1:


have you tested this without the if and a single argument/len(tokens) == 1? I tested bake with an empty list and it worked fine, I'm pretty sure this conditional is not required

Oh, you're quite right, I misremembered what list slice syntax would do outside the bounds of the list. It's fine to revert this.

@Jonast Yes, I tested the len(tokens) == 1 case, and it basically works. But, I found that .bake() raises a complaint when it's argument is [] (see the following REPL test).

>>> import sh >>> sh.Command('strip').bake([]) __main__:1: UserWarning: Empty list passed as an argument to '/opt/local/bin/strip'. If you're using glob.glob(), please use sh.glob() instead.

If we want to eliminate this warning, I think we still need the if check. Thoughts? @Jonast @inclement

@j-devel oh weird, have never seen that (maybe I got a different version?). In that case definitely keep the check in

no longer applicable

AndreMiras · 2019-01-31T08:42:41Z

pythonforandroid/bootstrap.py

            info('Python was loaded from CrystaX, skipping strip')
            return
        env = arch.get_env()
-        strip = which('arm-linux-androideabi-strip', env['PATH'])


Since which is not used anymore, would you mind removing the import to please the CI 🙏

pythonforandroid/bootstrap.py:12:1: F401 'pythonforandroid.util.which' imported but unused

Done. Sorry about the CI. Thanks for your help.

inclement · 2019-01-31T11:21:56Z

Great, thanks!

j-devel added 2 commits January 25, 2019 11:32

Fix strip failing (when arch is other than armeabi-v7a, e.g. x86) due…

7a515d0

… to hardcoding

Fix applying strip when the file name is ''

a2e63c9

ghost previously requested changes Jan 26, 2019

View reviewed changes

pythonforandroid/bootstrap.py Outdated Show resolved Hide resolved

Use strip that is truthfully reconstructed from env['STRIP']

73d13ad

ghost reviewed Jan 26, 2019

View reviewed changes

Simplify strip command construction and use shlex.split

8cd69dc

inclement requested changes Jan 30, 2019

View reviewed changes

Add a check to bake strip command's options only when they exist

7ad0207

ghost reviewed Jan 30, 2019

View reviewed changes

AndreMiras reviewed Jan 31, 2019

View reviewed changes

Remove unused import of pythonforandroid.util.which

06f2d39

inclement approved these changes Jan 31, 2019

View reviewed changes

inclement merged commit 8c1d5c8 into kivy:master Jan 31, 2019

j-devel deleted the fix-strip-bootstrap branch January 31, 2019 13:55

Conversation

j-devel commented Jan 25, 2019

Uh oh!

ghost commented Jan 25, 2019

Uh oh!

j-devel commented Jan 26, 2019

Uh oh!

ghost commented Jan 26, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-devel commented Jan 26, 2019

Uh oh!

ghost commented Jan 26, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j-devel Jan 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inclement left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jan 30, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jan 30, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inclement commented Jan 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ghost commented Jan 26, 2019 •

edited by ghost

Loading

ghost commented Jan 26, 2019 •

edited by ghost

Loading

j-devel Jan 27, 2019 •

edited

Loading

ghost Jan 30, 2019 •

edited by ghost

Loading

ghost Jan 30, 2019 •

edited by ghost

Loading