Fix dotnet-local script to work. #8222
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonpryor
reviewed
Jul 31, 2023
dellis1972
commented
Jul 31, 2023
|
@pjcollins I added a call in the windows |
jonathanpeppers
approved these changes
Aug 2, 2023
pjcollins
approved these changes
Aug 2, 2023
jonpryor
reviewed
Aug 3, 2023
| @@ -20,21 +20,18 @@ MSBuild version 15 or later is required. | |||
|
|
|||
| 5. In a [Developer Command Prompt][developer-prompt], prepare the project: | |||
|
|
|||
| dotnet msbuild Xamarin.Android.sln -t:Prepare | |||
| dotnet msbuild Xamarin.Android.sln -t:Prepare -nr:false -m:1 | |||
There was a problem hiding this comment.
Why is -nr:false needed? Why is -m:1 needed?
There was a problem hiding this comment.
I'd also prefer using the "long form" instead of "short form" of parameters, e.g. -nodeReuse:false instead of -nr:false.
I'm still not sure why we need -nr:false, though. CI doesn't appear to be to use that, except within unit tests: https://github.com/xamarin/xamarin-android/blob/c22b2a8bbbfb906d5d9029147dda7cfaf68d947a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/Builder.cs#L224
There was a problem hiding this comment.
It stops you getting errors locally when rebuilding the tree. I'll see if I can get the error message I see.
There was a problem hiding this comment.
This is the message I get then not disabling nodeReuse.
error MSB4018: The "GenerateDepsFile" task failed unexpectedly. [/Users/dean/Documents/Sandbox/Xamarin/WI1714603/external/Java.Interop/tools/jcw-gen/jcw-gen.csproj]
/Users/dean/Documents/Sandbox/Xamarin/WI1714603/bin/Release/dotnet/sdk/8.0.100-rc.1.23373.1/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.targets(267,5): error MSB4018: System.IO.IOException: The process cannot access the file '/Users/dean/Documents/Sandbox/Xamarin/WI1714603/bin/Release/lib/packs/Microsoft.Android.Sdk.Darwin/34.0.0/tools/jcw-gen.deps.json' because it is being used by another process. [/Users/dean/Documents/Sandbox/Xamarin/WI1714603/external/Java.Interop/tools/jcw-gen/jcw-gen.csproj
The recent bump to dotnet broke the dotnet-local scripts. It now seems that the "local" path must be FIRST in the PATH in order to pick up the packs. Also added a smoke test step to make sure the script works, at least one Mac and Linux.
`export` all env vars. Remove code duplication and just use a loop.
dellis1972
force-pushed
the
fixdotnetlocal
branch
from
651d525
to
06e9be1
Compare
|
WIP commit message: The .NET 8 `dotnet` command appears to require that `dotnet` be inThe .NET 8 `dotnet` command appears to require that `dotnet` be in
`$PATH`, lest it invoke a *different* `dotnet`. This can result in
weird error messages:
% ~/Developer/src/xamarin/xamarin-android/dotnet-local.sh build *.csproj
…
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error : You must install or update .NET to run this application.
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error :
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error : App: …/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/bincore/csc.dll
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error : Architecture: x64
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error : Framework: 'Microsoft.NETCore.App', version '8.0.0-rc.1.23371.3' (x64)
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error : .NET location: /usr/local/share/dotnet/
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error :
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error : The following frameworks were found:
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error : 6.0.16 at [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error : 7.0.3 at [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error : 7.0.4 at [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error : 7.0.5 at [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error : 7.0.9 at [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error :
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error : Learn about framework resolution:
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error : https://aka.ms/dotnet/app-launch-failed
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error :
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error : To install missing framework, download:
…/dotnet/sdk/8.0.100-rc.1.23373.1/Roslyn/Microsoft.CSharp.Core.targets(80,5): error : https://aka.ms/dotnet-core-applaunch?framework=Microsoft.NETCore.App&framework_version=8.0.0-rc.1.23371.3&arch=x64&rid=osx.13-x64
…
To focus on the weirdness: `dotnet-local.sh` uses the .NET 8 that we
provision into `bin/$(Configuration)/dotnet`, which is .NET 8, while
the error message states that it can only find .NET 6 and 7 from
`/usr/local/share/dotnet`, and *isn't* looking at `bin/*/dotnet`!
.NET 8 Roslyn apparently requires that the "executing" `dotnet` be in
`$PATH`:
% PATH=…/xamarin-android/bin/Debug/dotnet:$PATH \
…/xamarin-android/dotnet-local.sh build *.csproj
# no errors
Update `dotnet-local.sh` and `dotnet-local.cmd` so that `bin/*/dotnet`
is prepended to `$PATH`. Additionally, for readability in
`dotnet-loca.sh`, explicitly export all `DOTNET*` environment
variables so that they don't all need to be on one line.
Update build scripts to use `-nodeReuse:false` instead of `-nr:false`
for readability.
…and some notes about `dotnet-local.cmd`: [`CMD.EXE`][0] environment
variable handling is "baroque".
By default, evironment variables added within a `.cmd` script are
exported to the caller. This is similar to Unixy platforms
["source"][1]ing a script instead of executing a script. To avoid
this behavior, the script should use [SETLOCAL][2]. As we want
`dotnet-local.cmd` to update `%PATH%`, add use of `SETLOCAL` so that
we don't update `%PATH%` on every `dotnet-local.cmd` invocation.
Environment variables are evalated *when a statement is read*,
***not*** when that statement is *executed*. This can make for some
"weird" behavior:
IF EXIST "%ROOT%\bin\Release\dotnet\dotnet.exe" (
SET XA_DOTNET_ROOT=%ROOT%\bin\Release\dotnet
echo XA_DOTNET_ROOT=%XA_DOTNET_ROOT%
)
The above fragment will print `XA_DOTNET_ROOT=` because
`%XA_DOTNET_ROOT%` is evaluated when the entire `IF EXIST …` statment
is read, *not* when the `echo` is executed. This has similar
"wierdness" if we instead attempt to update `PATH`:
IF EXIST "%ROOT%\bin\Release\dotnet\dotnet.exe" (
SET PATH=%ROOT%\bin\Release\dotnet;%PATH%
)
The above invariably fails with weirdness such as:
\NSIS\ was unexpected at this time.
or
\Microsoft was unexpected at this time.
The workaround is one of two options:
1. Consider use "delayed environment variable expansion"; see the
`SET /?` output [^0].
2. Don't Do That™: don't `SET` an environment variable to include
values which are also `SET` in the same statement, and `(…)`
is evaluated as a single statement.
We opt to go with (2), drastically simplifying `dotnet-local.cmd` so
that the `COMMAND` within `IF EXIST filename COMMAND` *only* sets
environment variables which do not reference other environment
variables within the same `COMMAND`
[0]: https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/cmd
[1]: https://linuxize.com/post/bash-source-command/
[2]: https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/setlocal
[^0]: From `SET /?`:
Delayed environment variable expansion is useful for getting around
the limitations of the current expansion which happens when a line
of text is read, not when it is executed. The following example
demonstrates the problem with immediate variable expansion:
set VAR=before
if "%VAR%" == "before" (
set VAR=after
if "%VAR%" == "after" @echo If you see this, it worked
)
would never display the message, since the %VAR% in BOTH IF statements
is substituted when the first IF statement is read, since it logically
includes the body of the IF, which is a compound statement. So the
IF inside the compound statement is really comparing "before" with
"after" which will never be equal. Similarly, the following example
will not work as expected:
set LIST=
for %i in (*) do set LIST=%LIST% %i
echo %LIST%
in that it will NOT build up a list of files in the current directory,
but instead will just set the LIST variable to the last file found.
Again, this is because the %LIST% is expanded just once when the
FOR statement is read, and at that time the LIST variable is empty.
So the actual FOR loop we are executing is:
for %i in (*) do set LIST= %i
which just keeps setting LIST to the last file found.
Delayed environment variable expansion allows you to use a different
character (the exclamation mark) to expand environment variables at
execution time. … |
Also, override %PATH% on Windows. This way the scripts are consistent between Unix & Windows.
Take…3?
The problem: running `dotnet-local.cmd build path\to\app.csproj` would error out on CI with:
\NSIS\ was unexpected at this time.
##[error]Process completed with exit code 255.
Running locally was no better:
\Microsoft was unexpected at this time.
like, what?
Eventually, after a bit of experimentation and "man page" reading, we see the `SET /?` output:
```
Finally, support for delayed environment variable expansion has been
added. This support is always disabled by default, but may be
enabled/disabled via the /V command line switch to CMD.EXE. See CMD /?
Delayed environment variable expansion is useful for getting around
the limitations of the current expansion which happens when a line
of text is read, not when it is executed. The following example
demonstrates the problem with immediate variable expansion:
set VAR=before
if "%VAR%" == "before" (
set VAR=after
if "%VAR%" == "after" @echo If you see this, it worked
)
would never display the message, since the %VAR% in BOTH IF statements
is substituted when the first IF statement is read, since it logically
includes the body of the IF, which is a compound statement. So the
IF inside the compound statement is really comparing "before" with
"after" which will never be equal. Similarly, the following example
will not work as expected:
set LIST=
for %i in (*) do set LIST=%LIST% %i
echo %LIST%
in that it will NOT build up a list of files in the current directory,
but instead will just set the LIST variable to the last file found.
Again, this is because the %LIST% is expanded just once when the
FOR statement is read, and at that time the LIST variable is empty.
So the actual FOR loop we are executing is:
for %i in (*) do set LIST= %i
which just keeps setting LIST to the last file found.
Delayed environment variable expansion allows you to use a different
character (the exclamation mark) to expand environment variables at
execution time. …
```
:thinking:
As a quick experiment, consider this fragment:
```
IF EXIST "%ROOT%\bin\Release\dotnet\dotnet.exe" (
SET XA_DOTNET_ROOT=%ROOT%\bin\Release\dotnet
echo # jonp: XA_DOTNET_ROOT=%XA_DOTNET_ROOT%
)
```
The result is that it would print:
```
# jonp: XA_DOTNET_ROOT=
```
i.e. *the empty string*!
My hypothesis is that because environment expansion was "immediate" (not "delayed"), attempting to do `SET PATH=%XA_DOTNET_ROOT%;%PATH%` would result in "weird recursion", and thus all the errors.
The fix? Avoid immediate environment variable expansion, by LIMITING what is done in statement for `IF CONDITION STATEMENT`. Instead of executing, *only* set env vars:
```
IF EXIST "%ROOT%\bin\Release\dotnet\dotnet.exe" (
SET XA_DOTNET_ROOT=%ROOT%\bin\Release\dotnet
SET XA_CONFIG=Release
) …
```
Then when we later try `SET PATH=%XA_DOTNET_ROOT%;%PATH%`, things are "saner", and -- at least locally -- we shouldn't get those "bizarre" `… was unexpected at this time` messages.
Finally, use `SETLOCAL` so that our updates to `%PATH%` don't "overwrite" the calling environment.
|
The Windows CI build is green. I guess that means that the hypothesis of 1619c48 is correct! |
Cleanup
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The recent bump to dotnet broke the dotnet-local scripts. It now seems that the "local" path must be FIRST in the PATH in order to pick up the packs.
Also added a smoke test step to make sure the script works, at least one Mac and Linux.