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

[BUG] Current folder with spaces does not watch #308

Closed
asinino opened this issue Dec 21, 2021 · 20 comments · Fixed by #379
Closed

[BUG] Current folder with spaces does not watch #308

asinino opened this issue Dec 21, 2021 · 20 comments · Fixed by #379
Labels
bug Something isn't working outdated scope: core

Comments

@asinino
Copy link
Contributor

asinino commented Dec 21, 2021

Describe the bug
On my local computer, the project I'm working on includes spaces on the folder, trying to run 'nx serve api' bugs out with the message:

watch : The project file 'C:\Users\user\Documents\Projects\Team Workload\apps\api\C:\Users\user\Documents\Projects\Team' does not exist..

I tried to solve the problem locally and the solution is easy, on DotNetClient.run just convert the cmd variable to an array of parameters:

let cmd = watch ? [`watch`, `--project`, project, `run`] : [`run`, `--project`, project];

Do the proper changes to accumulate the function parameter 'parameters'. And to print the message on the console just do a .join(' ')

To Reproduce
Steps to reproduce the behavior:

  1. run 'nx serve api'

Expected behavior
The project API should be served as described in the documentation.

Environment:

  • OS: used Windows but probably any OS
  • Version: 1.8.0
  • DotnetClient
@asinino asinino added bug Something isn't working needs-triage This issue has yet to be looked over by a core team member labels Dec 21, 2021
@asinino
Copy link
Contributor Author

asinino commented Dec 21, 2021

How can I push the fix to the repository?

@AgentEnder
Copy link
Member

Hey @asinino, sorry I've been away for paternity leave at work, and trying to stay away to spend time with family. If you have WSL setup on windows, I can verify the repo works well under it. Further tooling changes would have to wait for me to be back, unless you feel like working on those and then I can review them.

@AgentEnder AgentEnder added scope: core and removed needs-triage This issue has yet to be looked over by a core team member labels Jan 16, 2022
asinino added a commit to asinino/nx-dotnet that referenced this issue Feb 28, 2022
When a user has a project that contains a space in it's name the command
failed because it does not escaped the space to the shell command.
Fixes nx-dotnet#308
@AgentEnder
Copy link
Member

@allcontributors add asinino for code and bugs

@allcontributors
Copy link
Contributor

@AgentEnder

I've put up a pull request to add @asinino! 🎉

@asinino
Copy link
Contributor Author

asinino commented Feb 28, 2022

Hello
Took some time to have spare time to do this.

I added a PR that encloses the project path + name with double quotes (present ao all commands referencing a project).
I did not add a test for this situation, a name with spaces and or a parent folder name with spaces, is the test a requirement? If needed how can I develop this solution?

@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 1.9.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@AgentEnder
Copy link
Member

The test would be nice, but this is fine as-is IMHO. The dotnet package itself is pretty light on tests, if one were to be added I'd recommend just adding an e2e test in the core-e2e project since there are not currently e2e tests set up for the dotnet project.

Eventually, we should probably set up e2e for it, shouldn't be hard but isn't currently done.

@asinino
Copy link
Contributor Author

asinino commented Mar 2, 2022

TLDR: watch commands fail to start. CWD is injected on spawn command also so double directory path is inserted on the command.

I received this update today, and it's in a big mess, I just broke your package with my last update.
I'm trying to fix it up right now and will publish a solution soon.

TLTR: watch commands fail to start. CWD is injected on spawn command also so double directory path is inserted on the command.

My proposed solution is:

  • A: remove CWD from spawn because the project variable already has a full path.
  • B: remove the CWD from the project variable

The first target is going with solution B because CWD is passed down to the command.

@AgentEnder
Copy link
Member

Hmmmm, that doesn't seem like something that should have been changed by your previous PR. We've always passed fully resolved paths to the dotnet commands, and the updates you added shouldn't have changed anything with that. Let me look into it real quick.

@AgentEnder
Copy link
Member

I think the issue is just with how spawn handles parameters. Before we were splitting on spaces which worked fine, but now with the quotes we need to make sure we don't split them. It may be best to move the command out some. Let me do some testing.

@asinino
Copy link
Contributor Author

asinino commented Mar 2, 2022

Ok, I'll convert the cmd into an array of arguments, it should fix the issue.

I'm having some trouble testing it offline, how can I run it locally? I changed the way Verdaccio is called and now it's correctly initialized. But the command to publish-local is falling (I'll open a new issue for this and try to properly supply a fix).

I don't control everything on my current machine and the environment does not allow me to have WSL :/

@asinino
Copy link
Contributor Author

asinino commented Mar 2, 2022

Also, change this ticket back to open because the currently applied fix is breaking the package.

@AgentEnder AgentEnder reopened this Mar 2, 2022
@asinino
Copy link
Contributor Author

asinino commented Mar 2, 2022

Have submitted a new fix, but please review the changes because a lot has changed.

All parameters are now handled as an array, so we may not need special care for paths with spaces.

But still, if someone can test this as I can't have a local version of the change, script publish-local is not working on my environment.

@ntziolis
Copy link

ntziolis commented Mar 3, 2022

Running into a very similar issue WITHOUT any spaces in the path:

  • Installed this lib via npm install --save-dev @nx-dotnet/core
  • Created a new web api project via NX CONSOLE UI
  • Executed serve for project via nx console (command used is npx nx serve my-project-name

Output

Executing Command: dotnet watch --project "/home/username/code/reponame/apps/my-project-name/My.Project.Name.csproj" run --configuration Debug 
watch : The project file '/home/username/code/reponame/apps/my-project-name/"/home/username/code/reponame/apps/my-project-name/My.Project.Name.csproj"' does not exist.

When executing the command listed above in bash directly the web api starts up right away no issues, for refrence this is the command that works:

dotnet watch --project "/home/username/code/reponame/apps/my-project-name/My.Project.Name.csproj" run --configuration Debug 

Given that the issue is path based this seems rather central and I would guess that right now running dotnet apps via the serve command is broken in general in the latest version.

Any timeline on a fix?

@asinino
Copy link
Contributor Author

asinino commented Mar 3, 2022

I'm currently working on the solution for the problem, I'll give it a day.

In the current state, the serve command is useless.

@AgentEnder
Copy link
Member

CC @asinino I pushed some updates to your branch and went ahead and merged it. Going to cut a release soon to fix since its pretty urgent bug. Everything looks fine testing locally, and E2E passed.

@ntziolis
Copy link

ntziolis commented Mar 3, 2022

I can confirm this fixed the bug for us. Hella quick turnaround guys, keep it up !!!

@asinino
Copy link
Contributor Author

asinino commented Mar 3, 2022

I couldn't test it locally, it always failed, with my version of dotnet-format it still fails because the parameter "--check" does not exist.

Still, I pushed a fix to execute return the buffer from stdout. Can you see the change? Do I need another pull request?

I added some notes on previous pull request #392 to the questions you have made, like why the use of spawnSync.

@AgentEnder
Copy link
Member

@asinino Open a new issue for the dotnet-format bug, I verified it worked with dotnet 5.0.203 and 6.0.200 locally.

@github-actions
Copy link
Contributor

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working outdated scope: core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants