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

Add setting to run Lantern on system startup #175

Merged
merged 34 commits into from
Apr 20, 2015
Merged

Add setting to run Lantern on system startup #175

merged 34 commits into from
Apr 20, 2015

Conversation

atavism
Copy link

@atavism atavism commented Apr 13, 2015

@myleshorton
Copy link
Contributor

It looks like this still has all the windows service code from code.google.com/p/winsvc in there even though it's not used @atavism .

@atavism
Copy link
Author

atavism commented Apr 13, 2015

@myleshorton Ah, thanks. I just removed it.

log.Errorf("Could not get Lantern directory path: %q", err)
return
}
err = gowin.WriteStringReg("HKCU", RunDir, "value", lanternPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

These registry calls still succeed if not running as admin, is that right? Might be worth testing with a full installer.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I verified with regedit. Good point though we should test these changes with the full installer though

@myleshorton
Copy link
Contributor

I get this build error on windows:

src/github.com/getlantern/launcher/launcher_windows.go:6:2: no buildable Go source files in /flashlight-build/src/github.com/luisiturrios/gowin

Indeed that package is empty!

@myleshorton
Copy link
Contributor

Shoot now I get:

src/github.com/luisiturrios/gowin/registry.go:8:2: cannot find package "code.google.com/p/winsvc/winapi" in any of:

So sorry looks like gowin actually uses winsvc? Seems a little unfortunate but probably OK.

)

const (
RunDir = `Software\Microsoft\Windows\CurrentVersion\Run`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it seems like this should just be a local variable in CreateLaunchFile since it's not used anywhere else and just adds to the global state otherwise.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, true. I usually prefer declaring constants at the top of a file regardless, but I went ahead and moved it inside the function body.

Choose a reason for hiding this comment

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

Cool yeah I guess its partly stylistic.
On Apr 14, 2015 8:05 AM, "atavism" notifications@github.com wrote:

In src/github.com/getlantern/launcher/launcher_windows.go
#175 (comment)
:

@@ -0,0 +1,38 @@
+// Package launcher configures Lantern to run on system start
+package launcher
+
+import (

  • "github.com/kardianos/osext"
  • "github.com/luisiturrios/gowin"
  • "github.com/getlantern/golog"
    +)

+const (

  • RunDir = Software\Microsoft\Windows\CurrentVersion\Run

Yeah, true. I usually prefer declaring constants at the top of a file
regardless, but I went ahead and moved it inside the function body.


Reply to this email directly or view it on GitHub
https://github.com/getlantern/flashlight-build/pull/175/files#r28335161.

@myleshorton
Copy link
Contributor

Thanks on the winsvc front. I still am not sure what's going on, but if I build the installer from this branch using:

VERSION=2.0.0-beta5 make package-windows

The resulting installer does two odd things:

  1. It doesn't have a settings button
  2. It goes directly to the old welcome screen

Both make it seem like it's using an old version of the UI. Do you see the same things @atavism? This could also be related to cache settings -- it's possible the browser's just caching the old copy of the UI.

Oh, also, I can't see to get it to affect the registry. I also checked, and there aren't any stray 2.0.0-beta5 tags lying around that might make it point to the wrong version.

@atavism
Copy link
Author

atavism commented Apr 15, 2015

@myleshorton Yeah, this was happening to me too at some point. Did tarfs update the resources.go file after you built the installers? Also, you might try running UPDATE_DIST=true make docker-genassets prior to that (though the latest changes should already be committed and I see them in the diff).

@xiam xiam mentioned this pull request Apr 15, 2015
@myleshorton
Copy link
Contributor

OK @atavism this issue with the outdated UI on Windows is gone for me now, but it's unfortunately still not successfully updating the registry when I toggle the auto-start setting.

@myleshorton
Copy link
Contributor

Actually it looks like the auto-start checkbox is controlling stats reporting for me. The logs show:

flashlight.statreporter: Stat reporting turned off

Oddly clicking either checkbox on or off seems to always turn stat reporting off. This is also in Firefox so could be related to that and not the browser. I'll check Chrome on Windows.

@atavism
Copy link
Author

atavism commented Apr 16, 2015

@myleshorton You pulled in the latest changes, right? Are you looking for the registry key in Software\Microsoft\Windows\CurrentVersion\Run under HKCU?

@atavism
Copy link
Author

atavism commented Apr 16, 2015

It's not actually turning stat reporting on/off.. that just means config updates are propagating. those messages are always printed.

@myleshorton
Copy link
Contributor

Hey thanks @atavism. Actually, it looks like it's correctly turning it on but just not off.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling abf0488 on launcher into * on valencia*.

@atavism
Copy link
Author

atavism commented Apr 17, 2015

@myleshorton Thanks. Just fixed another issue with the path we were using.

  • Here are the two tests I'm running in addition to checking the registry key gets added/removed. Note: the run on system start setting is disabled by default
- check "Run Lantern on system start" in Settings 
  Quit Lantern 
  Logout and log back in 
  -> Lantern should start
- uncheck "Run Lantern on system start" in Settings
  Quit Lantern
  Logout and log back in 
  -> Lantern shouldn't start at all

@myleshorton
Copy link
Contributor

OK this is working for me. I'm going to merge since the test failure has been fixed on valencia (ui resources again).

myleshorton added a commit that referenced this pull request Apr 20, 2015
Add setting to run Lantern on system startup
@myleshorton myleshorton merged commit 0a17866 into valencia Apr 20, 2015
@myleshorton myleshorton deleted the launcher branch April 20, 2015 20:19
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