Skip to content

Conversation

@FilippoPolo
Copy link

Tightened the installer so that it checks for every static file, and also checks a "version" file, including when the kernel is started from the notebook. Changes to kernel.js to support starting with no config file, equivalent to Colin's (got rid of some replication of paths).

@cgravill
Copy link
Member

Do you know what the idea behind the argument check is? I'm wondering if the else branch is still compatible with these changes

if args.Length = 0 then

    InstallAndStart(true, true)

else
    // Verify kernel installation status
    InstallAndStart(false, false)

    // Clear the temporary folder
    try
      if Directory.Exists(Config.TempDir) then Directory.Delete(Config.TempDir, true)
    with exc -> Console.Out.Write(exc.ToString())

    // adds the default display printers
    Printers.addDefaultDisplayPrinters()

    // get connection information
    let fileName = args.[0]
    let json = File.ReadAllText(fileName)
    let connectionInformation = JsonConvert.DeserializeObject<ConnectionInformation>(json)

    // start the kernel
    Kernel <- Some (IfSharpKernel(connectionInformation))
    Kernel.Value.StartAsync()

    // block forever
    Thread.Sleep(Timeout.Infinite)

@cgravill
Copy link
Member

Ensuring a consistent state with the installer looks like a good change!

@FilippoPolo
Copy link
Author

The argument check is used to figure out whether I'm being run directly, or by Jupyter. When a kernel is started by Jupyter, it has an argument (the location where to find connection information). Before my change, in this condition it would never validate the installation. It's working correctly for me on either branch, though more testing is welcome.

@cgravill
Copy link
Member

Makes sense and I see how your safer installation will work better there too. I'd tested it but was curious.

I'll merge this now.

@cgravill cgravill merged commit f57b3a7 into fsprojects:jupyter Jul 22, 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.

3 participants