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

win support #329

Merged
merged 44 commits into from
Oct 6, 2019
Merged

win support #329

merged 44 commits into from
Oct 6, 2019

Conversation

haiyangwu
Copy link
Contributor

@haiyangwu haiyangwu commented Aug 29, 2019

Installation for Win

ensure your host satisfies the following requirements:

  • Visual Studio 2015/2017
  • Add the path of MSBuild.exe to System Environment Variable (eg. C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\15.0\Bin)
  • Add GYP_MSVS_VERSION to System Environment Variable (eg. 2017 for visual studio 2017 )

key changes:

supported scripts:

  • lint
  • format:worker
  • test:node
  • coverage:node
  • postinstall

not supported scripts:

  • test:worker (haven't found an alternative tool to replace lcov in windows, but running in visual studio is ok )

not sure

  • changes about channels and pipes

@haiyangwu haiyangwu marked this pull request as ready for review August 29, 2019 07:28
@saghul
Copy link
Contributor

saghul commented Aug 29, 2019

* **using two pipes to replace the duplex pipe to avoid deadlock**, the related bug in NodeJS: [nodejs/node#29238](https://github.com/nodejs/node/issues/29238)

That is solved by passing the UV_OVERLAPPED_PIPE flag when creating the pipe. It's a noop on Unix. See: libuv/libuv@62a0f76

@jmillan
Copy link
Member

jmillan commented Aug 29, 2019

looks great @haiyangwu,

  • using two pipes to replace the duplex pipe to avoid deadlock, the related bug in NodeJS:
    nodejs/node#29238

That is solved by passing the UV_OVERLAPPED_PIPE flag when creating the pipe. It's a noop on >Unix. See: libuv/libuv@62a0f76

Could you please try the UV_OVERLAPPED_PIPE flag and adapt the PR accordingly?

.eslintrc.js Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
gulpfile.js Outdated Show resolved Hide resolved
lib/Channel.js Outdated Show resolved Hide resolved
worker/src/Settings.cpp Outdated Show resolved Hide resolved
worker/src/Settings.cpp Outdated Show resolved Hide resolved
worker/src/Worker.cpp Outdated Show resolved Hide resolved
worker/src/handles/TcpConnection.cpp Outdated Show resolved Hide resolved
worker/test/include/helpers.hpp Outdated Show resolved Hide resolved
@ibc
Copy link
Member

ibc commented Aug 29, 2019

It's not enough. The bug is also in Node usage of libuv.

worker/include/opt/getopt.h Outdated Show resolved Hide resolved
@versatica versatica deleted a comment Aug 30, 2019
@versatica versatica deleted a comment Aug 30, 2019
@versatica versatica deleted a comment Aug 30, 2019
gulpfile.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/test-Worker.js Show resolved Hide resolved
worker/deps/getopt/getopt.gyp Outdated Show resolved Hide resolved
worker/include/Channel/Request.hpp Outdated Show resolved Hide resolved
worker/src/Channel/Request.cpp Outdated Show resolved Hide resolved
@ibc
Copy link
Member

ibc commented Oct 1, 2019

Hi, @haiyangwu. The "devel" branch with work-in-progress transport-cc support is now in GitHub:

https://github.com/versatica/mediasoup/tree/devel

May you please try to merge your branch with Win support into the new "devel" branch? Some notes:

  • If you need to modify any file under worker/deps/libwebrtc, please let's know first as those are files from Google's libwebrtc.
  • I hope you don't need to modify anything in worker/deps/libwebrtc/deps/abseil-cpp, but if so, please let's also know. May be modifications into its .gyp` file are required.

@haiyangwu
Copy link
Contributor Author

@ibc Merged and all tests passed. Just like your comment, require some modifications in worker/deps/libwebrtc/deps/abseil-cpp's gyp file which is also described abseil-cpp/CMake/README.md

@haiyangwu
Copy link
Contributor Author

@ibc 🤔Checks failed. Is the config wrong? May you please check it.

@ibc
Copy link
Member

ibc commented Oct 4, 2019

The failing travis task is this:

../deps/libwebrtc/deps/abseil-cpp/abseil-cpp/absl/base/policy_checks.h:77:2: error: #error "C++ versions less than C++11 are not supported."
 #error "C++ versions less than C++11 are not supported."
  ^
In file included from ../deps/libwebrtc/deps/abseil-cpp/abseil-cpp/absl/strings/string_view.h:51:0,
                 from ../deps/libwebrtc/deps/abseil-cpp/abseil-cpp/absl/strings/match.h:38,
                 from ../deps/libwebrtc/deps/abseil-cpp/abseil-cpp/absl/strings/match.cc:15:
../deps/libwebrtc/deps/abseil-cpp/abseil-cpp/absl/base/internal/throw_delegate.h:39:1: error: expected unqualified-id before ‘[’ token
 [[noreturn]] void ThrowStdLogicError(const std::string& what_arg);
 ^

It's like we need to force C++11 in the gyp of abseil-cpp, am I right?

@ibc
Copy link
Member

ibc commented Oct 4, 2019

NOTE: We did never run the "deve" branch in Travis, so...

Comment on lines 42 to 51
'defines': [
'NOMINMAX', # Don't define min and max macros (windows.h)
# Don't bloat namespace with incompatible winsock versions.
'WIN32_LEAN_AND_MEAN',
# Don't warn about usage of insecure C functions.
'_CRT_SECURE_NO_WARNINGS',
'_SCL_SECURE_NO_WARNINGS',
# Introduced in VS 2017 15.8, allow overaligned types in aligned_storage
'_ENABLE_EXTENDED_ALIGNED_STORAGE'
]
Copy link
Member

Choose a reason for hiding this comment

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

NOTE: worker/deps/abseil-cpp has been removed in "devel" branch. It's now just in worker/deps/libwebrtc/deps/abseil-cpp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok... New commits in devel branch merged.

@ibc
Copy link
Member

ibc commented Oct 4, 2019

It's like we need to force C++11 in the gyp of abseil-cpp, am I right?

I've added it in devel branch: 780d1c1

@ibc
Copy link
Member

ibc commented Oct 4, 2019

BTW, regarding run-script-os dep now I'm 100% confused. I understood in the past that we should use this one, however the same author is also updating this one.

I've asked the author (@kawatzaki) here :)

@kawatzaki
Copy link

Hi fellas, don't worry about @dr.amaton/run-script-os, we just updated to 1.1.3, which shouldn't break anything at all, I was just clarifying which repo should be updated and which should be left alone....

Keep this project going!

@ibc
Copy link
Member

ibc commented Oct 4, 2019

Thanks a lot for the clarification, @kawatzaki :)

@ibc
Copy link
Member

ibc commented Oct 5, 2019

@haiyangwu can you please make this PR against the "devel" branch instead? We don't want to merge "devel" into "v3" yet (it's not ready).

@haiyangwu haiyangwu changed the base branch from v3 to devel October 6, 2019 01:43
@haiyangwu
Copy link
Contributor Author

@haiyangwu can you please make this PR against the "devel" branch instead? We don't want to merge "devel" into "v3" yet (it's not ready).

This PR's base branch already switched to "devel" branch. BTW, doc works about "requirements" as below still need to add, please let me know when the proper time and tell me where it should be put.

ensure your host satisfies the following requirements:

  • Visual Studio 2015/2017
  • Add the path of MSBuild.exe to System Enviroment (eg. C:\Program Files (x86)\MSBuild\14.0\Bin)
  • Add GYP_MSVS_VERSION to System Enviroment (eg. 2017 for visual studio 2017 )

Copy link
Member

@ibc ibc left a comment

Choose a reason for hiding this comment

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

Just two minor changes, please (two dots) :)

worker/deps/libwebrtc/deps/abseil-cpp/abseil-cpp.gyp Outdated Show resolved Hide resolved
worker/include/common.hpp Outdated Show resolved Hide resolved
@ibc
Copy link
Member

ibc commented Oct 6, 2019

@haiyangwu can you please make this PR against the "devel" branch instead? We don't want to merge "devel" into "v3" yet (it's not ready).

This PR's base branch already switched to "devel" branch.

Great, please just add two missing dots (as requested) and I'll merge it.

BTW, doc works about "requirements" as below still need to add, please let me know when the proper time and tell me where it should be put.

ensure your host satisfies the following requirements:

  • Visual Studio 2015/2017
  • Add the path of MSBuild.exe to System Enviroment (eg. C:\Program Files (x86)\MSBuild\14.0\Bin)
  • Add GYP_MSVS_VERSION to System Enviroment (eg. 2017 for visual studio 2017 )

Don't worry, I'll add those requirements in the mediasoup website :)

@ibc ibc merged commit a42e5c4 into versatica:devel Oct 6, 2019
@ibc
Copy link
Member

ibc commented Oct 6, 2019

Hi @haiyangwu, merged! so many thanks! This will be eventually merged into v3 once we end dealing with transport-cc. BTW let me two questions:

  1. Is this ok and accurate?: https://mediasoup.org/documentation/v3/mediasoup/installation/#windows

  2. I've added you to Acknowledgements. Is Haiyang Wu your name? and do you have a preferred personal site?

@haiyangwu
Copy link
Contributor Author

@ibc My pleasure.

  1. Is this ok and accurate?: https://mediasoup.org/documentation/v3/mediasoup/installation/#windows

For accurate, please change a little bit.

  • Append the path of MSBuild.exe folder to PATH in Windows environment variable (e.g. C:\Program Files (x86)\MSBuild\14.0\Bin).
  • Create a new Windows environment variable GYP_MSVS_VERSION to demostrate Vistual Studio's version (e.g. 2017 for Visual Studio 2017).
  1. I've added you to Acknowledgements. Is Haiyang Wu your name? and do you have a preferred personal site?

Thanks again and yes my name is Haiyang Wu. Linking to Github is ok.

@ibc
Copy link
Member

ibc commented Oct 7, 2019

Thanks, done.

@ibc
Copy link
Member

ibc commented Oct 29, 2019

@haiyangwu may you please check whether the new "scripts" in package.json in the typescript branch run fine in Windows?

@ibc
Copy link
Member

ibc commented Oct 29, 2019

Also @haiyangwu, there is an important issue due the usage of "@dr.amaton/run-script-os":

kawatzaki/dr.amaton-run-script-os#1

I hope @kawatzaki can take a look to it since this's a show stopper for me right now :)

@kawatzaki
Copy link

I'll be checking it out, I'm currently at lunch!

But I'll get it fixed today

@haiyangwu
Copy link
Contributor Author

Also @haiyangwu, there is an important issue due the usage of "@dr.amaton/run-script-os":

kawatzaki/dr.amaton-run-script-os#1

I hope @kawatzaki can take a look to it since this's a show stopper for me right now :)

@ibc I saw the issue closed (add extra --no-arguments. It works in OSX now? I just want to confirm and I'll check it in Windows.

@ibc
Copy link
Member

ibc commented Oct 30, 2019

Yes, with --no-arguments it works (pushed to "devel" branch).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants