search

ywangd / stash

StaSh - Shell for Pythonista
MIT License
1.92k stars 227 forks source link

Patch stash to run with Pythonista 3.4 beta #463

Closed mkb79 closed 1 year ago

mkb79 commented 1 year ago

launch_stash.py will work. But an ignored exception No method found for selector "setAutomaticallyAdjustsContentInsetForKeyboard:" is raised.

pip seams to fail to download files.

Edit: The issue with pip is an exception raising in wget. <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:997)>

bennr01 commented 1 year ago

Thank you for your bug report and the patch. I hadn't yet the chance to try your patch out myself, but looking at the changes I think these changes may still need a bit of improvement:

  1. You use certifi to patch wget. certifi is not part of the default python library, but included in pythonista by default. That's ok, but you'll need to add it as a dependency to setup.py so it will automatically be installed in the test environment too.
  2. In your changes to shcommon.py, you've removed PYTHONISTA_VERSION = ... and PYTHONISTA_VERSION_LONG = .... I assume you wanted to move them behind the definition of the _properties() function and forgot to re-add these defintions. Either way, unless I am missing something here, this may break some stuff. Please re-add those definitions.
mkb79 commented 1 year ago

@bennr01

In your changes to shcommon.py, you've removed PYTHONISTA_VERSION = ... and PYTHONISTA_VERSION_LONG = ....

Sorry, my mistake. I've re-added them now.

You use certifi to patch wget. I can add them to the setup.py or I can add a try/except import to add the cafile only if certifi is installed. What’s the best option your you?

jsbain commented 1 year ago

Looks like we need to change isAlive to is_alive in shthreads.py

also, is it just me, or do you have to call _stash.launch() from a background thread? I’m getting a crash on iPadOS 15 otherwise.

mkb79 commented 1 year ago

Looks like we need to change isAlive to is_alive in shthreads.py

Thank you for your information. I've added these to the PR. And thanks for your hint with the SUITextView_PY3.

also, is it just me, or do you have to call _stash.launch() from a background thread? I’m getting a crash on iPadOS 15 otherwise.

Which line of code you mean exactly?

yaroslavyaroslav commented 1 year ago

Come on guys, millions of eyes are watching you right now like never before. If OMZ could do it, so can you.

bennr01 commented 1 year ago

I can add them to the setup.py or I can add a try/except import to add the cafile only if certifi is installed. What’s the best option your you?

I think adding them to setup.py would be the best, as it would mean less difference between the test environment and user enviroments. Either way, the changes seem to be good now, I'll merge this PR as soon as certifi is added to setup.py.

Come on guys, millions of eyes are watching you right now like never before. If OMZ could do it, so can you.

I think millions of eyes may be a bit optimistic... unless you have like a lot of them in a jar somewhere...

mkb79 commented 1 year ago

@bennr01

I think adding them to setup.py would be the best, as it would mean less difference between the test environment and user enviroments. Either way, the changes seem to be good now, I'll merge this PR as soon as certifi is added to setup.py.

certifi is Python 3+ only. So this may break something?!

fschaeck commented 1 year ago

certifi is Python 3+ only. So this may break something?!

Since Pythonista 3.4 won‘t support Python 2 anymore and comes with Python 3.10 only, I think it is high time for stash to say Good-Bye to Python 2 as well and make the next release Python 3.10 only. Everybody having scripts that need earlier Python versions will have to stay on Pythonista 3.3 anyway or migrate them to 3.10.

… Coming to think of it, stash might be used in other places than Pythonista as well. Thus it might not be that simple to drop at least Python 2 support.

jsbain commented 1 year ago

We could just fork and rename to stash3.

On my iPad 6th gen running ipados15.1 I was getting a crash when running launch_stash. I chenged

_stash.launch(ns.command)

To something wrapping in_background


import ui
@ui.in_background()
def launch(*args):
   _stash.launch(*args)

launch(ns.command)
bennr01 commented 1 year ago

Sorry for the delay.

certifi is Python 3+ only. So this may break something?!

That's a good point, I hadn't checked.

I agree with the others that it may finally be time to drop py2, as much as it hurts (py27 FTW!). I propose the following course of action:

Anyone here opposed to this proposal?

As for the PR, I'll merge it and #465 once we've decided on the future of py2 stash. It all looks ok, I don't think any more changes are necessary. Once again, thank you for your contribution.

ikiziki commented 1 year ago

I see it has been a month since there was any further activity on this PR, is there an ETA on when it can be merged in to work with Pythonista 3.4 Beta?

cclauss commented 1 year ago

Please rebase and resolve git conflicts.

bennr01 commented 1 year ago

Thank you for your conttribution and you patience. I'll merge it now (Edit: once the tests finish.).

Regarding merge conflicts: No need to rebase and resolve this, this is trivial enough that I can do this quickly myself. (Edit 2 oops, I've messed it up, give me a minute...)

cclauss commented 1 year ago

Let's add the test run on Python 3.10 because that is what the beta runs.

cclauss commented 1 year ago

On legacy Python:

E   Querying PyPI ... 
E   Using rsa==3.2.2...
E   A binary distribution is available and will be used.

Can we pip install --no-binary? Or PIP_NO_BINARY=rsa pipenv install ...?

bennr01 commented 1 year ago

On legacy Python:

E   Querying PyPI ... 
E   Using rsa==3.2.2...
E   A binary distribution is available and will be used.

Can we pip install --no-binary?

Yes, we could, but shouldn't we rather figure out why the binary install is failing? Basic wheel support is included and worked rather well so far (which is why our pip implementation prefers it over our setuptools mock). Either way, this seems like its not caused by the PR itself but rather due to the merge with the dev-branch, so I'd merge it even with that specific test failure. I am more worried that the py39 tests took 16 minutes...

cclauss commented 1 year ago

I don't think that Apple's security guidelines for iOS are going to allow a binary install.

Please add py310 (see above) and run the tests again. That long run might have been a fluke.

bennr01 commented 1 year ago

I don't think that Apple's security guidelines for iOS are going to allow a binary install.

In this case "Binary" does not refer to a compiled file, but rather a distribution that does not require the execution of an installer. It's been a while since I worked on pip, but IIRC nearly all source distribution require arbitrary python code as part of the setup.py file (which may invoke a C-compilation), while binary distributions (like wheels) may include compiled extensions (but we don't support this) or may not include compiled extensions (as indicated by the -none-any part of the wheel name), but more importantly change the setuptools.setup() tool into a couple of static configuration/metadata files.

Anyway, you were right that the py36 failure was a fluke, I'll go ahead and finally merge this PR.