Start web server after building the documentation #2574

joschmitt · 2023-07-19T14:39:07Z

As discussed in Slack, the open_browser option of build_doc does not work (at least) on Ubuntu's sandboxed snap-firefox. I added a comment regarding this to the documentation (first commit).
@benlorenz suggested to start a web server using LiveServer.jl in the build directory, so I also tried this.
I'm not really familiar with the setup for building the documentation and eventually added LiveServer as a dependency at every place I could find because this seemed to be the only way to make it work. Is there a better way and do we want to have this dependency at all?

As of now this will of course simply start the server, but not update anything, when the user edits the documentation (like the LiveServer.servedocs function claims to do).

benlorenz · 2023-07-19T15:35:39Z

I don't really like adding it as a dependency.
Maybe we can just run it via a separate julia process and keep the liveserver process in the background, storing the process object in a variable to make sure we start it only once. (Stopping it with a Base.atexit hook).

The command line might look like this:

julia -e 'using Pkg; Pkg.activate(temp=true); Pkg.add("LiveServer"); using LiveServer; LiveServer.serve(dir="$(docspath), open_browser=$(open_browser)");'

I don't know if we want to try to integrate the servedocs variant, it does not work for adjusted docstrings anyway (only .md files). Maybe the best approach is to have users run the normal docs build again (with Revise) and keep the server + browser running, just refreshing the page after the build is done again.

codecov · 2023-07-19T18:37:36Z

Codecov Report

Merging #2574 (7af4c60) into master (e38b18b) will increase coverage by 0.00%.
Report is 1 commits behind head on master.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master    #2574   +/-   ##
=======================================
  Coverage   73.20%   73.20%           
=======================================
  Files         446      446           
  Lines       63676    63678    +2     
=======================================
+ Hits        46615    46618    +3     
+ Misses      17061    17060    -1

Files Changed	Coverage
src/utils/docs.jl	`0.00%`

joschmitt · 2023-07-20T08:38:36Z

Here is a version that starts LiveServer as a background process. For me, this is fine, but there are some nuisances:

If the background process fails, the user will not get any feedback. I tried to redirect stderr (but not stdout) for this, but somehow one either gets stderr AND stdout or nothing?
If there is already something running at port 8000, LiveServer chooses another port and I have no idea how to find out which (other than reading the output from stdout...).
If one kills the main julia process via SIGKILL, the atexit does not get executed and the background process stays alive.

joschmitt · 2023-07-20T09:31:42Z

~~Looks like another occurrence of #2541 (in this case not on nightly).~~
Well, not any more.

joschmitt · 2023-07-24T12:44:06Z

Small update on my comments above: one can in fact redirect only stderr, it's just that Pkg.activate prints to stderr by default, but one can also redirect that. Then again, I'm not too convinced any more that one should print the stderr of the background process to the calling julia session as it will just result in out-of-context error messages.

benlorenz · 2023-07-24T13:10:40Z

I am still in favor of doing this, but I would love some other opinions on whether we should add this.

Maybe we should put live_server_process in a global variable to be able to check whether it is still running when calling the function a second time?
And I think hiding the error output should be fine.

joschmitt · 2023-07-26T10:29:02Z

There was a short discussion on this in the Wednesday's meeting in Kaiserslautern:

Potentially @aaruni96 is interested in this
We could consider the (third?) option of providing the functionality in principle, but telling the user to install LiveServer by themselves, if they want to use it.

fingolfin

I haven't tested this, but it looks great, thanks!

src/utils/docs.jl

joschmitt · 2023-07-28T07:54:23Z

As far as I know, @aaruni96 wanted to work on an alternative solution... For the record, the following approaches have been mentioned/tried out:

Add LiveServer.jl as a dependency. We don't want that.
Start a background process that installs LiveServer.jl in a temporary environment and starts the server (the current state of this PR). Comes with some drawbacks, most notably that we start a background process. We could improve the interface a bit by remembering in a global variable (or maybe we can even check?) whether a server is already running.
Only provide a function that starts the server and tell the user that they have to install LiveServer.jl themselves. Would also be fine for me; the test_module function also tells you that you need to do using Test after all.
Aaruni's multithreading solution.

(Un)fortunately, I'm on holidays the next two weeks, so if I am further involved in this it has to wait until after that.

aaruni96 · 2023-07-28T09:00:08Z

I apologize, I was refreshing some julia multiprocessing while thinking about this PR and forgot to update here.

The solution I propose such:

We can spawn a julia worker process (addprocs() from Distributed) , and ask it to load LiveServer in a temporary environment. We can then check if the server is still running by asking the status of the future, or kill it by sending an interrupt to the worker.

This would require adding Distributed to Oscar dependencies (currently, its only a dependency for the tests).

The solution should work on single core CPUs as well (the OS can schedule multiple jobs), but its something I am not fully sure about.

fingolfin · 2023-08-23T09:49:05Z

@aaruni96 I think adding a dependency on the stdlib Distributed is fine, so feel free to go ahead with providing an alternate PR.

fingolfin · 2023-08-30T13:17:45Z

@aaruni96 look into an alternative implementation, but it turns out that the approach here also allows some control over the subprocess running the webserver (should we want to), which we were unaware of before. So the main reason for an alternate approach really does not hold.

So I would just merge this PR -- we can still improve it later, if we want to (as with any feature).

@joschmitt you marked it as draft again -- I assume because of our discussion, but I want to double check just in case you had other reasons?

build

src/utils/docs.jl

fingolfin · 2023-08-30T13:25:56Z

src/utils/docs.jl

+         Pkg.add(\"LiveServer\"); using LiveServer;
+         LiveServer.serve(dir = \"$build_dir\", launch_browser = $open_browser, port = $port);"
+  live_server_process = run(`julia -e $cmd`, wait = false)
+  atexit(_ -> kill(live_server_process))


Did you verify this works? Because when I just tried this by copy&pasting this code, kill was just ignored. I suspect this was because I am using juliaup, which actually forks julia into a subprocess...

Okay... I'm very sure it worked for me at some point. I will try again.

So yes, for me it works.

Could you try again? Maybe it is fixed by 7af4c60 as @benlorenz suggested.

I tried it now with juliaup and it works (with 7af4c60). Without that commit it doesn't, so I assume that this is fixed.

joschmitt · 2023-08-30T13:26:01Z

@joschmitt you marked it as draft again -- I assume because of our discussion, but I want to double check just in case you had other reasons?

Yes, I wanted to avoid that somebody merges this "accidentally". Let me add your suggestions and then I will mark it as ready for review.

src/utils/docs.jl

Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>

joschmitt force-pushed the my_browser_knows_better_what_to_open_than_me branch from 3bf1ade to 30901b6 Compare July 19, 2023 14:55

joschmitt force-pushed the my_browser_knows_better_what_to_open_than_me branch from 30901b6 to 4e9c25c Compare July 20, 2023 08:32

joschmitt marked this pull request as ready for review July 20, 2023 08:43

joschmitt added the Decision label Jul 26, 2023

fingolfin approved these changes Jul 28, 2023

View reviewed changes

src/utils/docs.jl Outdated Show resolved Hide resolved

src/utils/docs.jl Outdated Show resolved Hide resolved

joschmitt removed the Decision label Aug 23, 2023

joschmitt marked this pull request as draft August 23, 2023 10:24

joschmitt added 3 commits August 30, 2023 15:22

Add information on browser reporting "access denied" for documentation

6691ea9

Start LiveServer.jl as background process to serve documentation after

ed6cafc

build

Typos

2fda375

fingolfin reviewed Aug 30, 2023

View reviewed changes

src/utils/docs.jl Outdated Show resolved Hide resolved

fingolfin reviewed Aug 30, 2023

View reviewed changes

Suggestions from code review

c09de4c

joschmitt force-pushed the my_browser_knows_better_what_to_open_than_me branch from 8651691 to c09de4c Compare August 30, 2023 13:35

joschmitt marked this pull request as ready for review August 30, 2023 13:36

benlorenz reviewed Aug 30, 2023

View reviewed changes

src/utils/docs.jl Outdated Show resolved Hide resolved

julia -> Base.julia_cmd()

7af4c60

Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>

fingolfin merged commit d42af1a into oscar-system:master Aug 31, 2023
13 of 15 checks passed

joschmitt deleted the my_browser_knows_better_what_to_open_than_me branch September 1, 2023 07:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start web server after building the documentation #2574

Start web server after building the documentation #2574

joschmitt commented Jul 19, 2023 •

edited

Loading

benlorenz commented Jul 19, 2023

codecov bot commented Jul 19, 2023 •

edited

Loading

joschmitt commented Jul 20, 2023

joschmitt commented Jul 20, 2023 •

edited

Loading

joschmitt commented Jul 24, 2023 •

edited

Loading

benlorenz commented Jul 24, 2023

joschmitt commented Jul 26, 2023

fingolfin left a comment

joschmitt commented Jul 28, 2023

aaruni96 commented Jul 28, 2023

fingolfin commented Aug 23, 2023

fingolfin commented Aug 30, 2023

fingolfin Aug 30, 2023

joschmitt Aug 30, 2023

joschmitt Aug 30, 2023

joschmitt Aug 31, 2023

joschmitt Aug 31, 2023

joschmitt commented Aug 30, 2023

Start web server after building the documentation #2574

Start web server after building the documentation #2574

Conversation

joschmitt commented Jul 19, 2023 • edited Loading

benlorenz commented Jul 19, 2023

codecov bot commented Jul 19, 2023 • edited Loading

Codecov Report

joschmitt commented Jul 20, 2023

joschmitt commented Jul 20, 2023 • edited Loading

joschmitt commented Jul 24, 2023 • edited Loading

benlorenz commented Jul 24, 2023

joschmitt commented Jul 26, 2023

fingolfin left a comment

Choose a reason for hiding this comment

joschmitt commented Jul 28, 2023

aaruni96 commented Jul 28, 2023

fingolfin commented Aug 23, 2023

fingolfin commented Aug 30, 2023

fingolfin Aug 30, 2023

Choose a reason for hiding this comment

joschmitt Aug 30, 2023

Choose a reason for hiding this comment

joschmitt Aug 30, 2023

Choose a reason for hiding this comment

joschmitt Aug 31, 2023

Choose a reason for hiding this comment

joschmitt Aug 31, 2023

Choose a reason for hiding this comment

joschmitt commented Aug 30, 2023

joschmitt commented Jul 19, 2023 •

edited

Loading

codecov bot commented Jul 19, 2023 •

edited

Loading

joschmitt commented Jul 20, 2023 •

edited

Loading

joschmitt commented Jul 24, 2023 •

edited

Loading