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

Pin pyjnius version #1416

Closed
wants to merge 1 commit into from
Closed

Pin pyjnius version #1416

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 20, 2018

This pull request adds a version pin for pyjnius as requested here: #1415

Please note I am proposing this as a permanent measure because this is such a core component - not this specific version of course, but that it is always pinned.

Even if you just randomly bump the version up in any random commit without checking, this is a huge improvement: it will prevent p4a master builds from randomly failing out of the blue when not even changing the commit, and people will be able to go back to an earlier p4a master commit to avoid sudden pyjnius breakages instead of patching around in the recipes folder (which depending on the build pipeline might be quite a time waster to do, especially compared to just going back to a known working p4a master commit).

Summed up, please pin this, carelessly bump it whenever, and have less unhappy users. 😄

(And I'm not proposing pinning everything, I know you have way too many recipes and people would forget to bump it, I understand - but at least the core components like pyjnius, would that possibly sound feasible?)

@AndreMiras
Copy link
Member

Thanks for the pull request!
FYI I've restarted some of the failing builds because some downloads are flaky and make the build fail.
Things I generally like in pull requests:

  • the 50/72 rule
  • write descriptive title rather than lecturing e.g. Pins pyjnius version, fixes #1415
  • short commit hashes are cute e.g. 6233ffe

It's more a matter of personal taste than a pro tip.

@rnixx
Copy link
Member

rnixx commented Oct 20, 2018

If pinning gets done, which is actually a good idea, please make at least a tag of pyjnius.

@KeyWeeUsr
Copy link
Contributor

@rnixx PyJNIus is already prepared for simple deployment via Travis, you can test it with pip install ... from kivy/pyjnius#356, so ++ for doing a release ASAP. If no one is against, I can do it.

Also the issue is the same one that broke the patches (because me + style checkers and making it more readable) 😄 Fixed in #1417.

@AndreMiras
Copy link
Member

AndreMiras commented Oct 20, 2018

This is now "conflicting" with #1417
In #1417 @KeyWeeUsr is updating the patch to match master. It means your pinned commit hash would now fail since the new patch is now working past this hash.
In the meantime we do agree we should pin when possible. So we could wait for pyjnius to be tagged before we can eventually pin it.

@ghost
Copy link
Author

ghost commented Oct 21, 2018

Shouldn't #1417 just update the pin with exactly that merge? That is the point of having a pin, updating it along with all changes that make things incompatible, such that no arbitrarily incompatible versions of patch & pyjnius or whatever are combined. So in my humble opinion the pin should be set as early as possible, and then anything that changes things that don't work with this pin should simply update the pin in that same changeset

Edit: ah, already got merged. well, at least that would be a good process for the future

@ghost
Copy link
Author

ghost commented Oct 21, 2018

@rnixx I have been thinking about your comment, and I'm wondering, while pinning a tag seems helpful for major releases, wouldn't it be better to stick with pinning to commits for p4a's master?

Otherwise it'll either be a huge amount of tags, or p4a's master will be quite inflexible / it'll be effort for to use unusual experimental changes in pyjnius, and that seems like it wouldn't be a good idea... but that's just my first thought on this

@inclement
Copy link
Member

Summary of my opinions from discord:

  • This seems like an excellent idea for releases, and for the stable branch. Perhaps it can be a part of release procedure to pin certain recipes.
  • I feel like having p4a master track pyjnius master has advantages, in fact it's like this in the first place because it's usually been better than the alternative in the past as pyjnius is only updated quite rarely and usually for important reasons. It also guarantees that pyjnius gets meaningful testing without much effort, otherwise basically nobody would use pyjnius master.
    That said, pinning specific commits and updating regularly probably wouldn't be a major issue for point 2, I guess I'm really saying that it might not be worth the trouble since updating the pin could easily get forgotten and lead to surprising bugs at a later date.

AndreMiras added a commit to AndreMiras/python-for-android that referenced this pull request Oct 22, 2018
Fixes patch failing with:
```
patching file setup.py
Hunk #1 FAILED at 53.
1 out of 1 hunk FAILED -- saving rejects to file setup.py.rej
```
refs kivy#1414, kivy#1416, kivy#1417
@AndreMiras AndreMiras mentioned this pull request Oct 22, 2018
@ghost
Copy link
Author

ghost commented Oct 22, 2018

@inclement I'll sure be jumping ship from using master even if such pyjnius breakage only happens rarely. The problem is not the breakage itself, but that it's not pinned / reproducible for one commit, that's just pure acid for any actual deployment use even for the more adventurous. So if you want people to test it, I think leaving this unpinned won't necessarily do you a favor.

Also, for Python 3, the p4a stable is pretty useless right now given how much doesn't work with it. But of course that could change with upcoming releases.

Edit: or maybe stable has already received updates, I admit I haven't been closely tracking it. But at least whatever is on pip wasn't in a good shape (for python 3 use, that is) last time I tried it, if that's actually the stable version

AndreMiras added a commit to AndreMiras/python-for-android that referenced this pull request Oct 22, 2018
Fixes patch failing with:
```
patching file setup.py
Hunk #1 FAILED at 53.
1 out of 1 hunk FAILED -- saving rejects to file setup.py.rej
```
refs kivy#1414, kivy#1416, kivy#1417
AndreMiras added a commit to AndreMiras/python-for-android that referenced this pull request Oct 22, 2018
Fixes patch failing with:
```
patching file setup.py
Hunk #1 FAILED at 53.
1 out of 1 hunk FAILED -- saving rejects to file setup.py.rej
```
refs kivy#1414, kivy#1416, kivy#1417
@AndreMiras
Copy link
Member

Pinned in p4a stable to begin with #1426

@AndreMiras
Copy link
Member

I've just created #1427 to pin on the last freshly created tag.
I would prefer to give you the credits if you want to amend your change with tag and new commit message

@ghost
Copy link
Author

ghost commented Oct 23, 2018

I really couldn't care less about credits especially for such a minor change, so if it's less work, just merge it in 👍

AndreMiras added a commit that referenced this pull request Oct 23, 2018
@ghost ghost deleted the pyjnius-version-pin branch November 23, 2018 12:35
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.

4 participants