Rework common bootstrap area based on kollivier's work #1496
Conversation
|
Edit: no longer relevant / solved |
2 similar comments
|
Edit: no longer relevant / solved |
|
Edit: no longer relevant / solved |
ghost
mentioned this pull request
ghost
changed the title
|
Okay, got it to work, and especially Can someone help me test a little? Or if nobody uses these bootstraps anyway, I suggest this is merged soon because it's quite likely to lead to conflicts with further bootstrap changes since things are moved around quite a bit |
| @@ -11,6 +13,26 @@ | |||
| from pythonforandroid.recipe import Recipe | |||
|
|
|||
|
|
|||
| def copy_files(src_root, dest_root, override=True): | |||
There was a problem hiding this comment.
any reason not to use shutil.copytree ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
Sorry, still had a small mistake: the wrong Edit: did another rebase on top of this current version and it builds my app fine. Also, travis is happy now, yay! 👍 |
| @@ -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 | |||
There was a problem hiding this comment.
The comment says we run prepare_bootstrap() but I don't see that we do 😕
Also why are we calling Bootstrap.get_bootstrap() for?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also, thanks for cleaning this up and getting it running against the latest code! :)
There was a problem hiding this comment.
@kollivier you're welcome, and this would have been infinitely more work without your foundation, so thanks again for starting this!
| for filename in filenames: | ||
| subdir = root.replace(src_root, "") | ||
| if subdir.startswith(sep): | ||
| subdir = subdir[1:] |
There was a problem hiding this comment.
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, ""))There was a problem hiding this comment.
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
There was a problem hiding this comment.
oh yes I misread as endwiths() 😅 so forget about it
There was a problem hiding this comment.
I gave it a try and it works.
However as discussed on Discord, this doesn't kill the DRY we have all over the place like start.c.
We agree that start.c files diverge slightly across bootstraps, but that's more likely because we added fixes and features to some places and didn't also copied over to the other.
So at the end we still need to investigate and address it.
|
I suggest that the missing |
|
(I suppose it could also still be added in a minor merge request afterwards) |
ghost
deleted the
ghost
mentioned this pull request
|
Follow-up pull request for outstanding possible issues & fixes with the common area handling code itself is here: #1499 |
This is a rework of a common shared bootstrap area based on @kollivier 's work started here: #991
Ignore comments below about errors, got it to work! 😄