-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: WCOW: add powershell.exe dir to default PATH for backward compatibiliy #5446
base: master
Are you sure you want to change the base?
Conversation
Curious; would this case work if the i.e., are we tapering over the actual bug (that no |
527baa3
to
092999c
Compare
@thaJeztah -- I was suggesting here #5445 that we are at a cross-road to decide to either go for registry-backed or config-backed environment variables. Going for both ends up in confusion, especially in cases where there are name collusions (and config takes precedence). For instance if you attempt to do this with registry-backed PATH env like it is in classic docker-build: FROM <any official base image>
ENV PATH "$PATH;C:\\my\\added\\path"
RUN echo PATH=%PATH% You just end up with We can carry on the conversations on #5445 -- will appreciate your input. |
092999c
to
aa093d3
Compare
aa093d3
to
9d5b592
Compare
The current default `PATH` has been set to the most basic subset for both `nanoserver` and `servercore`. ```go const DefaultPathEnvWindows = "c:\\Windows\\System32;c:\\Windows; ``` The path to `powershell.exe` is conspicuously missing hence breaking the developer experience for most workloads that depend on PS. This fix proposes to append `;C:\\Windows\\System32\\WindowsPowerShell\\v1.0` to that list to support PS scenarios, that make a big chunk of workloads, especially legacy ones, migrating from on-prem to cloud. Fixes moby#4901, microsoft/Windows-Containers#500 Ref moby#3158 Implication for nanoserver: =========================== This does not any way break the experiences for those using `nanoserver` base image, as much as PS and the `C:\\Windows\\System32\\WindowsPowerShell\\v1.0` path is not present on `nanoserver`. Transition users to using `ENV`: ============================== Users coming from classic `docker build` will need to transition from using `RUN setx /M` to `ENV` as a way of persisting environment variables in the images. We can take a hit on not supporting backward compatibility for `RUN setx /M`, as opposed to not supporting both `RUN setx /M` and `RUN powershell` as it is the case right now. Alternative considered: ======================= We thought of an option to ask the Windows platform team to add a default `Config.Env.PATH` in the config file for the base image. However, this causes a regression for `RUN setx /M` on the classic `docker build`. There could be a couple of more people too that might be depending on `Config.Env = null` as it is currently. See `docker inspect mcr.microsoft.com/windows/servercore:ltsc2022`. Here is the regression details when we set `ENV` in the base image. ``` PS> type .\Dockerfile FROM mcr.microsoft.com/windows/servercore:ltsc2022 AS newbase ENV PATH="C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Windows\System32\OpenSSH\;C:\Users\ContainerAdministrator\AppData\Local\Microsoft\WindowsApps" FROM newbase RUN setx /M PATH "C:\my\path;%PATH%" RUN echo %PATH% PS> docker build --no-cache -t profnandaa/servercore-path-tests . Sending build context to Docker daemon 2.048kB Step 1/5 : FROM mcr.microsoft.com/windows/servercore:ltsc2022 AS newbase ---> e64ba0f4256b Step 2/5 : ENV PATH="C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Windows\System32\OpenSSH\;C:\Users\ContainerAdministrator\AppData\Local\Microsoft\WindowsApps" ---> Running in ab3dc4921d7d ---> Removed intermediate container ab3dc4921d7d ---> f57b0a2d0e28 Step 3/5 : FROM newbase ---> f57b0a2d0e28 Step 4/5 : RUN setx /M PATH "%PATH%;C:\my\path;" ---> Running in 6ca6171334a0 SUCCESS: Specified value was saved. ---> Removed intermediate container 6ca6171334a0 ---> 6d2870e2f91d Step 5/5 : RUN echo %PATH% ---> Running in 785633e2a31c C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Windows\System32\OpenSSH\;C:\Users\ContainerAdministrator\AppData\Local\Microsoft\WindowsApps ^ | ~~~~~~~~~ notice "C:\my\path" is missing, meaning `setx /M` is not persisting as before. ---> Removed intermediate container 785633e2a31c ---> ed490181b903 Successfully built ed490181b903 Successfully tagged profnandaa/servercore-path-tests:latest ``` Current `RUN setx /M` behavior: ``` PS> type .\Dockerfile FROM mcr.microsoft.com/windows/servercore:ltsc2022 RUN setx /M PATH "%PATH%;C:\my\path;" RUN echo %PATH% PS> docker build --no-cache -t profnandaa/servercore-path-tests . Sending build context to Docker daemon 2.048kB Step 1/3 : FROM mcr.microsoft.com/windows/servercore:ltsc2022 ---> e64ba0f4256b Step 2/3 : RUN setx /M PATH "C:\my\path;%PATH%" ---> Running in 5502dd679495 SUCCESS: Specified value was saved. ---> Removed intermediate container 5502dd679495 ---> 0b59f38e2da4 Step 3/3 : RUN echo %PATH% ---> Running in 3bacb0b27bad C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Windows\System32\OpenSSH\;C:\Users\ContainerAdministrator\AppData\Local\Microsoft\WindowsApps;C:\my\path; ^ |~~~~ `setx /M` persists. ---> Removed intermediate container 3bacb0b27bad ---> cda1b4cd27ff Successfully built cda1b4cd27ff Successfully tagged profnandaa/servercore-path-tests:latest ``` `setx` vs. `ENV` has been discussed in details in moby#5445 Signed-off-by: Anthony Nandaa <[email protected]>
9d5b592
to
af5e909
Compare
The current default
PATH
has been set to the most basic subset for bothnanoserver
andservercore
.The path to
powershell.exe
is conspicuously missing hence breaking the developer experience for most workloads that depend on PS.This fix proposes to append
;C:\\Windows\\System32\\WindowsPowerShell\\v1.0
to that list to support PS scenarios, that make a big chunk of workloads, especially legacy ones, migrating from on-prem to cloud.Fixes #4901, microsoft/Windows-Containers#500
Ref #3158
Basic Test
On classic docker build:
Buildkit before fix:
Buildkit after fix:
Implication for nanoserver:
This does not any way break the experiences for those using
nanoserver
base image, as much as PS and theC:\\Windows\\System32\\WindowsPowerShell\\v1.0
path is not present onnanoserver
.Transition users to using
ENV
:Users coming from classic
docker build
will need to transition from usingRUN setx /M
toENV
as a way of persisting environment variables in the images.We can take a hit on not supporting backward compatibility for
RUN setx /M
, as opposed to not supporting bothRUN setx /M
andRUN powershell
as it is the case right now.Alternative considered:
We thought of an option to ask the Windows platform team to add a default
Config.Env.PATH
in the config file for the base image. However, this causes a regression forRUN setx /M
on the classicdocker build
. There could be a couple of more people too that might be depending onConfig.Env = null
as it is currently. See:Here is the regression details when we set
ENV
in the base image.Current
RUN setx /M
behavior:setx
vs.ENV
has been discussed in details in #5445