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

Make bootstrap dynamic for travis conditional builds #1594

Closed
wants to merge 3 commits into from

Conversation

opacam
Copy link
Member

@opacam opacam commented Jan 15, 2019

This should solve: #1588

@AndreMiras
Copy link
Member

Thanks for it @opacam I like the approach and we can probably merge after some testing.
The approach I was thinking of was to play with git diff --name-only and compile on bootstrap changes. So for instance if a PR touches service_only we recompile using this bootstrap.
With your approach it feels more like if the recipe is compatible with webview bootstrap only then that's the one that will be picked up. Am I right?

@opacam
Copy link
Member Author

opacam commented Jan 15, 2019

Oh yes, uses the system that we use to resolve the dependencies and pick up one bootstrap (just the same as when building without specifying a bootstrap)

opacam added a commit to opacam/python-for-android that referenced this pull request Jan 17, 2019
Caused because we try to compile the package with hostpython and we need to use target python or we will not have the right python headers

Resolves: kivy#1575
@opacam opacam changed the title Make bootstrap dynamic for travis conditional builds [WIP] Make bootstrap dynamic for travis conditional builds Feb 10, 2019
ci/constants.py Outdated
BROKEN_RECIPES_PYTHON2 = set([
])
# to be created via https://github.com/kivy/python-for-android/issues/1514
BROKEN_RECIPES_PYTHON3 = set([
Copy link
Member

Choose a reason for hiding this comment

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

This list is already defined here

BROKEN_RECIPES_PYTHON3 = set([
why would we override?
I would keep that comment about #1514 however, good idea

bootstrap = get_bootstrap(recipes | {target_python.name})
if bootstrap:
break
else:
Copy link
Member

Choose a reason for hiding this comment

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

I understand you wanted to keep some meaningful logs to see the fallback in action, but we could greatly simplify the code if we drop that feature actually. Having the trying to get a bootstrap forcing target python above would be more than enough to guess when falling back. If we do so you can drop all the else block and the enumerate requirement

target_python_priorities[n + 1].name))
continue
if not bootstrap:
logger.warning('we didn\'t find any valid combination of bootstrap and'
Copy link
Member

Choose a reason for hiding this comment

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

Minor: this is a good example where I would use double quotes in my string, just to avoid escaping inner single quotes

logger.warning('we didn\'t find any valid combination of bootstrap and'
' target python...rebuild updated recipes cancelled.'
'The recipes we couldn\'t rebuild are:'
'\n\t-{}'.format('\n\t-'.join(list(recipes))))
Copy link
Member

Choose a reason for hiding this comment

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

Minor: why would you need the list() here, can't you join directly?

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.

Thanks for pushing this further, I left some early comments.
Also I'm puzzled if we want to support both python2 and python2legacy in that conditional build thing.
I would be tempted to support if it doesn't cause too much trouble, but drop at least one if it starts annoying us

@opacam
Copy link
Member Author

opacam commented Feb 10, 2019

Thanks for the comments, tomorrow I will look into it..:sleeping:
🌑

ci/constants.py Outdated
@@ -96,9 +98,13 @@ class TargetPython(Enum):
'sympy',
'vlc',
])
# to be created via https://github.com/kivy/python-for-android/issues/XXXX
Copy link
Member Author

Choose a reason for hiding this comment

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

I think that we should create an issue page for python2.
What do you think @AndreMiras ?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it's worth the effort, but feel free if want

ci/constants.py Outdated
@@ -5,12 +5,13 @@ class TargetPython(Enum):
python2 = 0
python3crystax = 1
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should remove python3crystax?

Copy link
Member

Choose a reason for hiding this comment

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

Yes feel free 👍

target_python_priorities = [
TargetPython.python3,
TargetPython.python2,
TargetPython.python2legacy
Copy link
Member Author

Choose a reason for hiding this comment

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

python2legacy is unlikely to be used unless we modify some pygame dependant recipe, which will be removed soon, so...I'm not sure if we have to introduce this 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Yes by default I would be lazy and drop

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but we already have the work done in here, so maybe we should maintain this, at least until @inclement can finish his pygame removal work, then maybe we could start to remove all the python2legacy stuff. It's easy to remove this, I'm more concerned with other python2legacy code...but we will add some more code to remove...mmm...@inclement, what is your opinion on this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, after thinking about it, I took the lazy road 😁, removed python2legacy

@opacam
Copy link
Member Author

opacam commented Feb 10, 2019

Again @AndreMiras, so many thanks for your comments of last night, you were right with all of that, and I must say, a lot better 😄. I don't plan touch this pull request unless you request me, so feel free to modify wathever you consider necessary, no problem 😉 (ahhh...and I don't have any more un-pushed work related with this pr...so we shouln't have problems If you want to add some commit, I could pull your changes...so feel free to modify whatever you want)

@opacam opacam changed the title [WIP] Make bootstrap dynamic for travis conditional builds Make bootstrap dynamic for travis conditional builds Feb 12, 2019
@inclement inclement changed the base branch from master to develop June 6, 2019 20:41
@inclement inclement closed this in 185936e Jul 14, 2019
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