Better Win support: correct resolving of node/npm/npx & prisma #3258
Conversation
Martinsos
temporarily deployed
to
fly-deploy-test
GitHub Actions
Inactive
| prismaExecutableAbs = | ||
| waspProjectDir | ||
| </> nodeModulesDirInWaspProjectDir | ||
| </> SP.castRel (getPathToExecutableInNodeModules "prisma") |
There was a problem hiding this comment.
We are now being system-agnostic here, when figuring out abs path to prisma binary.
Martinsos
had a problem deploying
to
fly-deploy-test
GitHub Actions
Error
Martinsos
temporarily deployed
to
fly-deploy-test
GitHub Actions
Inactive
waspc/src/Wasp/Node/Executables.hs
Outdated
| {-# NOINLINE nodeExec #-} | ||
| nodeExec :: ExecName | ||
| nodeExec = | ||
| -- NOTE: We are taking whole resolved absolute path here because just using the resolved exec name |
There was a problem hiding this comment.
This I am not super happy with.
So basically just resolving npm to npm.cmd was not enough, we were still getting errors on Windows. Not the same ones, we moved forward, but different kind, I wish I wrote them down, something about not being able to find some .js files in node_modules.
But maybe it is for the best to have absolute paths to executables we use? Not 100% sure, I think if npm.cmd was working I woudl have stuck with it.
Interested in discussion here. Maybe I should try I again and write down the errors, that might help us make a decision.
There was a problem hiding this comment.
It is weird that just npm.cmd is not enough. But I vote for using absolute path if it works for node and npx. AFAIK, it works with my Windows 11 without any issues. I don't see any drawbacks of using the full path, except, for the logging, it will be longer than just the executable name.
There was a problem hiding this comment.
Yeah I agree. I still don't like the idea that I don't know what is causing this. Plus shorter names feel less fragile in some way, but that is not quite a weak argument in this case, as it could be made the opposite way easily also.
Will put at least one more attempt into this, to gather more info before making a final decision!
There was a problem hiding this comment.
Ok, so just to replicate the error I was mentioning, I modified this code to use fst instead of snd, therefore the command name (e.g. npm.cmd) that it successfully resolved, and then I get
So it fails when calling the command with npm.cmd instead of the full path.
This confuses me because of this:
-- The resolved path corresponds to the program that would be executed by
-- 'System.Process.createProcess' if exec name was provided as 'System.Process.RawCommand'. Check
-- 'System.Process.findExecutable' for more details since that is what we use internally.This is what System.Process.findExecutable promises, and we are using it. But that doesn't sem to be correct (or we are somehow using it somewhere in some way that is not createProcess, although what could that be).
There was a problem hiding this comment.
Some related issues I found online, but I am not sure if those inform us much beyond that it is indeed problem with exec path resolution:
There was a problem hiding this comment.
GPT tells me that what happens is that Windows resolution picks up the local npm shim in local node_modules but that then fails. Instead of picking up the global one.
But why does findExecutable then pick up the global npm? Shouldn't they be using the same mechanism and therefore hitting the same problem?
Here is from findExecutable docs:
Ok, so findExecutable uses this SearchPath and it depends on it what it will find. And it sounds like that one also will look into relative dir, based on these docs, but obviously not in our case, or somehow skips .node_modules? Yeah I mean .node_modules shoudln't even be hit by that, it is a neighbouring dir in a way. I am actually wondering why does the npm.cmd with proc even hit the local node_modules.
Some ideas for further testing:
- Try calling
SearchPathfrom powershell withnpm.cmdand see what it gives us. Do this from inside thewaspcdir. Confirm it is absolute path to the global npm installation. Or callfindExecutablefromghci, should be the same. - Try calling
procwithnpm.cmdfromwaspcdir, viaghci. Try confirming somehow it resolves to the localnpminstallation in local.node_modules, and if so, figure out why.
There was a problem hiding this comment.
I think I knew why. Node has both npm.cmd and npm.ps1 to support all shells that users might use.
Both scripts set the variable NPM_CLI_JS to the absolute path of npm-cli.js.
npm.cmd
SET "NPM_CLI_JS=%~dp0\node_modules\npm\bin\npm-cli.js"
npm.ps1
$NPM_CLI_JS="$PSScriptRoot/node_modules/npm/bin/npm-cli.js"
The behavior of $PSScriptRoot and %~dp0 varies on the current working directory (cwd) and the document of System process suggests that
If cwd is provided, it is implementation-dependent whether relative paths are resolved with respect to cwd or the current working directory, so absolute paths should be used to ensure portability.
That's why we need the absolute path of npm.cmd because we set cwd to the waspc folder.
There was a problem hiding this comment.
@nodtem66 thanks so much for this! Ok yeah that makes sense, so it is at the end cwd that is problematic -> it was my initial hypothesis in the issue, but I was missing understanding why is it problematic.
Ok, so absolute paths are then great because we don't care about what cwd is set to in haskell's calls to processes.
Or, we could make sure that cwd is kept undefined in those calls. But that sounds like a recipe for future problems, better to stick to absolute paths and we are good.
There was a problem hiding this comment.
Without setting cwd, the variable %~dp0 will be the current directory that the command wasp runs.
So, using absolute path is the proper way to do. We will have more flexible option to set or unset cwd. Plus, we could use the absolute paths of all executables for logging, so that users can monitor the fake npm.cmd or the invalid path of npm.cmd.
There was a problem hiding this comment.
Ok makes sense! I polished the code a bit, removed duplication of comments by moving that knowledge to the resolveExecNameIO, and also made it return only absolute path, not execName + absolutepath, as we don't want to make the mistake of using the first one.
waspc/src/Wasp/Node/Executables.hs
Outdated
| nodeExec = | ||
| -- NOTE: We are taking whole resolved absolute path here because just using the resolved exec name | ||
| -- was still flaky on Windows in some situations. | ||
| fromAbsFile $ snd $ unsafePerformIO $ resolveExecNameIO "node" |
There was a problem hiding this comment.
Instead of doing unsaferPerformIO, we could have actually obtained these in real IO, somweher at the start of our app, and then propagated them through the code, likely via our monad stack.
While that would be the "best" way to do it, I didn't think it makes much sense to bother with such big refactoring at the moment, for no real gain. We can do it if it turns out it is needed.
Martinsos
force-pushed
the
martin-win-cmd-resolution
branch
2 times, most recently
from
3a64514 to
12a7ab3
Compare
Martinsos
temporarily deployed
to
fly-deploy-test
GitHub Actions
Inactive
Martinsos
temporarily deployed
to
fly-deploy-test
GitHub Actions
Inactive
Martinsos
had a problem deploying
to
fly-deploy-test
GitHub Actions
Error
Martinsos
force-pushed
the
martin-win-cmd-resolution
branch
from
d1d320f to
02b0e63
Compare
Martinsos
temporarily deployed
to
fly-deploy-test
GitHub Actions
Inactive
Martinsos
force-pushed
the
martin-win-cmd-resolution
branch
from
02b0e63 to
ede2738
Compare
Martinsos
had a problem deploying
to
fly-deploy-test
GitHub Actions
Error
Martinsos
temporarily deployed
to
fly-deploy-test
GitHub Actions
Inactive
Martinsos
changed the title
Martinsos
force-pushed
the
martin-win-cmd-resolution
branch
from
6af92dc to
6085b35
Compare
Martinsos
had a problem deploying
to
fly-deploy-test
GitHub Actions
Error
Co-authored-by: Buniamin Shabanov <[email protected]> Co-authored-by: Jirawat Iamsamang <[email protected]>
Martinsos
force-pushed
the
martin-win-cmd-resolution
branch
from
6085b35 to
4e1e17c
Compare
Martinsos
temporarily deployed
to
fly-deploy-test
GitHub Actions
Inactive
|
@nodtem66 in case you want to take a final look, feel free to, although I didn't change it much: however I finally made it ready for review (out of draft)! I did separate out the terminal unstyling into another PR to keep this one clean. I think this is now in good shape. Thanks again for all the help, and I added you as a co-author. Btw apologies for my intermittent activity on the PRs: these days I am mostly taken away by tasks that are not programming but stuff like management, research and similar, that benefit the team the most, but I do make sure to dedicate at least one day per sprint on development, and that is when I usually grab the time for these PRs. |
nodtem66
left a comment
nodtem66
left a comment
There was a problem hiding this comment.
Nice, all of these look great. Thank you for the co-authorship. I'm happy to help out and pick up new things along the way.
Don't worry about that. I get that someone needed to do that job. I usually use my free time for this, and I really enjoy it. The best part was definitely what I earned from the discussion. I hope it is useful for the project more or less.
It is super useful! I probably wouldn't have the energy to drive through some of these obstacles on my own with the limited time I had available, and I really appreciate the quality of your responses and research/work you put behind each one. Hope to see you around more. |
Deploying wasp-docs-on-main with
|
| Latest commit: |
b2f290c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://54eb47cc.wasp-docs-on-main.pages.dev |
| Branch Preview URL: | https://martin-win-cmd-resolution.wasp-docs-on-main.pages.dev |
Martinsos
temporarily deployed
to
fly-deploy-test
GitHub Actions
Inactive
Description
I added no tests here because to unit test changes here is too hard because of IO and global vars, and while we could do it if we really wanted to, I don't think it is worth the effort, taking into account the content of the changes. This will all be well covered on the e2e tests and is not that logic heavy.
I did create a separate issue to get wasp cli e2e tests working for Windows (#3404), that will be important.
I based this PR on previous work by @MetaMmodern and @nodtem66 + quite some help from @nodtem66 , so big thanks to both, and I added you both as co-authors, as is more than deserved.
Type of change
Checklist
I tested my change in a Wasp app to verify that it works as intended.
🧪 Tests and apps:
- [ ] I added unit tests for my change.- [ ] (if you fixed a bug) I added a regression test for the bug I fixed.- [ ] (if you added/updated a feature) I added/updated e2e tests inexamples/kitchen-sink/e2e-tests.- [ ] (if you added/updated a feature) I updated the starter templates inwaspc/data/Cli/templates, as needed.- [ ] (if you added/updated a feature) I updated the example apps inexamples/, as needed.- [ ] (if you updatedexamples/tutorials) I updated the tutorial in the docs (and vice versa).📜 Documentation:
- [ ] (if you added/updated a feature) I added/updated the documentation inweb/docs/.🆕 Changelog: (if change is more than just code/docs improvement)
waspc/ChangeLog.mdwith a user-friendly description of the change.- [ ] (if you did a breaking change) I added a step to the current migration guide inweb/docs/migration-guides/.- [ ] I bumped theversioninwaspc/waspc.cabalto reflect the changes I introduced.