Skip to content

Rescript v10 building in watch mode fails with permission error #5617

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

Closed
hellos3b opened this issue Aug 26, 2022 · 21 comments · Fixed by #5621
Closed

Rescript v10 building in watch mode fails with permission error #5617

hellos3b opened this issue Aug 26, 2022 · 21 comments · Fixed by #5621

Comments

@hellos3b
Copy link
Contributor

hellos3b commented Aug 26, 2022

I've upgraded to rescript@10 and watch mode now fails with Error: EPERM: operation not permitted, open 'C:\Users\sebhe\Code\issue-rescript-permissions\.bsb.lock'

Full message:

PS C:\Users\sebhe\Code\issue-rescript-permissions> npm run dev

> [email protected] dev
> rescript build -w

Error: EPERM: operation not permitted, open 'C:\Users\sebhe\Code\issue-rescript-permissions\.bsb.lock'
    at Object.openSync (node:fs:585:3)
    at acquireBuild (C:\Users\sebhe\Code\issue-rescript-permissions\node_modules\rescript\rescript:207:10)
    at build (C:\Users\sebhe\Code\issue-rescript-permissions\node_modules\rescript\rescript:516:11)
    at in_watch_mode (C:\Users\sebhe\Code\issue-rescript-permissions\node_modules\rescript\rescript:587:5)
    at ChildProcess.<anonymous> (C:\Users\sebhe\Code\issue-rescript-permissions\node_modules\rescript\rescript:317:9)
    at ChildProcess.emit (node:events:390:28)
    at maybeClose (node:internal/child_process:1064:16)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:301:5) {
  errno: -4048,
  syscall: 'open',
  code: 'EPERM',
  path: 'C:\\Users\\sebhe\\Code\\issue-rescript-permissions\\.bsb.lock'
}

I am on Windows 11 and have had no issue with previous versions.

Steps I've taken to reproduce:

mkdir ./issue
cd ./issue
npm init -y
npm install rescript@10 -d
# > Copy bsconfig.json from documentation
# > Create src/HelloWorld.res
npm run dev # rescript build -w

System Info:

OS Name:                   Microsoft Windows 11 Home
OS Version:                10.0.22000 N/A Build 22000
OS Manufacturer:           Microsoft Corporation
OS Configuration:          Standalone Workstation
OS Build Type:             Multiprocessor Free
System Manufacturer:       Microsoft Corporation
System Model:              Surface Laptop Studio
System Type:               x64-based PC
Processor(s):              1 Processor(s) Installed.
                           [01]: Intel64 Family 6 Model 140 Stepping 1 GenuineIntel ~3110 Mhz
BIOS Version:              Microsoft Corporation 18.101.141, 12/15/2021

I created an empty project and tried it in there and got the same error message. I've pushed it to github and included the lib/bs file in case anything in there is helpful

https://github.com/hellos3b/issue-rescript-permissions

Running npm install rescript to go back to v9 resolves the issue

@hellos3b hellos3b changed the title Rescript@10 build watch fails Rescript v10 building in watch mode fails with permission error Aug 26, 2022
@hellos3b
Copy link
Contributor Author

hellos3b commented Aug 27, 2022

So I've been looking around to see if I can find anything. I noticed that the lock file now has permissions added

// We use [~perm:0o664] rather than our usual default perms, [0o666], because
// lock files shouldn't rely on the umask to disallow tampering by other.
(...)
      fs.openSync(lockFileName, "wx", 0o664);

(Source)

I created a small js file to try to mimick it to see if windows doesn't like that:

const fs = require("fs");
const path = require("path");
const lockFileName = path.resolve(process.cwd(), ".bsb.lock");

fs.openSync(lockFileName, "wx", 0o664);

But that had no problem running and creating the file, so not too sure where to look next as I'm not too familiar with filesystem permissions

@cristianoc
Copy link
Collaborator

Would you post on the forum, see if some other Windows user has ideas.

@srikanthkyatham
Copy link

I am facing the same issue on windows with the rescript@10

@sprkv5
Copy link

sprkv5 commented Aug 29, 2022

I am on Windows 10 and on upgrade to rescript@10 I observed the same issue with rescript build -w
but not with either rescript build or rescript build -with-deps. Seems like this problem is limited to the watch mode.

@zth
Copy link
Collaborator

zth commented Aug 29, 2022

Would either of you who experience this problem be up for doing some debugging to fix this issue? I'm afraid we're short on contributors using Windows.

@hellos3b
Copy link
Contributor Author

I can help if there's something you'd like me to try

@cristianoc
Copy link
Collaborator

cristianoc commented Aug 29, 2022

1 check changes to the "rescript" script since then, see if something rings a bell
Or just copy over the old script see what happens
Also try to install the beta versions from npm to narrow it down

2 If that does not work, check recent changes to ninja (https://github.com/rescript-lang/ninja), see if something rings a bell
Or just copy over the old ninja executable, see what happens

@hellos3b
Copy link
Contributor Author

hellos3b commented Aug 30, 2022

@cristianoc Ok so every version of 10x down to alpha does not work.

I tried your idea of copying the contents of rescript over. Going back far enough gets it to work with 10.0.0. This is the commit where it breaks: TheSpyder@c32cc34

I added some logging inside the acquireBuild call and here's the order of operations and error:

acquireBuild: is_building? false
acquireBuild: fs.openSync C:\Users\sebhe\Code\issue-rescript-permissions\.bsb.lock
BSB check build spec : C:\Users\sebhe\Code\issue-rescript-permissions\node_modules\rescript\win32\rescript.exe 
Rebuilding since proj,started
acquireBuild: is_building? false
acquireBuild: fs.openSync C:\Users\sebhe\Code\issue-rescript-permissions\.bsb.lock
acquireBuild: catch error:
Error: EPERM: operation not permitted, open 'C:\Users\sebhe\Code\issue-rescript-permissions\.bsb.lock'

@cristianoc
Copy link
Collaborator

@hellos3b thank you for digging. So there's an easy "fix", and ideally we'd like a proper fix.

@TheSpyder what would a proper fix look like?

@cristianoc
Copy link
Collaborator

(and after that, we should add a test to catch future regressions)

@cristianoc
Copy link
Collaborator

@cknitt: opam exec -- node scripts/ciTest.js -mocha -theme -format wondering if there's a way to run a subset of build tests on Windows in CI, rather than none as some simply don't work at the moment.

@cknitt
Copy link
Member

cknitt commented Aug 30, 2022

I can look into that when I have some time.

We are already running a (very simple) build test with the finished npm package at the very end of the CI process though:

https://github.com/rescript-lang/rescript-compiler/blob/6de8e49f02185b5abd54803cfa781df20d3a85bf/.github/workflows/ci.yml#L184

@TheSpyder
Copy link
Contributor

If TheSpyder@c32cc34 is the problem it suggests aquireBuild has never worked since the transition from BuckleScript - all it does is call a function that the code clearly intended should be called.

Perhaps it's because the function is already called elsewhere in the file:
https://github.com/rescript-lang/rescript-compiler/blob/6de8e49f02185b5abd54803cfa781df20d3a85bf/rescript#L517

And previously the code didn't attempt to acquire the lock twice.

My suggested fix is to make the function use fs.existsSync() rather than catching the openSync error, it looks like the style of error thrown is different across platforms.

@Mng12345
Copy link

Mng12345 commented Aug 30, 2022

This is because the api fs.unlinkSync can not delete the file if it is not closed on Windows. Closing the lock file after opening it will fix this build error. #5620 @TheSpyder

@TheSpyder
Copy link
Contributor

TheSpyder commented Aug 30, 2022

@Mng12345 Thanks for the tip! Are you able to submit a PR?

@Mng12345
Copy link

@Mng12345 Thanks for the tip! Are you able to submit a PR?

Hi, i'm afraid not. This repository is too big for me to clone it locally due to the network is limitted in China...
This may helps you fixing it.
From:

function acquireBuild() {
  if (is_building) {
    return false;
  } else {
    try {
      fs.openSync(lockFileName, "wx", 0o664);
      is_building = true;
    } catch (err) {
      if (err.code === "EEXIST") {
        console.warn(lockFileName, "already exists, try later");
      } else console.log(err);
    }
    return is_building;
  }
}

To:

function acquireBuild() {
  if (is_building) {
    return false;
  } else {
    try {
      const fid = fs.openSync(lockFileName, "wx", 0o664);
      fs.closeSync(fid);
      is_building = true;
    } catch (err) {
      if (err.code === "EEXIST") {
        console.warn(lockFileName, "already exists, try later");
      } else console.log(err);
    }
    return is_building;
  }
}

@TheSpyder
Copy link
Contributor

Thank you! And that still throws the EEXIST error if two watch modes are attempted at the same time?

@Mng12345
Copy link

Mng12345 commented Aug 30, 2022

Emm, what does two watch modes mean?
I started two build processes, it's fine. Don't know if it is ok on Mac and Linux.

image

Two process of rescript build -w are worked too.
image

@cristianoc
Copy link
Collaborator

I can look into that when I have some time.

We are already running a (very simple) build test with the finished npm package at the very end of the CI process though:

https://github.com/rescript-lang/rescript-compiler/blob/6de8e49f02185b5abd54803cfa781df20d3a85bf/.github/workflows/ci.yml#L184

That might be an even better place to run build tests.
The only thing is what to do during local development.
Interesting to look into that when there is time, no rush.

How about creating an issue for now, just as a reminder to revisit that later.

@hellos3b
Copy link
Contributor Author

I tried it out on my local and can confirm that @Mng12345 's suggestion of closing sync after opening works on windows, and ran two process and spam save to see the error come up at least once

image

@TheSpyder
Copy link
Contributor

I thought watch mode threw 'lockfile exists' on startup, but I must be remembering back to the bsb days because the above is how it works on macOS today. Although when I went to double check, it turns out the old acquire function worked in a fairly similar way to these changes so we're on the right track 😂
https://github.com/rescript-lang/rescript-compiler/blob/c9e7ce28380f7d97a5b7b0b4532a1488a93f40db/bsb#L220-L226

What it used to do was write the pid of the active watch process to the lock file, meaning only one watch process could be active at a time, and also stale watch files could be removed automatically.

Might be nice to get closer to the old behaviour eventually - this approach of continuously retry until a build can be started certainly avoids errors, but doesn't seem correct. It should hold the lock the entire time watch mode is running.

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

Successfully merging a pull request may close this issue.

8 participants