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 logging issue on windows #5364

Conversation

mrpau-richard
Copy link
Contributor

Summary

Hi @benjaoming & @radinamatic This fixes #5057

@radinamatic Here's the windows installer for testing the KA Lite windows logging.

@radinamatic
Copy link
Member

@mrpau-richard Fix confirmed! 👍

What a beautiful sight: logging on Windows again! 😄

virtualbox_windows 7_16_12_2016_14_37_14

@benjaoming
Copy link
Contributor

@radinamatic do you have a server.log with these contents, too? Does --foreground option still work, too?

@benjaoming
Copy link
Contributor

This change seems to suppress final command line output.

Before:

➜  ka-lite git:(develop) bin/kalite start
Running 'kalite start' as daemon (system service)

Stand by while the server loads its data...

To access KA Lite from another connected computer, try the following address(es):
	http://192.168.1.100:8008/
	http://10.20.20.1:8008/
To access KA Lite from this machine, try the following address:
	http://127.0.0.1:8008/

Going to daemon mode, logging to /home/ben/.kalite/server.log

Now:

➜  ka-lite git:(mrpau-logging) bin/kalite start 
Running 'kalite start' as daemon (system service)

Stand by while the server loads its data...

Going to daemon mode, logging to /home/ben/.kalite/server.log

@codecov-io
Copy link

codecov-io commented Dec 19, 2016

Current coverage is 51.78% (diff: 0.00%)

Merging #5364 into develop will increase coverage by 0.23%

@@            develop      #5364   diff @@
==========================================
  Files           142        142          
  Lines          7608       7606     -2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3922       3939    +17   
+ Misses         3686       3667    -19   
  Partials          0          0          

Powered by Codecov. Last update 4c7a2b2...adb0a6b

@mrpau-richard
Copy link
Contributor Author

Hi @benjaoming done fixing the final command line output.

screen shot 2016-12-19 at 7 11 28 pm

Hi @radinamatic Here's the windows installer for your testing.

@radinamatic
Copy link
Member

@benjaoming Yes, server.log file is present, and --foreground option works!

server.log does not contain all the stuff that appears in the command prompt window, though...

virtualbox_windows 7_19_12_2016_15_45_15

@radinamatic
Copy link
Member

radinamatic commented Dec 19, 2016

On Windows this is the command prompt output:

C:\Users\IEUser>kalite start --foreground
Running 'kalite start' in foreground...

Stand by while the server loads its data...

To access KA Lite from another connected computer, try the following address(es)
:
        http://10.0.2.15:8008/
To access KA Lite from this machine, try the following address:
        http://127.0.0.1:8008/

[INFO] [2016-12-19 07:03:13,808] cherrypy.error: [19/Dec/2016:07:03:13] ENGINE L
istening for SIGTERM.
[INFO] [2016-12-19 07:03:13,812] cherrypy.error: [19/Dec/2016:07:03:13] ENGINE B
us STARTING
[INFO] [2016-12-19 07:03:13,815] cherrypy.error: [19/Dec/2016:07:03:13]  Loading
 and serving the Django application
[INFO] [2016-12-19 07:03:13,825] cherrypy.error: [19/Dec/2016:07:03:13] ENGINE S
tarted monitor thread '_TimeoutMonitor'.
[INFO] [2016-12-19 07:03:14,059] cherrypy.error: [19/Dec/2016:07:03:14] ENGINE S
erving on http://0.0.0.0:8008
[INFO] [2016-12-19 07:03:14,062] cherrypy.error: [19/Dec/2016:07:03:14] ENGINE B
us STARTED

@benjaoming
Copy link
Contributor

@radinamatic
Command-line output in your comparison starts at 06:35, while the server.log starts 06:33, so it's probably log data from previous invocation.

@mrpau-richard I'll test this later tonight, will merge when it's confirmed working :) Great fix!!

@@ -511,6 +509,9 @@ def start(debug=False, daemonize=True, args=[], skip_job_scheduler=False, port=N

manage('initialize_kalite')

for addr in get_urls_proxy(output_pipe=sys.stdout):
sys.stdout.write("\t{}\n".format(addr))

This comment was marked as spam.

@benjaoming
Copy link
Contributor

@mrpau-richard - this is actually part of the PR template since merging another PR (which isn't part of yours, that's why tests are failing) - you should always add something in the release notes about your fixes or features. I'll add it after, so don't worry. Just letting you know :) ..it's to keep the develop branch "always releasable".

@benjaoming
Copy link
Contributor

Confirming that stdout is working now again on Ubuntu, merging and making a bit of cleanup after.

@benjaoming benjaoming merged commit 321b9c6 into learningequality:develop Dec 19, 2016
@benjaoming benjaoming removed the has PR label Dec 19, 2016
benjaoming added a commit that referenced this pull request Dec 20, 2016
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.

5 participants