Fix NPM 'postinstall' task in Windows #1187

ibc · 2023-10-20T08:27:41Z

Provide getmake.py with the absolute path in which it must place fetched stuff.

Fixes #1179 Provide `getmake.py` with the absolute path in which it must place fetched stuff.

ibc · 2023-10-20T08:27:56Z

Let's see if CI passes.

ibc · 2023-10-20T08:35:17Z

@nazar-pc please, tell me where mediasoup Rust installation calls the getmake.py script. It obviously does so now it fails since it must pass a mandatory --dir arg. However there is no "getmake" in rust/ directory...

ibc · 2023-10-20T08:35:43Z

@nazar-pc please, tell me where mediasoup Rust installation calls the getmake.py script. It obviously does so now it fails since it must pass a mandatory --dir arg. However there is no "getmake" in rust/ directory...

Sorry, found it. In worker/build.rs.

ibc · 2023-10-20T08:53:24Z

@nazar-pc may you help here please? Here in worker/build.rs:

if !std::path::Path::new("worker/out/msys/bin/make.exe").exists() {
            let python = if Command::new("where")
                .arg("python3.exe")
                .status()
                .expect("Failed to start")
                .success()
            {
                "python3"
            } else {
                "python"
            };

            if !Command::new(python)
                .arg("scripts\\getmake.py")
                .status()
                .expect("Failed to start")
                .success()
            {
                panic!("Failed to install MSYS/make")
            }
        }

I need to make it check if PYTHON env is given so it will use it instead of trying where. Is it ok?

          let python = if env::var("PYTHON") {
              env::var("PYTHON")
          } else if Command::new("where")
              .arg("python3.exe")
              .status()
              .expect("Failed to start")
              .success()
          {
              "python3"
          } else {
              "python"
          };

I need to call getmake.py with --dst ABSOLUTE_PATH_TO_worker\\out\\msys. We are in worker/build.rs so current folder is /xxx/mediasoup/worker but current os working dir is /xxx/mediasoup (not sure about this). How can I get current absolute path to worker directory in Rust so I can append \\out\\msys? I couldn't find any nice way to go.

BTW this PR is making Node+Windows CI pass :) https://github.com/versatica/mediasoup/actions/runs/6585288341/job/17891419544?pr=1187

nazar-pc · 2023-10-20T08:59:51Z

env::var gives you Result, you can use .unwrap_or_else(|| where_stuff) to get path or use match, depending on what looks better
https://doc.rust-lang.org/stable/std/fs/fn.canonicalize.html or https://doc.rust-lang.org/stable/std/path/struct.Path.html#method.canonicalize depending on which type you have, https://doc.rust-lang.org/stable/std/path/struct.Path.html#method.join is used to combine paths. Rust docs are pretty good and have search at the top.

ibc · 2023-10-20T09:03:48Z

env::var gives you Result, you can use .unwrap_or_else(|| where_stuff) to get path or use match, depending on what looks better

What do I need that for? I mean, I assume that env::var("PYTHON") returns a string or None so the condition below is enough, am I wrong?

          let python = if env::var("PYTHON") {
              env::var("PYTHON")
          } else if Command::new("where")

ibc · 2023-10-20T09:05:27Z

env::var gives you Result, you can use .unwrap_or_else(|| where_stuff) to get path or use match, depending on what looks better

What do I need that for? I mean, I assume that env::var("PYTHON") returns a string or None so the condition below is enough, am I wrong?
          let python = if env::var("PYTHON") {
              env::var("PYTHON")
          } else if Command::new("where")

No :) Solving it.

nazar-pc · 2023-10-20T09:34:03Z

returns a string or None

No, it returns Result, which is either Ok or Err (explicit form is Result::Ok and Result::Err, it is an enum).

jmillan · 2023-10-20T10:26:55Z

Hell, why is this failing? https://github.com/versatica/mediasoup/actions/runs/6586227674/job/17894163137?pr=1187

It's complaining about no permissions error:

PermissionError: [Errno 13] Permission denied: 'D:\\a\\mediasoup\\mediasoup\\worker\\out\\msys\\bin\\bashbug'

ibc · 2023-10-20T10:27:39Z

Hell, why is this failing? https://github.com/versatica/mediasoup/actions/runs/6586227674/job/17894163137?pr=1187

It's complaining about no permissions error:

PermissionError: [Errno 13] Permission denied: 'D:\\a\\mediasoup\\mediasoup\\worker\\out\\msys\\bin\\bashbug'

There is no "bashbug" in the whole mediasoup repo.

error: failed to run custom build command for `mediasoup-sys v0.5.4 (D:\a\mediasoup\mediasoup\worker)`

Caused by:
  process didn't exit successfully: `D:\a\mediasoup\mediasoup\target\debug\build\mediasoup-sys-609a6396dde901ad\build-script-build` (exit code: 101)
  --- stdout
  C:\hostedtoolcache\windows\Python\3.9.13\x64\python3.exe

  --- stderr
  Traceback (most recent call last):
    File "D:\a\mediasoup\mediasoup\worker\scripts\getmake.py", line 27, in <module>
      get('https://sourceforge.net/projects/mingw/files/MSYS/Base/bash/bash-3.1.23-1/bash-3.1.23-1-msys-1.0.18-bin.tar.xz/download', '38da5419969ab883058a96322bb0f51434dd4e9f71de09cd4f75b96750944533')
    File "D:\a\mediasoup\mediasoup\worker\scripts\getmake.py", line 18, in get
      tar.extractall(args.dir)
    File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\tarfile.py", line 2045, in extractall
      self.extract(tarinfo, path, set_attrs=not tarinfo.isdir(),
    File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\tarfile.py", line 2086, in extract
      self._extract_member(tarinfo, os.path.join(path, tarinfo.name),
    File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\tarfile.py", line 2159, in _extract_member
      self.makefile(tarinfo, targetpath)
    File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\tarfile.py", line 2200, in makefile
      with bltn_open(targetpath, "wb") as target:
  PermissionError: [Errno 13] Permission denied: 'D:\\a\\mediasoup\\mediasoup\\worker\\out\\msys\\bin\\bashbug'
  thread 'main' panicked at 'Failed to install MSYS/make', worker\build.rs:107:17
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...
Error: The process 'C:\Users\runneradmin\.cargo\bin\cargo.exe' failed with exit code 101

worker/build.rs

nazar-pc · 2023-10-20T10:34:34Z

worker/build.rs

            };

+            let worker_abs_path = env::current_dir().unwrap();


This is incorrect, Rust crates are not supposed to touch anything anywhere in the file system except a location where they are allowed to modify things defined by OUT_DIR environment variable and already stored in out_dir variable (also see mediasoup_out_dir in case that is what you actually need here).

println!("--- mediasoup_out_dir: {}", mediasoup_out_dir); // => mediasoup_out_dir: /Users/ibc/src/v3-mediasoup/target/debug/build/mediasoup-sys-11923be8cb8ca4cd/out/out

That's far from what I want.

That is actually precisely what you want

I don't see that.

if !std::path::Path::new("worker/out/msys/bin/make.exe").exists() { let python = if let Ok(python) = env::var("PYTHON") { python } else if Command::new("where") .arg("python3") .status() .expect("Failed to start") .success() { "python3".to_string() } else { "python".to_string() }; let worker_abs_path = env::current_dir().unwrap(); let dir = worker_abs_path.join("out/msys"); if !Command::new(python) .arg("scripts\\getmake.py") .arg("--dir") .arg(dir) .status() .expect("Failed to start") .success() { panic!("Failed to install MSYS/make") } } env::set_var( "PATH", format!( "{}\\worker\\out\\msys\\bin;{}", env::current_dir() .unwrap() .into_os_string() .into_string() .unwrap(), env::var("PATH").unwrap() ), );

Here we first check mediasoup/worker/out/msys/bin/make.exe.

If it doesn't exist then we run getmake.py and we want to store things in dir (let's see below what dir should be).

Then we do this:
env::set_var( "PATH", format!( "{}\\worker\\out\\msys\\bin;{}", env::current_dir() .unwrap() .into_os_string() .into_string() .unwrap(), env::var("PATH").unwrap() ), );

There we are clearly adding current dir + \worker\out\msys\bin to the PATH.

So it's very clear that dir given to getmake.py must point to current dir + \worker\out\msys as well.

So everything was wrong before or there is something I miss.

In addition, it looks suspicious to me that mediasoup_out_dir in my macOS points to "/Users/ibc/src/v3-mediasoup/target/debug/build/mediasoup-sys-11923be8cb8ca4cd/out/out" (note the double "out" at the end).

It might, but probably because CI worked before too due to GitHub Actions containing correct make, but when people install mediasoup on their machines, suddenly things break because make in PATH is nmake or whatever the thing is that is on Windows by default.

Looks like that, yes. One out is from Cargo, second is "ours".

But how can it be wrong? I mean, mediasoup_out_dir is being used by build.rs to give env MEDIASOUP_OUT_DIR to make libmediasoup-worker. How is possible that it was wrong all this time? I mean, can I just make mediasoup_out_dir be out_dir:

- let mediasoup_out_dir = format!("{}/out", out_dir.replace('\\', "/")); + let mediasoup_out_dir = out_dir.replace('\\', "/");

and things will work?

That part isn't wrong, I meant get_make and where it places downloaded files wasn't correct.

This is gonna be fine: cca0dab

Rust is amazing. I declare a let dir and it later complains because I have "moved" it so I cannot use it in two places and must clone it in one of them.

error[E0382]: borrow of moved value: `dir` --> worker\build.rs:109:39 | 94 | let dir = format!("{}/msys", mediasoup_out_dir.replace('\\', "/")); | --- move occurs because `dir` has type `std::string::String`, which does not implement the `Copy` trait ... 99 | .arg(dir) | --- value moved here ... 109 | format!("{}\\bin;{}", dir, env::var("PATH").unwrap()), | ^^^ value borrowed here after move | = note: this error originates in the macro `$crate::__export::format_args` which comes from the expansion of the macro `format` (in Nightly builds, run with -Z macro-backtrace for more info) help: consider cloning the value if the performance cost is acceptable | 99 | .arg(dir.clone()) | ++++++++

ibc · 2023-10-20T12:13:47Z

Now this:

https://github.com/versatica/mediasoup/actions/runs/6587295108/job/17897281193?pr=1187

[...]

Deleting D:\a\mediasoup\mediasoup\worker\subprojects\packagecache\openssl-3.0.8.tar.gz
  Deleting D:\a\mediasoup\mediasoup\worker\subprojects\packagecache\openssl_3.0.8-1_patch.zip
  Deleting D:\a\mediasoup\mediasoup\worker\subprojects\openssl-3.0.8
  Progress: 8 / 8 ()

rm -f -rf D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out

  --- stderr

  rm: cannot remove `D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out/msys/bin/make.exe': Permission denied
  rm: cannot remove `D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out/msys/bin/msys-1.0.dll': Permission denied
  rm: cannot remove `D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out/msys/bin/msys-iconv-2.dll': Permission denied
  rm: cannot remove `D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out/msys/bin/msys-intl-8.dll': Permission denied
  rm: cannot remove `D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out/msys/bin/rm.exe': Permission denied
  rm: cannot chdir from `D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out/msys' to ..: No such file or directory
  "make": *** [clean-all] Error 1

I give up.

nazar-pc · 2023-10-20T12:22:12Z

I give up.

Looks like you're trying to remove rm, same rm.exe you are using to remove itself 🙃

Try https://doc.rust-lang.org/stable/std/fs/fn.remove_dir_all.html instead

ibc · 2023-10-20T12:23:37Z

Now this:

https://github.com/versatica/mediasoup/actions/runs/6587295108/job/17897281193?pr=1187

[...]

Deleting D:\a\mediasoup\mediasoup\worker\subprojects\packagecache\openssl-3.0.8.tar.gz
  Deleting D:\a\mediasoup\mediasoup\worker\subprojects\packagecache\openssl_3.0.8-1_patch.zip
  Deleting D:\a\mediasoup\mediasoup\worker\subprojects\openssl-3.0.8
  Progress: 8 / 8 ()

rm -f -rf D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out

  --- stderr

  rm: cannot remove `D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out/msys/bin/make.exe': Permission denied
  rm: cannot remove `D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out/msys/bin/msys-1.0.dll': Permission denied
  rm: cannot remove `D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out/msys/bin/msys-iconv-2.dll': Permission denied
  rm: cannot remove `D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out/msys/bin/msys-intl-8.dll': Permission denied
  rm: cannot remove `D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out/msys/bin/rm.exe': Permission denied
  rm: cannot chdir from `D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out/msys' to ..: No such file or directory
  "make": *** [clean-all] Error 1

So make clean-all is failing. It does this:

clean-all: clean-subprojects
$(RM) -rf $(MEDIASOUP_OUT_DIR)

It's clear that clean-subprojects does work.
MEDIASOUP_OUT_DIR' is supposed to be Rust mediasoup_out_dirinbuild.rsbecause it's passed as env tomake`.
So it tries to delete it: rm -f -rf D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out
How is it possible that it later fails because "rm: cannot chdir from `D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out/msys' to ..: No such file or directory"?

Why such a syntax with / instead of \\... oh well, wait. In build.rs:

// Force forward slashes on Windows too so that is plays well with our dumb `Makefile`.
let mediasoup_out_dir = format!("{}/out", out_dir.replace('\\', "/"));

So this is NOT good right? Because we are passing it to our Makefile. But anyway the whole paths in Makefile use/ Unix syntax instead of \\ so...
And anyway, why does it complain about such a "D:/a/mediasoup/mediasoup/target/debug/build/mediasoup-sys-0443c0f553b7fb4e/out/out" path? make clean-all task doesn't do that at all!!!

ibc · 2023-10-20T12:24:45Z

I give up.

Looks like you're trying to remove rm, same rm.exe you are using to remove itself 🙃

Try https://doc.rust-lang.org/stable/std/fs/fn.remove_dir_all.html instead

WHAT??? Do you mean that the rm command used by Rust+Windows is the one being downloaded with getmake.py?????

nazar-pc · 2023-10-20T12:26:45Z

WHAT??? Do you mean that the rm command used by Rust+Windows is the one being downloaded with getmake.py?????

I mean it doesn't download make, it downloads the whole thing and once it is in PATH... Is there an API to extract a single file instead?

ibc · 2023-10-20T12:26:54Z

I give up.

Looks like you're trying to remove rm, same rm.exe you are using to remove itself 🙃
Try https://doc.rust-lang.org/stable/std/fs/fn.remove_dir_all.html instead

WHAT??? Do you mean that the rm command used by Rust+Windows is the one being downloaded with getmake.py?????

Hell, there is rm.exe in msys/bin/... so now what?? and how is possible that this didn't happen before? So this is because, indeed, msys was NOT being stored under mediasoup_out_dir, right??

ibc · 2023-10-20T12:27:09Z

I give up.

Looks like you're trying to remove rm, same rm.exe you are using to remove itself 🙃
Try https://doc.rust-lang.org/stable/std/fs/fn.remove_dir_all.html instead

WHAT??? Do you mean that the rm command used by Rust+Windows is the one being downloaded with getmake.py?????

Hell, there is rm.exe in msys/bin/... so now what?? and how is possible that this didn't happen before? So this is because, indeed, msys was NOT being stored under mediasoup_out_dir, right?? So we had a bug that prevented another bug!

ibc · 2023-10-20T12:28:47Z

WHAT??? Do you mean that the rm command used by Rust+Windows is the one being downloaded with getmake.py?????

I mean it doesn't download make, it downloads the whole thing and once it is in PATH... Is there an API to extract a single file instead?

Yes, I get it. So I have no idea about a proper solution for this other than placing msys somewhere else and leaving it there (do not delete it at the end).

nazar-pc · 2023-10-20T12:29:53Z

Maybe using C:\Windows\system32\rm.exe or wherever the original is on Windows.

ibc · 2023-10-20T12:36:16Z

Maybe using C:\Windows\system32\rm.exe or wherever the original is on Windows.

So I have to tell Makefile to use C:\Windows\system32\rm.exe or whatever only in Windows. And it could be that the user doesn't have C: but D: or whatever...

nazar-pc · 2023-10-20T12:40:00Z

I have a better option: call make by its path on Windows instead of modifying PATH, that would definitely solve it.

ibc · 2023-10-20T12:51:46Z

I have a better option: call make by its path on Windows instead of modifying PATH, that would definitely solve it.

yes

ibc · 2023-10-20T13:49:51Z

@nazar-pc CI is passing!!!!

jmillan · 2023-10-20T14:28:44Z

npm-scripts.mjs

-	executeCmd(`${pythonPath} worker\\scripts\\getmake.py`);
+	const dir = path.resolve('worker/out/msys');
+
+	executeCmd(`${pythonPath} worker\\scripts\\getmake.py --dir="${dir}"`);


cosmetic, this path could aslo be created with path.resolve() and avoid the double back slashes.

Yes, but we don't follow an specific approach for that in the whole file. I agree we need consistency but I don't want to decide such a consistency within this PR.

worker/build.rs

Co-authored-by: José Luis Millán <[email protected]>

Fix NPM 'postinstall' task in Windows

9f609da

Fixes #1179 Provide `getmake.py` with the absolute path in which it must place fetched stuff.

ibc requested a review from nazar-pc October 20, 2023 08:27

ibc mentioned this pull request Oct 20, 2023

npm postinstall fails in Windows: rd /s /q worker\out\msys => The system cannot find the file specified #1179

Closed

Rust

87f683c

ibc added 3 commits October 20, 2023 11:43

Rust: use Path to join paths

8381be7

fix Rust

1db2dc8

fix cargo clippy in macos/Linux

d7e7821

nazar-pc requested changes Oct 20, 2023

View reviewed changes

ibc added 4 commits October 20, 2023 12:37

Address fb 1

d6037cb

address feedback 2

c856bb3

Use mediasoup_out_dir to store getmake.py and add it to PATH

cca0dab

"I love Rust"

457a2bc

Try keeping original PATH in make clean-all

603c907

jmillan reviewed Oct 20, 2023

View reviewed changes

ibc and others added 3 commits October 20, 2023 16:32

Improve inline doc

6848851

Update worker/build.rs

f16b33e

Co-authored-by: José Luis Millán <[email protected]>

revert single quotes

0a85fc0

jmillan approved these changes Oct 20, 2023

View reviewed changes

ibc merged commit a3b13cc into v3 Oct 20, 2023

ibc deleted the fix-npm-postinstall-in-windows branch October 20, 2023 15:19

Fix NPM 'postinstall' task in Windows #1187

Fix NPM 'postinstall' task in Windows #1187

Conversation

ibc commented Oct 20, 2023

ibc commented Oct 20, 2023

ibc commented Oct 20, 2023

ibc commented Oct 20, 2023

ibc commented Oct 20, 2023 • edited Loading

nazar-pc commented Oct 20, 2023

ibc commented Oct 20, 2023

ibc commented Oct 20, 2023

nazar-pc commented Oct 20, 2023

jmillan commented Oct 20, 2023

ibc commented Oct 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibc Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibc commented Oct 20, 2023

nazar-pc commented Oct 20, 2023

ibc commented Oct 20, 2023

ibc commented Oct 20, 2023

nazar-pc commented Oct 20, 2023

ibc commented Oct 20, 2023

ibc commented Oct 20, 2023

ibc commented Oct 20, 2023

nazar-pc commented Oct 20, 2023

ibc commented Oct 20, 2023

nazar-pc commented Oct 20, 2023

ibc commented Oct 20, 2023

ibc commented Oct 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibc commented Oct 20, 2023 •

edited

Loading

ibc Oct 20, 2023 •

edited

Loading