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

Support launching the devtools server on a random port #25907

Closed
jdm opened this issue Mar 5, 2020 · 11 comments
Closed

Support launching the devtools server on a random port #25907

jdm opened this issue Mar 5, 2020 · 11 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Mar 5, 2020

The latest OS images for HoloLens apparently have something running on port 6000, which is the default port we choose for the devtools server. We should support launching the server on a random free port and reporting that port to the embedding layer, so that it can be surfaced to the user in an appropriate manner through the app UI.

@highfive
Copy link

@highfive highfive commented Mar 5, 2020

@jdm
Copy link
Member Author

@jdm jdm commented Mar 5, 2020

This will require:

  • passing port 0 in this code, calling local_addr to find out the actual port in use
  • adding a new EmbedderMsg variant for reporting the devtools server is running
  • using embedder to send the new message with the port value
  • handling the new message appropriately
@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Mar 6, 2020

I would like to work on this issue 🙂

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Mar 6, 2020

@jdm I am a bit confused about the handling of the new message part. I could not find a place where the port is currently stored other than the Opts struct which, in case a random port is allocated, will have the wrong port. I am thinking of updating the Opts instance by introducing new getter and setter functions for the devtools_port option. Should I proceed with this or is a different behaviour expected here?

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Mar 6, 2020

Then again I am not sure if Opts will be used for displaying the configs to the user.

@jdm
Copy link
Member Author

@jdm jdm commented Mar 6, 2020

I recommend adding a new HostTrait method that accepts the port number as an argument, so each servo embedder can decide what to do with it. Then we'll want an addition to CHostCallbacks that allows a user of C API to decide what to do with it. That will lead us to this code where we can use a stub function that does nothing for the time being.

You'll want to build with --libsimpleservo to ensure you're building the ports/libsimpleservo code when making these changes.

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Mar 7, 2020

@jdm I am having trouble interfacing the method in C API. I cannot figure out what is missing here. Also the error message is not really helpful. Do you mind taking a look?
master...kunalmohan:25907-DevtoolsServer

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Mar 9, 2020

Also the error message is not really helpful

What's the error message?

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Mar 9, 2020

error: failed to run custom build command for `simpleservo_capi v0.0.1 (/mnt/sda1/Documents/web/rust/servo/ports/libsimpleservo/capi)`

Caused by:
  process didn't exit successfully: `/mnt/sda1/Documents/web/rust/servo/target/debug/build/simpleservo_capi-9fd08d64afff3a73/build-script-build` (exit code: 101)
--- stderr
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: NotPresent', ports/libsimpleservo/capi/build.rs:10:22
stack backtrace:
   0:     0x55968e228aa4 - backtrace::backtrace::libunwind::trace::he7d38fb68e11d902
                               at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.44/src/backtrace/libunwind.rs:86
   1:     0x55968e228aa4 - backtrace::backtrace::trace_unsynchronized::h29c61f1b7ae1f71f
                               at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.44/src/backtrace/mod.rs:66
   2:     0x55968e228aa4 - std::sys_common::backtrace::_print_fmt::hf442c6eaf0984cac
                               at src/libstd/sys_common/backtrace.rs:78
   3:     0x55968e228aa4 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h9c0ab4dfe9f0ca3c
                               at src/libstd/sys_common/backtrace.rs:59
   4:     0x55968e24ef5c - core::fmt::write::h166aeb88877a83bc
                               at src/libcore/fmt/mod.rs:1053
   5:     0x55968e225987 - std::io::Write::write_fmt::h2cb1f0a90c9c57d3
                               at src/libstd/io/mod.rs:1428
   6:     0x55968e22b8e5 - std::sys_common::backtrace::_print::hdbf035ea4e80a708
                               at src/libstd/sys_common/backtrace.rs:62
   7:     0x55968e22b8e5 - std::sys_common::backtrace::print::h375187bb60001fa7
                               at src/libstd/sys_common/backtrace.rs:49
   8:     0x55968e22b8e5 - std::panicking::default_hook::{{closure}}::hbc515a7671acbfd7
                               at src/libstd/panicking.rs:204
   9:     0x55968e22b631 - std::panicking::default_hook::hfa86fd19288a8449
                               at src/libstd/panicking.rs:224
  10:     0x55968e22bef2 - std::panicking::rust_panic_with_hook::hc3cb523914ca9ae4
                               at src/libstd/panicking.rs:470
  11:     0x55968e22badb - rust_begin_unwind
                               at src/libstd/panicking.rs:378
  12:     0x55968e24d921 - core::panicking::panic_fmt::h16921e48cfda0697
                               at src/libcore/panicking.rs:85
  13:     0x55968e24d743 - core::option::expect_none_failed::h18074c5f13b79377
                               at src/libcore/option.rs:1211
  14:     0x55968dced07c - core::result::Result<T,E>::unwrap::h3dda7e2fa72671d6
                               at /rustc/4ad62488258972bdb0e2df225d100f99ef58dbad/src/libcore/result.rs:1003
  15:     0x55968dd258cb - build_script_build::main::h6670cc166ed111c1
                               at ports/libsimpleservo/capi/build.rs:10
  16:     0x55968dcf195b - std::rt::lang_start::{{closure}}::h7b55d5f7892b4ba3
                               at /rustc/4ad62488258972bdb0e2df225d100f99ef58dbad/src/libstd/rt.rs:67
  17:     0x55968e22ba03 - std::rt::lang_start_internal::{{closure}}::h060ffe2c16c8e6ff
                               at src/libstd/rt.rs:52
  18:     0x55968e22ba03 - std::panicking::try::do_call::he1804851e947f83f
                               at src/libstd/panicking.rs:303
  19:     0x55968e2335a7 - __rust_maybe_catch_panic
                               at src/libpanic_unwind/lib.rs:86
  20:     0x55968e22c469 - std::panicking::try::h698825ef9758de7b
                               at src/libstd/panicking.rs:281
  21:     0x55968e22c469 - std::panic::catch_unwind::h40785eb45c80cc2d
                               at src/libstd/panic.rs:394
  22:     0x55968e22c469 - std::rt::lang_start_internal::hc5a9988fa147d6fa
                               at src/libstd/rt.rs:51
  23:     0x55968dcf1937 - std::rt::lang_start::h649e712d313ae0c4
                               at /rustc/4ad62488258972bdb0e2df225d100f99ef58dbad/src/libstd/rt.rs:67
  24:     0x55968dd25e3a - main
  25:     0x7f3157529b6b - __libc_start_main
  26:     0x55968dce53ca - _start
  27:                0x0 - <unknown>

warning: build failed, waiting for other jobs to finish...
error: build failed
@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Mar 10, 2020

Issue with ports/libsimpleservo/capi/build.rs ? Wondering why CARGO_TARGET_DIR would be missing. How did you run your build? ./mach build? Does it build without your patch?

Few comments:

@kunalmohan
Copy link
Collaborator

@kunalmohan kunalmohan commented Mar 10, 2020

I ran ./mach check --libsimpleservo and yes it builds without my patch.
The link that you included in the first point isn't opening for me but I guess I know which line you're referring to, so I'll add it there and will open a PR asap for you to review 🙂

EDIT: I just ran a release build and simpleservo_capi builds now. Hmn.../mach check doesn't compile the C code I guess.

@atouchet atouchet added this to In progress in HoloLens Mar 10, 2020
bors-servo added a commit that referenced this issue Mar 16, 2020
Add support for launching devtools server on random port

In case the default port(6000) or the port specified by user for
devtools server is already taken, random port will be assigned to
it which is reported to the embedding layer for display to user.

r?@jdm @paulrouget

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25907  (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Mar 16, 2020
Add support for launching devtools server on random port

In case the default port(6000) or the port specified by user for
devtools server is already taken, random port will be assigned to
it which is reported to the embedding layer for display to user.

r?@jdm @paulrouget

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25907  (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@atouchet atouchet moved this from In progress to Done in HoloLens Mar 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
HoloLens
  
Done
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.