Skip to content

Rework common bootstrap area based on kollivier's work #1496

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.

Sign up for GitHub

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

Merged
merged 1 commit into from Nov 30, 2018
Merged

Rework common bootstrap area based on kollivier's work #1496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 31 additions & 5 deletions pythonforandroid/bootstrap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
from os.path import (join, dirname, isdir, splitext, basename)
from os import listdir
from os import listdir, walk, sep
import sh
import glob
import importlib
import os
import shutil

from pythonforandroid.logger import (warning, shprint, info, logger,
debug)
Expand All @@ -11,6 +13,26 @@
from pythonforandroid.recipe import Recipe


def copy_files(src_root, dest_root, override=True):
Copy link
Member

Choose a reason for hiding this comment

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

any reason not to use shutil.copytree ?

Copy link
Author

Choose a reason for hiding this comment

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

shutil.copytree can't override I think, or at least I am not aware of it. Also, according to Stack Overflow, it can't skip existing files either which is really needed here, and which is what the function does with override=False. So I don't want to rule out that this could be done with shutil functions somehow, but this seemed to be a good way to do it. I didn't write this code by the way, this is 99% @kollivier 's part - but I would have done something similar

Copy link
Member

Choose a reason for hiding this comment

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

ah right, it should work in python3 with a copy_function that ignore existing files, but that option is not present in python2, there is the ignore argument, probably with set intersection to ignore, but not sure how much simpler that would be. Nothing wrong with having a specific function anyway.

for root, dirnames, filenames in walk(src_root):
for filename in filenames:
subdir = root.replace(src_root, "")
if subdir.startswith(sep):
subdir = subdir[1:]
Copy link
Member

Choose a reason for hiding this comment

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

Minor suggestion/question:
I wonder if it's not cleaner to use something like os.path.normpath()?

>>> dest_root = '/foo/'
>>> subdir = 'bar/'
>>> os.path.join(os.path.normpath(dest_root), os.path.normpath(subdir))
'/foo/bar'

Which also covers he case were subdir by chance ends with two slashes.
So basically we could replace the three lines above by one:

subdir = os.path.normpath(root.replace(src_root, ""))

Copy link
Author

Choose a reason for hiding this comment

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

normpath won't remove a leading(!) slash, only a trailing one. It could still be used in addition, but I'm pretty certain the point of this code is to make sure subdir becomes a relative path, and normpath alone wouldn't achieve that

Copy link
Member

Choose a reason for hiding this comment

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

oh yes I misread as endwiths() 😅 so forget about it

dest_dir = join(dest_root, subdir)
if not os.path.exists(dest_dir):
os.makedirs(dest_dir)
src_file = join(root, filename)
dest_file = join(dest_dir, filename)
if os.path.isfile(src_file):
if override and os.path.exists(dest_file):
os.unlink(dest_file)
if not os.path.exists(dest_file):
shutil.copy(src_file, dest_file)
else:
os.makedirs(dest_file)


class Bootstrap(object):
'''An Android project template, containing recipe stuff for
compilation and templated fields for APK info.
Expand Down Expand Up @@ -77,6 +99,9 @@ def get_build_dir(self):
def get_dist_dir(self, name):
return join(self.ctx.dist_dir, name)

def get_common_dir(self):
return os.path.abspath(join(self.bootstrap_dir, "..", 'common'))

@property
def name(self):
modname = self.__class__.__module__
Expand All @@ -86,9 +111,10 @@ def prepare_build_dir(self):
'''Ensure that a build dir exists for the recipe. This same single
dir will be used for building all different archs.'''
self.build_dir = self.get_build_dir()
shprint(sh.cp, '-r',
join(self.bootstrap_dir, 'build'),
self.build_dir)
self.common_dir = self.get_common_dir()
copy_files(join(self.bootstrap_dir, 'build'), self.build_dir)
copy_files(join(self.common_dir, 'build'), self.build_dir,
override=False)
if self.ctx.symlink_java_src:
info('Symlinking java src instead of copying')
shprint(sh.rm, '-r', join(self.build_dir, 'src'))
Expand All @@ -109,7 +135,7 @@ def run_distribute(self):
@classmethod
def list_bootstraps(cls):
'''Find all the available bootstraps and return them.'''
forbidden_dirs = ('__pycache__', )
forbidden_dirs = ('__pycache__', 'common')
bootstraps_dir = join(dirname(__file__), 'bootstraps')
for name in listdir(bootstraps_dir):
if name in forbidden_dirs:
Expand Down
0 ...roid/bootstraps/sdl2/build/ant.properties → ...id/bootstraps/common/build/ant.properties
View file
Open in desktop
File renamed without changes.
0 ...roid/bootstraps/sdl2/build/jni/Android.mk → ...id/bootstraps/common/build/jni/Android.mk
View file
Open in desktop
File renamed without changes.
1 change: 1 addition & 0 deletions pythonforandroid/bootstraps/common/build/jni/application/Android.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
include $(call all-subdir-makefiles)
2 changes: 1 addition & 1 deletion .../bootstraps/sdl2/build/jni/src/Android.mk → ...mmon/build/jni/application/src/Android.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ include $(CLEAR_VARS)

LOCAL_MODULE := main

SDL_PATH := ../SDL
SDL_PATH := ../../SDL

LOCAL_C_INCLUDES := $(LOCAL_PATH)/$(SDL_PATH)/include

Expand Down
0 ...oid/bootstraps/sdl2/build/jni/src/start.c → .../common/build/jni/application/src/start.c
View file
Open in desktop
File renamed without changes.
0 ...ps/sdl2/build/templates/Service.tmpl.java → .../common/build/templates/Service.tmpl.java
View file
Open in desktop
File renamed without changes.
0 ...traps/sdl2/build/templates/build.tmpl.xml → ...aps/common/build/templates/build.tmpl.xml
View file
Open in desktop
File renamed without changes.
0 ...dl2/build/templates/custom_rules.tmpl.xml → ...mon/build/templates/custom_rules.tmpl.xml
View file
Open in desktop
File renamed without changes.
0 ...straps/sdl2/build/templates/kivy-icon.png → ...raps/common/build/templates/kivy-icon.png
View file
Open in desktop
File renamed without changes
0 ...s/sdl2/build/templates/kivy-presplash.jpg → ...common/build/templates/kivy-presplash.jpg
View file
Open in desktop
File renamed without changes
0 ...droid/bootstraps/sdl2/build/whitelist.txt → ...oid/bootstraps/common/build/whitelist.txt
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion ...s/webview/build/jni/src/Android_static.mk → ...ild/jni/application/src/Android_static.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ include $(CLEAR_VARS)

LOCAL_MODULE := main

LOCAL_SRC_FILES := YourSourceHere.c
LOCAL_SRC_FILES := start.c

LOCAL_STATIC_LIBRARIES := SDL2_static

Expand Down
4 changes: 2 additions & 2 deletions ...aps/service_only/build/jni/src/Android.mk → ...only/build/jni/application/src/Android.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ LOCAL_MODULE := main
# Add your application source files here...
LOCAL_SRC_FILES := start.c pyjniusjni.c

LOCAL_CFLAGS += -I$(LOCAL_PATH)/../../../../other_builds/$(PYTHON2_NAME)/$(ARCH)/python2/python-install/include/python2.7 $(EXTRA_CFLAGS)
LOCAL_CFLAGS += -I$(LOCAL_PATH)/../../../../../other_builds/$(PYTHON2_NAME)/$(ARCH)/python2/python-install/include/python2.7 $(EXTRA_CFLAGS)

LOCAL_SHARED_LIBRARIES := python_shared

LOCAL_LDLIBS := -llog $(EXTRA_LDLIBS)

LOCAL_LDFLAGS += -L$(LOCAL_PATH)/../../../../other_builds/$(PYTHON2_NAME)/$(ARCH)/python2/python-install/lib $(APPLICATION_ADDITIONAL_LDFLAGS)
LOCAL_LDFLAGS += -L$(LOCAL_PATH)/../../../../../other_builds/$(PYTHON2_NAME)/$(ARCH)/python2/python-install/lib $(APPLICATION_ADDITIONAL_LDFLAGS)

include $(BUILD_SHARED_LIBRARY)

Expand Down
0 ...vice_only/build/jni/src/Android_static.mk → ...ild/jni/application/src/Android_static.mk
View file
Open in desktop
File renamed without changes.
0 ...s/service_only/build/jni/src/pyjniusjni.c → ...ly/build/jni/application/src/pyjniusjni.c
View file
Open in desktop
File renamed without changes.
0 ...straps/service_only/build/jni/src/start.c → ...ce_only/build/jni/application/src/start.c
View file
Open in desktop
File renamed without changes.
4 changes: 2 additions & 2 deletions ...otstraps/webview/build/jni/src/Android.mk → ...view/build/jni/application/src/Android.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ LOCAL_MODULE := main
# Add your application source files here...
LOCAL_SRC_FILES := start.c pyjniusjni.c

LOCAL_CFLAGS += -I$(LOCAL_PATH)/../../../../other_builds/$(PYTHON2_NAME)/$(ARCH)/python2/python-install/include/python2.7 $(EXTRA_CFLAGS)
LOCAL_CFLAGS += -I$(LOCAL_PATH)/../../../../../other_builds/$(PYTHON2_NAME)/$(ARCH)/python2/python-install/include/python2.7 $(EXTRA_CFLAGS)

LOCAL_SHARED_LIBRARIES := python_shared

LOCAL_LDLIBS := -llog $(EXTRA_LDLIBS)

LOCAL_LDFLAGS += -L$(LOCAL_PATH)/../../../../other_builds/$(PYTHON2_NAME)/$(ARCH)/python2/python-install/lib $(APPLICATION_ADDITIONAL_LDFLAGS)
LOCAL_LDFLAGS += -L$(LOCAL_PATH)/../../../../../other_builds/$(PYTHON2_NAME)/$(ARCH)/python2/python-install/lib $(APPLICATION_ADDITIONAL_LDFLAGS)

include $(BUILD_SHARED_LIBRARY)

Expand Down
0 ...raps/sdl2/build/jni/src/Android_static.mk → ...ild/jni/application/src/Android_static.mk
View file
Open in desktop
File renamed without changes.
0 ...straps/webview/build/jni/src/pyjniusjni.c → ...ew/build/jni/application/src/pyjniusjni.c
View file
Open in desktop
File renamed without changes.
0 .../bootstraps/webview/build/jni/src/start.c → ...webview/build/jni/application/src/start.c
View file
Open in desktop
File renamed without changes.
6 changes: 5 additions & 1 deletion pythonforandroid/toolchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,9 @@ def wrapper_func(self, args):
user_ndk_ver=self.ndk_version,
user_ndk_api=self.ndk_api)
dist = self._dist
bs = Bootstrap.get_bootstrap(args.bootstrap, ctx)
# recipes rarely change, but during dev, bootstraps can change from
# build to build, so run prepare_bootstrap even if needs_build is false
Copy link
Member

Choose a reason for hiding this comment

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

The comment says we run prepare_bootstrap() but I don't see that we do 😕
Also why are we calling Bootstrap.get_bootstrap() for?

Copy link
Author

Choose a reason for hiding this comment

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

Interesting! Might be an oversight by @kollivier and maybe it was supposed to have a ctx.prepare_bootstrap(bs) right after? That would explain why bs is unused and there's the confusing comment there. I don't reuse any of the build layout for subsequent builds, so I assume that's why I didn't run into a situation in my tests where this was relevant

Copy link
Contributor

@kollivier kollivier Nov 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The code wasn't used in the original PR, either, so whatever I was intending to do, I never actually made it happen. I think it can just be removed.

From the comment, I suspect that during some tests I found a case where switching the bootstrap didn't trigger a rebuild. Even if that was still true, it's likely not a common case (I was triggering it no doubt because I was testing with different bootstraps!) and can be fixed with a clean rebuild.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, thanks for cleaning this up and getting it running against the latest code! :)

Copy link
Author

Choose a reason for hiding this comment

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

@kollivier you're welcome, and this would have been infinitely more work without your foundation, so thanks again for starting this!

if dist.needs_build:
if dist.folder_exists(): # possible if the dist is being replaced
dist.delete()
Expand Down Expand Up @@ -183,7 +186,8 @@ def build_dist_from_args(ctx, dist, args):

ctx.dist_name = bs.distribution.name
ctx.prepare_bootstrap(bs)
ctx.prepare_dist(ctx.dist_name)
if dist.needs_build:
ctx.prepare_dist(ctx.dist_name)

build_recipes(build_order, python_modules, ctx)

Expand Down