Skip to content
This repository has been archived by the owner on Apr 30, 2021. It is now read-only.

Check for Update on Application Start (lightweight) #34

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

estberg
Copy link
Contributor

@estberg estberg commented Jan 17, 2020

A Continuation of 28 aiming to resolve 13.

Essentially uses @NizamLZ's approach to:

  1. In a background thread, it will hit the github releases api to get the latest release
  2. If new version is greater, based on the OS (MacOS or windows), correct asset url is picked and displayed in a dialog box as a link.
  3. Also along with the link, github release description will be converted from markdown to html and displayed in the update dialog box.

The only change is the packages that are used to do this are much more lightweight.
The mac binary is 37.3 MB and the windows binary is 13.1 MB.
(Special thanks to @PieterBenjamin for helping with building the windows binary)

@lognaturel
Copy link
Member

Thanks for taking this on! You have verified that both Windows and macOS builds work as expected, right? In particular, you're not getting certificate issues?

@estberg
Copy link
Contributor Author

estberg commented Jan 17, 2020

Sorry, but I am not sure where so I want to ask, where can I check if there were certificate issues with the builds? We are able to make the binaries and then open them.

@lognaturel
Copy link
Member

@estberg can you verify that the Windows binary actually makes a successful request to Github? If it makes the request and gets the response as expected, then you don't have those issues. To be 100% sure, you can observe network traffic.

@PieterBenjamin
Copy link
Contributor

PieterBenjamin commented Jan 24, 2020

@lognaturel

@estberg and I followed @yanokwa s comment in the following way:

  1. We set the spec file to print to console
  2. We added the following printlns in the update checker file after line 46:
print(latest_version[1:])
print(self._current_version[1:])

With those changes, we saw the following output:

C:\xlsform-offline>"dist\win\ODK XLSForm Offline.exe"
1.11.1
1.11.1

We're pretty sure this indicates that we are checking for the update correctly, but haven't explicitly monitored traffic. Should we go that step further or do you think this is sufficient evidence?

@lognaturel
Copy link
Member

That looks sufficient to me, thanks!

requirements.txt Outdated
pyinstaller==3.5.0
wxpython==4.0.6
pyxform==0.15.1

Copy link
Member

@yanokwa yanokwa Jan 28, 2020

Choose a reason for hiding this comment

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

Is there a reason you are using old versions of these packages instead of the most recent ones? If so, a comment here would be great. If not, we should use recent versions.

Relatedly, in the case of pyinstaller 3.4, it has a severe vulnerability and so we have to use 3.6. See GHSA-7fcj-pq9j-wh2r for more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I will try in my environment with the most recent versions.

@yanokwa
Copy link
Member

yanokwa commented Jan 28, 2020

I tried this on a fresh macOS 10.15.2 (Catalina) install and did not have success. This is a long comment, so bear with me...

I used Python 3
I did not use pyenv or virtualenv because I have not had luck with those in the past. Instead, I used the natively installed Python 3.7.3. Note that Python 2.7.16 is available on macOS Catalina at python (no pip though), but Python 2.x is no longer supported by the Python community, so I did not try it.

The steps I took to run from source are:

  1. xcode-select --install to install the required build tools
  2. sudo pip3 install pyinstaller wxpython pyxform packaging to install the requirements
  3. ./make-mac.sh to start the build.

The build could not be packaged, probably because of Python 3
The build did not complete. I got this error...

3786 ERROR: Can not find path @executable_path/../Python3 (needed by /Library/Developer/CommandLineTools/usr/bin/python3)
Traceback (most recent call last):
  File "/usr/local/bin/pyinstaller", line 10, in <module>
    sys.exit(run())
  File "/Library/Python/3.7/site-packages/PyInstaller/__main__.py", line 114, in run
    run_build(pyi_config, spec_file, **vars(args))
  File "/Library/Python/3.7/site-packages/PyInstaller/__main__.py", line 65, in run_build
    PyInstaller.building.build_main.main(pyi_config, spec_file, **kwargs)
  File "/Library/Python/3.7/site-packages/PyInstaller/building/build_main.py", line 734, in main
    build(specfile, kw.get('distpath'), kw.get('workpath'), kw.get('clean_build'))
  File "/Library/Python/3.7/site-packages/PyInstaller/building/build_main.py", line 681, in build
    exec(code, spec_namespace)
  File "pkg/xlsform-offline-mac.spec", line 12, in <module>
    a = Analysis(['../src/main.py'])
  File "/Library/Python/3.7/site-packages/PyInstaller/building/build_main.py", line 244, in __init__
    self.__postinit__()
  File "/Library/Python/3.7/site-packages/PyInstaller/building/datastruct.py", line 160, in __postinit__
    self.assemble()
  File "/Library/Python/3.7/site-packages/PyInstaller/building/build_main.py", line 478, in assemble
    self._check_python_library(self.binaries)
  File "/Library/Python/3.7/site-packages/PyInstaller/building/build_main.py", line 568, in _check_python_library
    python_lib = bindepend.get_python_library_path()
  File "/Library/Python/3.7/site-packages/PyInstaller/depend/bindepend.py", line 945, in get_python_library_path
    raise IOError(msg)
OSError: Python library not found: libpython3.7.dylib, Python, .Python, libpython3.7m.dylib
    This would mean your Python installation doesn't come with proper library files.
    This usually happens by missing development package, or unsuitable build parameters of Python installation.

    * On Debian/Ubuntu, you would need to install Python development packages
      * apt-get install python3-dev
      * apt-get install python-dev
    * If you're building Python by yourself, please rebuild your Python with `--enable-shared` (or, `--enable-framework` on Darwin)

Based on https://github.com/pyinstaller/pyinstaller/wiki/FAQ#mac-os-x, my guess is that we'll have to use pyenv and build with PYTHON_CONFIGURE_OPTS="--enable-shared". I did not proceed further, because I've had issues getting pyenv-based pyinstaller apps to work.

The app did run, but had some Python 3 issues
Next, I tried to run the app without bundling using python3 src/main.py. Here too, I ran into problems. Both of these are Python 3 issues.

  1. Import of urllib2 failed at update_checker.py:7 Below is the fix that worked for me.
try:
    import urllib.request as urllib2
except ImportError:
    import urllib2
  1. Java version check failed at main.py:378. Below is the fix that worked, but we should check with Python 2 to make sure that works.
java_regex = re.compile(b'(java|openjdk) version')

Once I did the above, the app launched, but I had two more problems. These should probably be fixed with follow on PRs, but I don't feel strongly about where those commits go.

  1. I got a macOS error for Java not being found. Ideally, we'd find some way to check that Java exists without throwing up this error message.

Screen Shot 2020-01-28 at 1 13 44 PM

  1. CERTIFICATE_VERIFY_FAILED error on the command line
<urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1056)>

My guess is that somehow Python doesn't know where the certs are? I tried sudo pip3 install --upgrade certifi to upgrade certs, and that didn't work. Without these certs, I bet I can't verify the Github connection works.

@estberg, I know this is a lot to digest, so maybe we can start with few questions so I can reproduce your successful build.

  1. What operating system and Python version did you use? And how did you install Python (e.g., brew, ports, chocolatey)?
  2. Is the choice of older package versions in your PR on purpose?
  3. Would it be possible for you to try this same PR with a Python 3 setup?

@estberg
Copy link
Contributor Author

estberg commented Jan 28, 2020

Thanks @yanokwa for taking all the time to try this out and leave this feedback for us.

  1. I developed this in a conda environment using Python Version 2.7.17, but I'm more than happy to try to find fixes to get it working for Python 3, now that we are in 2020. Thanks for noting that!

  2. The outdated package use was an accident, I will try to update.

  3. I'll test this on both python 2 and 3. Do you think that trying with pip and pyenv will be the best way to ensure consistency?

@yanokwa
Copy link
Member

yanokwa commented Jan 28, 2020

I'm not super familiar with conda, but it seems for this use-case, it's playing the role of pip and pyenv. So yes, I would use pip and pyenv and see how that goes.

@PieterBenjamin
Copy link
Contributor

@yanokwa @estberg and I have been working on windows computers with Python 3.7.4 installed. The way we got our end working was to change the make-win.bat file to specify which version of python to install (as well as ensure the correct packages have been installed).

@C:\Python27\python.exe -m pip install PyInstaller wxpython pyxform packaging
rd /s /q build\xlsform-offline-win 
rd /s /q dist\win
del /s /q *.pyc
@C:\Python27\python.exe -m PyInstaller pkg\xlsform-offline-win.spec --distpath dist\win --onefile --windowed --noconfirm --clean

I'm the one who made the decision to do it this way because my understanding was that Python2 and Python3 are separate enough that having Python3 installed does not satisfy the 2.7 requirement specified by this repos readme. Should we change the .sh and .bat files to always work regardless of Python version? If so, should the readme on the repo also be changed?

I'll also note that the urllib issue you mentioned sounds familiar from our experience trying to use Python3 without the correct packages installed in 2.7.

@yanokwa
Copy link
Member

yanokwa commented Jan 30, 2020

@PieterBenjamin The README is out of date so yes, please update the README to read "Python 3". I don't think we need to specify the point version just yet.

It is true that Python 2 and 3 are very different. I think we should drop Python 2 support and focus exclusively on Python 3 for this release.

As far as sequencing on the Window side, I'd try to get it running on a machine with a single Python 3 (kind of like you did above) first. Once that works, see if you can get it running with a pyenv setup.

I bet pyenv will not work, but we should try because that's what most developers expect. It's not super important to solve though because we're going to be using a build machine to make these binaries.

@PieterBenjamin
Copy link
Contributor

@yanokwa Should the installation files specify python 3 or just use whatever the client has installed?

@yanokwa
Copy link
Member

yanokwa commented Jan 31, 2020

To be explicit here...you don't need any Python to run the app. PyInstaller bundles whatever Python the developer has installed when packaging. In this case, we are requiring that devs use Python 3, so that's what will be bundled.

@estberg estberg force-pushed the feature/update-checker branch from a6002b2 to e33050d Compare April 24, 2020 21:58
@estberg
Copy link
Contributor Author

estberg commented May 1, 2020

We are still working on testing this on Windows, but it works on Mac now!

Co-authored-by: Pieter Benjamin <[email protected]>
@PieterBenjamin
Copy link
Contributor

We just pushed a commit - we weren't copying the update.html file over to the temp directory on Windows so even though an update was detected, the user wasn't notified!

Let us know if this looks good and we can squash the commit.

@lognaturel lognaturel requested a review from yanokwa May 8, 2020 23:36
@lognaturel
Copy link
Member

lognaturel commented Jun 11, 2020

Thanks so much for all the hard work that's gone into this @PieterBenjamin, @estberg and so sorry about the long radio silence. The code looks good to me and I've double checked that the binary sizes are only slightly increased. I wanted to do a sanity check on the behavior and am having some problems. It looks like only branches on the main repo, not forks, get their builds uploaded, is that right?

I pushed to https://github.com/getodk/xlsform-offline/compare/feature/update-checker where I added a commit setting the version to '1.0.0'. I can correctly see v1.0.0 in the footer of the about screen but I'm not getting a version update notice. Is this a reasonable way to try it out? http://travis.getodk.org/xlsform-offline/ODK-XLSForm-Offline-macOS-v2.0.0-21-g62b5a35.zip is the macOS build I tried.

@yanokwa
Copy link
Member

yanokwa commented Jun 11, 2020

I can confirm it works on Windows, but doesn't work on the Mac.

Screen Shot 2020-06-11 at 1 45 33 PM

@PieterBenjamin
Copy link
Contributor

Hi folks, thanks for the feedback. I think that yeah only branches get built. As to the v1.0.0, the thing which jumps to mind for me is how we check new versions. If v1.0.0 was less than the current version, I can imagine it not working. But it sounds like @yanokwa was able to get the notification on windows? Although from his screenshot, it sounds like he used v2.0.0.

@yanokwa
Copy link
Member

yanokwa commented Jun 11, 2020

I downloaded @lognaturel's build which should have set the version to v1.0.0. On Windows, it correctly showed that v2.0.0 is the newest version. On macOS, it did nothing.

@PieterBenjamin
Copy link
Contributor

PieterBenjamin commented Jun 11, 2020

I am not sure what's happening here. I added some file writes (as an alternative to println). We definitely detect that an update is available, and the requests get all the correct information. Below I've added a screenshot of the html_body field that we get. I also added some fwrites to ensure there are no exceptions. The only thing that doesn't seem to be working is wx.PostEvent.
Screen Shot 2020-06-11 at 2 52 54 PM
That being said, the post event doesn't seem to crash, it just doesn't do anything.

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

Successfully merging this pull request may close these issues.

Add update check
4 participants