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

Unify start.c of all bootstraps to one file #1500

Merged
merged 1 commit into from Dec 10, 2018
Merged

Unify start.c of all bootstraps to one file #1500

merged 1 commit into from Dec 10, 2018

Conversation

ghost
Copy link

@ghost ghost commented Dec 2, 2018

I have started reviewing the start.c files of all bootstraps for changes, and preparing a unification process to remove the different start.c files. This pull request is the result of this.

WIP because I still need to test this myself.

@ghost ghost changed the title [WIP] Unify services_only bootstrap start.c and sdl2/common bootstrap start.c [WIP] Unify start.c of all bootstraps to one file Dec 2, 2018
@ghost
Copy link
Author

ghost commented Dec 2, 2018

Okay, so I tested the sdl2 bootstrap with python3crystax and a real world app, and it builds & still works! Now the others still need to be tested. Anyone has some actual apps lying around to test with that?

@inclement
Copy link
Member

At a quick glance, looks very solid so far.

I've been meaning for a while to make the start.c code less awful, do you have any intention to try that? It might be an opportunity to avoid some of the ifdef mess we currently have. However, it isn't essential, just merging the files is a great improvement.

@ghost
Copy link
Author

ghost commented Dec 3, 2018

I've been meaning for a while to make the start.c code less awful, do you have any intention to try that?

Eventually, but so far I'm still mostly busy with fixing architectural issues to get my app back to building from p4a master. This involves rescheduling my ctypes.util.find_library() merge request (which changes start.c, hence is best to go in after this one) to finally get this working for Python 3.

I am interested in also cleaning up the code itself some more, I just haven't really gotten around yet. But this pull request should be a splendid foundation for anyone to do that in the near future - and if you want to jump into it first, by all means go for it (but I would suggest it's best done in a separate followup request, because this one has high conflict potential and should go in soon)

@ghost ghost changed the title [WIP] Unify start.c of all bootstraps to one file Unify start.c of all bootstraps to one file Dec 3, 2018
@ghost
Copy link
Author

ghost commented Dec 3, 2018

Right, since the only thing remaining is more testing, I'll remove the WIP now. I tested sdl2, I think @AndreMiras wanted to look into service_only, might be worth looking into webview still working (if it wasn't broken before) before merging

@ghost
Copy link
Author

ghost commented Dec 9, 2018

(latest force push is rebase onto master with gradle issue fixed, no change in this pull request)

Testing progress:

  • python3crystax / sdl2 bootstrap: ✔️ tested build & deploy & launch of actual real-world app with p4a-build-spaces' p4a-py3crystax-api19-ndkbundle environment
  • python3 / sdl2 bootstrap: ✔️ tested build of testapp_keyboard, to reproduce: p4aspaces cmd p4a-py3-api28ndk21 testbuild --p4a https://github.com/JonasT/python-for-android/archive/startcunify.zip
  • python2 / sdl2 bootstrap: ✔️ tested build of testapp_keyboard, to reproduce: p4aspaces cmd p4a-py2-api19-ndkbundle testbuild --p4a https://github.com/JonasT/python-for-android/archive/startcunify.zip
  • python2 / webview bootstrap: ❓(✔️) testapp_flask doesn't build on p4a master in the first place, see testapp_flask doesn't build with webview bootstrap, final bootstrap compiler options appear to have some sort of issue #1509 - but I made sure it gets to the same error with this pull request, to reproduce: p4aspaces cmd p4a-py2-api19-ndkbundle testbuild_webview --p4a https://github.com/JonasT/python-for-android/archive/startcunify.zip
  • python2 / service_only bootstrap: ❓(✔️) testapp_nogui doesn't build on p4a master either, possibly related error, again see testapp_flask doesn't build with webview bootstrap, final bootstrap compiler options appear to have some sort of issue #1509 - but I made again sure it gets to the same error with this pull request, to reproduce: p4aspaces cmd p4a-py2-api19-ndkbundle testbuild_service_only --p4a https://github.com/JonasT/python-for-android/archive/startcunify.zip

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 to me, I don't see any real issues.
Since the webview and service_only bootstraps are broken anyway, no need to worry about them for now.
I did a quick build test with an app I was working on, without any issues. I'll try a bit more than that just to check, but other than that it looks good to go as far as I'm concerned.

@ghost
Copy link
Author

ghost commented Dec 9, 2018

I did another Python 2 sdl 2 bootstrap testbuild with the force-pushed version that fixes the HAS_NO_SDL_BOOTSTRAP/BOOTSTRAP_USES_NO_SDL_HEADERS thing, and it worked fine. Would have been surprised about anything else but wanted to be sure

@AndreMiras
Copy link
Member

Also looks good to me, I'll give it a run-time try with service_only and crystax later today.
Thanks guys!

Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

Tested on host python3, target python3crystax with both sdl2 and service_only bootstraps.
Everything was OK.
The git stats looks great 😄 well done:

git diff --shortstat upstream/master
 9 files changed, 92 insertions(+), 856 deletions(-)

@AndreMiras AndreMiras merged commit 1d1dfa2 into kivy:master Dec 10, 2018
@ghost ghost deleted the startcunify branch December 10, 2018 23:43
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