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

android_new: fix force_build option #1006

Merged
merged 1 commit into from
Dec 15, 2018
Merged

android_new: fix force_build option #1006

merged 1 commit into from
Dec 15, 2018

Conversation

ZingBallyhoo
Copy link
Contributor

@ZingBallyhoo ZingBallyhoo commented Feb 3, 2017

In current master, the force-build argument is defined, but never used. Small patch to fix it, and clean up a bit of PEP8.

I have also added the config option to Buildozer here: kivy/buildozer#464

Copy link
Member

@inclement inclement left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! I'll merge it once I've tested it.

Did you check that the force_build option works? Although the get_distribution function handles it, I can't remember if it actually will behave the right way under all circumstances. I'll try to check this as well.

join(self.ctx.get_python_install_dir(), 'bin', 'python.host'))
self.ctx.hostpython = join(self.ctx.get_python_install_dir(), 'bin', 'python.host')

if isfile(join(self.ctx.get_python_install_dir(), 'bin', 'python.host')):
Copy link
Member

Choose a reason for hiding this comment

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

Am I right in reading that the only change here is skipping the copy if the target already exists? If so, it would be preferable to clean it up a little more and put only the sh.cp call in the if block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be better now

Copy link
Member

Choose a reason for hiding this comment

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

Thanks :)

@ZingBallyhoo
Copy link
Contributor Author

ZingBallyhoo commented Feb 3, 2017

@inclement AFAIK the option is working fine, as I am currently using it.

@ZingBallyhoo
Copy link
Contributor Author

ZingBallyhoo commented Feb 4, 2017

@inclement Wait never mind, I broke things while refactoring Just fixed. Should be working for real now. (it was working before refactor)

@inclement
Copy link
Member

I tested and was going to merge this but actually found that the formatting changes create a lot of unnecessarily long lines, in some places removing existing line breaks. Would you be interested in splitting this into a separate force_build fix and a formatting PR? Alternatively, I can pull the force_build fix out manually.

@ZingBallyhoo
Copy link
Contributor Author

@inclement Only the fix for force-build is there now.

@inclement inclement added this to the 0.5 milestone May 11, 2017
@inclement inclement modified the milestones: 0.6, 0.5 Aug 26, 2017
@inclement inclement modified the milestones: 0.6, 0.7 Nov 19, 2017
@AndreMiras
Copy link
Member

Hi @ZingBallyhoo I'm really sorry we didn't make it to the tree and now the PR is conflicting 😢
Do you mind giving it a last try and we would merge since @inclement already reviewed and said it was OK.
The PR is pretty straight forward, but we'd like to keep you in the authoring.
Basically all we need is your pythonforandroid/toolchain.py change, the other one with PEP8 fixes you can drop. So simply squash your commits or recreate a new PR and we would merge pretty fast this time.

@AndreMiras
Copy link
Member

I've rebased to master and squashed the commit.
Now it's really just addressing the force_build option and only that.
Will merge if the build goes green.
Thanks again for initiating the pull request 🍻

@AndreMiras AndreMiras merged commit 598918a into kivy:master Dec 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants