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

Download and use MSYS/make locally for Windows postinstall #792

Merged
merged 24 commits into from
Feb 17, 2023
Merged

Download and use MSYS/make locally for Windows postinstall #792

merged 24 commits into from
Feb 17, 2023

Conversation

snnz
Copy link
Contributor

@snnz snnz commented Mar 12, 2022

This makes unnecessary to install MSYS manually and avoids the problem of the incorrect make versions in the PATH.

npm-scripts.js Outdated Show resolved Hide resolved
worker/scripts/getmake.py Outdated Show resolved Hide resolved
worker/scripts/getmake.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Makes sense to me overall. Did you test it as a git dependency on Windows already?

npm-scripts.js Outdated

function addMsysToPath()
{
process.env['PATH'] = process.cwd() + '\\worker\\out\\msys\\bin;' + process.env['PATH'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

process.cwd() is probably not a good idea, since it can be called from anywhere. __dirname should be a better alternative

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the paths in all commands are relative to the cwd.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm... yes, other paths are implicitly relative to cwd, but that isn't necessarily what they should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yet this is what they are. 🙂 Currently it looks like the script (with package.json) can be anywhere, but everything else should be in the current directory, rather than vice versa.

@snnz
Copy link
Contributor Author

snnz commented Mar 13, 2022

I tested it from a local tar obtained by npm pack.

@nazar-pc
Copy link
Collaborator

You need to make linter happy

npm-scripts.js Outdated Show resolved Hide resolved
Co-authored-by: Nazar Mokrynskyi <[email protected]>
@nazar-pc nazar-pc requested review from ibc and jmillan March 13, 2022 04:41
@nazar-pc
Copy link
Collaborator

nazar-pc commented Apr 2, 2022

@snnz would you be so kind porting this to Rust as well, please?

@snnz
Copy link
Contributor Author

snnz commented Apr 3, 2022

Unfortunately, I do not know Rust at all. But since it is not a big task, I've tried to write something similar to the Node version looking at the examples. Probably it is clumsy, but it works. However, I didn't manage to complete the installation, because it failed with an error "Filename longer than 260 characters". There are some really long names like "subprojects/usrsctp-9d6b99b10a70f7a63d21cd80d03c353da9ac19d3/libusrsctp.a.p/C__Users_sergeyn_.cargo_registry_src_github.com-1ecc6299db9ec823_mediasoup-sys-0.3.3_subprojects_usrsctp-9d6b99b10a70f7a63d21cd80d03c353da9ac19d3_usrsctplib_netinet_sctp_cc_functions.c.obj". How do people deal with this?

I also noticed that the script cpu_cores.sh tries to run nproc in MinGW environment and cannot find it. I wonder what MSYS package contains it (if any)? It is not installed with msys-base.

@nazar-pc
Copy link
Collaborator

nazar-pc commented Apr 3, 2022

cargo fmt is necessary to run to fix formatting and let CI pass that step.

nproc must be present in one of the MinGW packages, I don't have access to Windows VM right now to check which one it came from.

C__Users_sergeyn_.cargo_registry_src_github.com-1ecc6299db9ec823_mediasoup-sys-0.3.3_subprojects_usrsctp-9d6b99b10a70f7a63d21cd80d03c353da9ac19d3_usrsctplib_netinet_sctp_cc_functions.c.obj is a ridiculous file name, I'll see what can be done about that.

@snnz
Copy link
Contributor Author

snnz commented Apr 3, 2022

cargo fmt is done.

But as to the nproc, I believe, NUMBER_OF_PROCESSORS environment variable should be used instead in Windows as more reliable (NUM_CORES=$NUMBER_OF_PROCESSORS in cpu_cores.sh)

@nazar-pc
Copy link
Collaborator

nazar-pc commented Apr 3, 2022

If it works in both native Windows and MinGW environment - sure, let's use that.

@snnz
Copy link
Contributor Author

snnz commented Apr 3, 2022

I have added it to the PR, since whether nproc exists somewhere in MinGW or not, this subset of MSYS definitely does not install it.

npm-scripts.js Outdated Show resolved Hide resolved
npm-scripts.js Outdated Show resolved Hide resolved
npm-scripts.js Outdated Show resolved Hide resolved
npm-scripts.js Outdated Show resolved Hide resolved
npm-scripts.js Outdated Show resolved Hide resolved
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.

Thanks a lot.
Is this ready? Tested?

@ibc
Copy link
Member

ibc commented Feb 17, 2023

So? Is this ready?

@snnz
Copy link
Contributor Author

snnz commented Feb 17, 2023

Yes, I suppose so. Github's ci runs without problems, and I also use it successfully to upgrade mediasoup in my dev system.

@ibc
Copy link
Member

ibc commented Feb 17, 2023

Running GH CI here. Merging once it passes. Thanks!

npm-scripts.js Outdated Show resolved Hide resolved
@ibc ibc merged commit 890a259 into versatica:v3 Feb 17, 2023
@ibc
Copy link
Member

ibc commented Feb 17, 2023

Released in 3.11.10. Thanks!

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.

3 participants