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

Fix Windows installer to correctly detect running Kolibri #8083

Merged
merged 4 commits into from Jul 9, 2021

Conversation

micahscopes
Copy link
Contributor

Summary

This integrates a fix for the Windows installer, so that the installer gets the correct port information from server.pid when putting together a running Kolibri instance's base URL.

References

Reviewer guidance

Testing checklist

  • Contributor has fully tested the PR manually

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@micahscopes micahscopes added the TODO: needs review Waiting for review label May 13, 2021
@radinamatic
Copy link
Member

We don't seem to be there yet 😕

On Windows 7, Kolibri.exe launcher still picks up the 4xxxx port by default, displays it in the system notification, and opens the browser with it.

win7-kolibri exe-start

I will re-test on Windows 10 to confirm, but doubt that anything will change.

@radinamatic
Copy link
Member

Confirmed as still extant on Windows 10 too:

win10-kolibri exe-start

@jonboiser
Copy link
Contributor

jonboiser commented May 26, 2021

@radinamatic @micahscopes Can you try to find the actual server.pid file on the VM? Looking at the code here, does the fact that it is looking for a specific port mean that

httpLink = joinChr("http://127.0.0.1:", line.c_str());

Evaluates to something? Otherwise wouldn't it just be httpLink = http://127.01.0.1<some inappropriate data>?

https://github.com/learningequality/kolibri-installer-windows/blob/develop/src/gui-source/Kolibri/Kolibri.cpp#L81

@radinamatic
Copy link
Member

@jonboiser That's the first thing both @rtibbles and @micahscopes asked for, but PID files were apparently OK 😕

pids.zip

docker/env.list Outdated Show resolved Hide resolved
@rtibbles
Copy link
Member

rtibbles commented Jul 9, 2021

The most confusing thing here is that it is picking up a discrete line in the PID file, it's just picking up the third line instead of the second.

docker/env.list Outdated Show resolved Hide resolved
@rtibbles
Copy link
Member

rtibbles commented Jul 9, 2021

Additional testing of a stripped down version of the PID reading code running against a Kolibri server shows very consistent output - always picking up the correct port, and returning nothing when the port is not available to be read.

Really not sure what is causing this to go awry in production.

docker/env.list Outdated Show resolved Hide resolved
@radinamatic
Copy link
Member

Shipit! 💯 🎉

Win 7 Win 10
210709-win7-start 210709-win10-start

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

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

Fixed, thank you @rtibbles! 👍🏽

@rtibbles
Copy link
Member

rtibbles commented Jul 9, 2021

For posterity - @micahscopes had made the correct fix, but we needed to rebuild the exe and commit it to the repo for the fix to take effect.

@rtibbles rtibbles merged commit bf07dbd into develop Jul 9, 2021
@rtibbles rtibbles deleted the windows-installer-detects-running-kolibri branch July 9, 2021 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants