Skip to content

Start on a common area for bootstraps, so that different bootstraps #991

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

Closed
wants to merge 1 commit into from

Conversation

kollivier
Copy link
Contributor

can share common code. This is a first step to fixing issue #988. Note that this new approach copies over bootstrap files on every build, so I think it may remove the need for --symlink-java-src, though I've left that in just in case I'm misunderstanding something. I can remove it if needed. BTW I did test performance and the impact is minimal.

Next step will be to build a BasePythonActivity that has any common functions that are used by, or could be useful to, multiple bootstraps.

@AndreMiras
Copy link
Member

Oh sweet thanks for the PR 🍻
So if I understood correctly your are copying over all the files for the different bootstraps? What happens when the bootstraps are slightly different, they won't be anymore right? I don't know if there's always a need for them to be different, but at least with the BasePythonActivity approach we have the choice to inherit and specialise, what do you think?
Do you want to give it a try to the second approach now?

@kollivier
Copy link
Contributor Author

kollivier commented Sep 16, 2018

BasePythonActivity is more like an additional step to help with sharing code, not a separate approach. This PR is about not keeping multiple copies of the same files in each separate backend, which would still be a problem if we added BasePythonActivity.

I think we can deal with backend-specific differences in shared files on a case-by-case basis. Worst-case, we can patch them, but I don't think it would come to that. In many cases, I think we could just refactor the code to offer some backend-specific handling with a flag or other method.

I'm interested in taking a stab at BasePythonActivity, but I probably won't have time in the near future to work on it. I think landing this patch is a good first step to start moving that process forward, and maybe once there's a shared space for backends, someone will feel it's easier to hack on a BasePythonActivity class. :) Love to hear your thoughts on my approach so far or if you guys see any issues with it!

@AndreMiras
Copy link
Member

OK I agree with the approach indeed, thanks!
I'm curious why your PR doesn't have any deleted files, we should have plenty of them right?
Also this is a bit cumbersome to test, since we need to test all bootstraps for regression 😕
Perhaps we should improve the test suite first to see if at least it compiles and next step would be to check for run time errors, but this is more tricky to achieve.

I've merged the merge conflicts and now it seems that your git mv got changed to git add ahah nice online github editor 😛 ...
If you don't have time for it give me write access to your branch and I'll try to fix and squash.

@kollivier
Copy link
Contributor Author

I've checked "Allow edits from maintainers", do I need to do something else in addition to that?

@AndreMiras
Copy link
Member

I think I actually need access to the branch of your fork rather than the pull request.
https://help.github.com/articles/committing-changes-to-a-pull-request-branch-created-from-a-fork/

That would probably allow me to do my rebasing/squashing magic 😄

@kollivier
Copy link
Contributor Author

Okay, sorry for the delay, I've invited you!

@AndreMiras AndreMiras force-pushed the bootstrap_refactor branch 2 times, most recently from f42f4c0 to f307718 Compare September 27, 2018 21:04
This is the first step to fix DRY across bootstraps, refs kivy#988
@AndreMiras
Copy link
Member

That's perfect, thanks for sharing. I've started the rebasing/squashing.
So basically now we have your changes in one single commit on top of the git log.
Next up is to make it works, then to make it right. Feel free to continue helping in these topics if you think there's something to test/add/fix and I can deal with the version control part 😄

Some of the things I'd like to address before finishing with that pull requests are #1382 and #1380.
Basically in #1382 I'd like to add the different bootstrap builds in our continuous integration. And in #1380 I'd like cover more code in unit test fashion (not yet sure how for bootstraps).

By the way, I made a back up of the branch in case I messed it up.

@ghost
Copy link

ghost commented Nov 5, 2018

Just as a note, I highly depend on this to be able to redo #1433 in a better, comprehensive and non-horrific way - and #1433 itself is highly relevant for proper Python 3 support (at least without kivy, not sure if that has something built-in that handles this). So until some functionality like this one is merged in first, it will be difficult for me to advance

@ghost
Copy link

ghost commented Nov 20, 2018

What's the progress / state of this pull request?

@AndreMiras
Copy link
Member

I think it still make sense, @kollivier already made his part, now it's on our side. We need to discuss if we want to integrate it this way. If we do I would fix the conflicts and merge, but I was busy on other things 😕

@ghost
Copy link

ghost commented Nov 20, 2018

I haven't looked at the details, but simply having a common folder where at least most of the files get copied out into all the separate bootstraps sounds like a rather good idea to me. (But then again, I have barely done work on the bootstraps so I'm not the best person to judge this) So thumbs up for the basic approach from me 👍

@ghost
Copy link

ghost commented Nov 26, 2018

@tito @inclement what's your input on this one? It would be lovely to get this in or something similar, so I can pick up work on #1433 and finally fix ctypes.util.find_library crashing for Python 3

@ghost
Copy link

ghost commented Nov 29, 2018

Weird, not sure what GitHub is on about, but I checked out the branch and rebased it onto master with no issue or no not-auto-resovled conflicts: https://github.com/JonasT/python-for-android/tree/bootstrap_refactor

I have yet to test it though. I will try to rebase my ctypes.util.find_library fix on top of this, and see if I can get things to build & run. This of course would be a huge waste of time if there wasn't some sort of plan to eventually merge this, so if that isn't the case, please let me know now

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I don't think removing the sdl2 bootstrap entirely makes any sense. I am currently reworking this to retain it

@ghost
Copy link

ghost commented Nov 29, 2018

Trying a reworked thing based on this here: #1496

AndreMiras added a commit that referenced this pull request Nov 30, 2018
Rework common bootstrap area based on kollivier's  work, refs #991
@kollivier
Copy link
Contributor Author

Superceded by the now-merged #1496!

@kollivier kollivier closed this Nov 30, 2018
@kollivier kollivier deleted the bootstrap_refactor branch November 30, 2018 20:25
@AndreMiras
Copy link
Member

I'm sorry we didn't make yours to the tree @kollivier. Thanks a mil for original initiative and the hard work you put in there 🍺
I do think however your point is still valid since we didn't yet kill the repeated code. But we might address it in a second round since we need to investigate what can be merged and what can't.

@ghost
Copy link

ghost commented Nov 30, 2018

@kollivier I put your name into the commit message, I hope that's fine with you! You did great work, very cool we could finally get this in

@kollivier
Copy link
Contributor Author

Yup, no worries! 👍 Thanks to you both for getting this past the finish line!

There is definitely still more work to be done, I have some customizations I want to merge upstream and this work makes doing so much easier. Specifically, one thing is that I made the python zip extraction into an AsyncTask in PythonActivity, so that it doesn't block the UI and we can show the load image sooner. I'd like to actually have the base code for that (along with some other common functionality) as a start on BasePythonActivity!

@AndreMiras
Copy link
Member

That sounds like a very great idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants