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

fix pacquet executer #41

Merged
merged 2 commits into from Jul 31, 2023
Merged

fix pacquet executer #41

merged 2 commits into from Jul 31, 2023

Conversation

Yakiyo
Copy link
Contributor

@Yakiyo Yakiyo commented Jul 31, 2023

Until now pacquet just ran any script/shell commands by just directly passing the entire command to Command::new. As per the std, Command::new takes only the file/program name, and any additional args need to be provided via the arg or args method.

Well now, i just touched it a little bit, to run sh -c and pass the actual command/script to it. This runs without any errors now, and sometimes scripts contain env vars too. For example

{
  "scripts": {
    "test": "KEY=VALUE node index.js"
  }
}

Now it is passed as sh -c "KEY=ENV node index.js. This seems to be how the peeps do it in pnpm. (in pn)

And also pass any sort of additional args provided in the run command to the script. For example,

{
  "scripts": {
    "test": "echo Hello"
  }
}

Current running pacquet test and pacquet test World both just prints "Hello", but pnpm passes any additional args provided to the script too. So now it prints "Hello World" when done pacquet test World

@Yakiyo
Copy link
Contributor Author

Yakiyo commented Jul 31, 2023

I tested it on my own laptop. Command indeed does fail to run before this fix was done.

image
The error message was also pretty obscure, and it took me a while just to find where it was stemming from. Maybe pacquet_executor::io_error's message could be made better? maybe an additional point stating, running external command failed or something.

@anonrig
Copy link
Member

anonrig commented Jul 31, 2023

You're absolutely right. Amazing finding! Thank you.

@codecov-commenter
Copy link

Codecov Report

Merging #41 (50b467c) into main (72ec745) will decrease coverage by 0.48%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
- Coverage   82.22%   81.74%   -0.48%     
==========================================
  Files          21       21              
  Lines        1035     1041       +6     
==========================================
  Hits          851      851              
- Misses        184      190       +6     
Files Changed Coverage Δ
crates/cli/src/commands.rs 24.00% <0.00%> (-1.00%) ⬇️
crates/cli/src/lib.rs 54.54% <0.00%> (-3.79%) ⬇️
crates/executor/src/lib.rs 0.00% <0.00%> (ø)

@anonrig anonrig merged commit 9349c78 into pnpm:main Jul 31, 2023
14 of 16 checks passed
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 this pull request may close these issues.

None yet

3 participants