Make sure process parameters are correctly quoted #10676
Conversation
nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Utilities/ProcessExtensions.cs
Outdated
Show resolved
Hide resolved
|
Good catch! |
|
@brettfo Thanks for the review. Do I need to do anything else to get this merged? |
You're good from your end, I'm talking to folks on my side to get this merged. Should be soon.
What was your process to see this error? I suspect it's related to #10521 that was merged yesterday that adds a PowwerShell dependency to the NuGet updater image, but only in certain circumstances. Building the dev shell with |
Cool, thanks!
Yea, I just ran |
|
@ffried I wonder if something is happening during the download and install of DOCKER_BUILD_ARGS="--progress=plain" bin/docker-dev-shell nuget --rebuild |
|
@brettfo I just cleaned my build cache and ran the command you provided. It first builds the |
|
Hi @ffried , I think your fork seems out-of-date with dependabot core main branch, can you please trying rebasing with dependabot-core so that i can try to do a release. |
ffried
force-pushed
the
space-for-everyone
branch
from
4a15f49
to
1e890fb
Compare
|
@sachin-sandhu I just rebased both |
|
@ffried , i can see some issues while testing #10676 (comment) ,i have deferred deploy and merge for now, Please let me know once you are good and we can resume. |
|
@sachin-sandhu Not sure what I can do here. The failure seems completely unrelated to my changes. |
ffried
force-pushed
the
space-for-everyone
branch
from
1e890fb
to
12d77fa
Compare
|
@sachin-sandhu I'm also getting this error on |
|
HI @brettfo, Can you please provide inputs on #10676 (comment) linking #9678 (comment) in case it is related. cc @ffried |
|
@sachin-sandhu I think both of those things are unrelated to these changes. In the first case (unable to build Dockerfile): I can build it locally and the CI is passing which means it can be built on more than my machine. There must be some weird caching issue with the other user's Docker instance, but I can't fathom what it could possibly be. In the second case, that looks like we're trying to update a file that we didn't report earlier, but from reading the code that should be impossible: we only check things previously reported as a In short, I don't see any blockers to this getting merged and deployed. |
|
@brettfo @sachin-sandhu One thing that just came to my mind is that I'm working on an Apple Silicon MacBook Pro. So it's ARM architecture. Maybe that's the issue with the Docker build. |
Good call, that's exactly it. I checked the feed and there is no Edit: it appears there is a |
One option could be to use the predefined build arguments to determine the arch: https://docs.docker.com/build/building/variables/#multi-platform-build-arguments |
|
@ffried We're getting ready to merge this, could you please update your fork/branch to the latest in |
…cessExtensions.cs Co-authored-by: Keith Dahlby <dahlbyk@gmail.com>
ffried
force-pushed
the
space-for-everyone
branch
from
12d77fa
to
a2f888c
Compare
|
@brettfo Done |
What are you trying to accomplish?
With #9678, the nuget updated gained support for package lock files.
However, it fails when the csproj path contains spaces (see #9678 (comment)).
This now changes the
ProcessExhelper to accept a list of args (instead of just a string). This should fix this issue for the lock file update as well as several other places.Anything you want to highlight for special attention from reviewers?
My initial thought was to simply escape the strings myself, but
ProcessStartInfohas a constructor that also takes an argument list, so I opted to use that one and leave the escaping to the system.Also, I didn't just want to fix the lock file updater, because I found other places where paths (which can possibly contains spaces) weren't correctly escaped. With that API change, it should no longer be an issue, also for new usages of the
ProcessExhelper.How will you know you've accomplished your goal?
I've updated all the call sites of the old extension by passing the args as array. One was even creating an array and joining it with a string.
Checklist
Unfortunately, I was unable to run the tests. Starting the development shell fails with the following error: