-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
Windows: MSB4016 error on build #5
Comments
Might be a Windows-specific issue, |
Thanks for testing. It looks like it's probably the net6.0 preview that is failing with something here. @KirillOsenkov I'm using StructuredLogger here (from NuGet
|
It doesn't, and I can't repro on my machine, but I'd certainly be the right person to investigate this and it's likely my fault somehow :) Do you have a full call stack? You can use the Child Process Attach VS extension: Then debug dotnet-releaser.exe with Mixed mode (Managed + Native) in VS, enable Child Process Attach, enable first-chance exceptions (Debug -> Exceptions -> All .NET exceptions), then debug and it will get you a call stack of the exception. Paste that callstack here (and ideally the source location where it happens) and we can investigate further. It looks like a bug in MSBuild currently (nothing a user is doing should be causing a NullRef in MSBuild), but we need to understand what is going on first (and whether it's MSBuild or the SDK). Excellent repro steps @rgwood, I appreciate good bugs when I see them! |
Happy to try debugging in VS but I'm not entirely sure how (I've just been using the CLI outside of VS). Would I need to clone this repo or is there a way to debug the dotnet-releaser.exe I installed as a global tool? edit: on second thought I'll just clone the repo, that should be quick to try |
@KirillOsenkov I've installed the extension but I'm not getting a call stack when I debug. Does this look right? Child Process Debugging enabled: Debugging with F5 fails as expected but VS never breaks on an exception: |
There's a way to debug an .exe in VS if you don't have the sources. In VS, open the .exe as a Project (File -> Open Project), it will open as a pseudo-project, you can right-click the "project" in Solution Explorer, open Properties, fill out the arguments, working directory, environment variables, debug engine (has to be Mixed for Child Process Attach to work), then just F5. This trick is generally useful if you don't have the source. Or you can attach to an already running process, but it's less convenient and since JIT optimizes all modules on load by default, you won't see any variables in the locals window. |
Can you try to uncheck Tools -> Options -> Debugging -> Just my code? But it's strange, I'd expect it to work. When it runs, do you see additional processes showing up in Debug -> Windows -> Processes? It may be that it isn't really attaching to the spawned dotnet msbuild process? |
Also you can try setting the MSBuildDebugEngine environment variable, it will dump binlogs for everything. Not sure if that will help here or not. |
Disabling Just My Code worked!
|
oh oh, some reflection going on here 😄 Seems that the field Edit: Commit changing this field is here |
Thanks for reproducing the bug @rgwood and investigating it! |
Do you have many places like this in StructuredLogger @KirillOsenkov ? Does it mean that to completely protect it against such changes, we would need MSBuild to expose proper public API for these? |
Could these reflections be more protected by putting them separately somewhere with a validator that could check frequently with a recent MSBuild DLL? |
Ouch. And fantastic job @rgwood, this is really helpful. I guess we haven't seen this because not a lot of people use the BinaryLogger copy from StructuredLogger.dll (almost everyone uses the original from MSBuild). Yeah, let's think about what's the right thing to do is here. I filed KirillOsenkov/MSBuildStructuredLog#556 so feel free to watch that for further development. For now the cheapest tactical fix is to probe for both field names and use the one that's not null. |
@xoofx would you please try to update to https://www.nuget.org/packages/MSBuild.StructuredLogger/2.1.617 and see if this fixes the issue? Thanks! |
And to answer your question about why Reflection is necessary - these fields shouldn't be exposed to the general public, but I need access for obscure reasons (binary serialization). Maybe it's worth thinking about opening them up, but in MSBuild land, once we open anything up, we can never change it, so we've been cautious about making more foundational stuff public. The perf improvement two months ago that broke my reflection wouldn't have been possible had we had those fields public. |
I couldn't reproduce it as I avoid installing previews on my dev machine. But I will ship a new version of |
Attempting to confirm the fix. The error is gone on
|
Would you try to debug again with Child Processes and first chance exceptions? |
I think the error is |
Confirmed that build succeeds on v0.1.2 if I add this to the config:
|
Great. The changelog issue should be fixed in |
Yup, confirmed that build (usually) works with the default config on v0.1.3. Thank you both for fixing things so quickly! I am running into occasional build failures related to StructuredLogger but I can raise that in another issue elsewhere if it would help:
|
Thanks, I’ve filed KirillOsenkov/MSBuildStructuredLog#558 and will investigate it there. |
One thing you said @KirillOsenkov that I wanted to clarify:
The reason I'm using
So does it make sense to use StructuredLogger like this in my case or should I use instead the MSBuild built-in version? |
I think it’ll be better to just use -bl (the logger built into MSBuild). It has been supported since 15.3 (2017). Both the viewer as well as StructuredLogger.dll support reading binlogs of any version. I was wondering why you’re using StructuredLogger.dll. I don’t think you’ll support ancient MSBuilds such as 14.0? |
Oh, ok, so I thought that using the logger of StructuredLogger would make sure that I could actually re-read it after. If you say that using MSBuild gives more guarantee then definitely, I will revert that. It will remove the hack I had to go through to have the StructuredLogger dll still packed on the side. |
Removed by commit e0825f8 |
Plus, I think even StructuredLogger.dll of recent versions has lost any hope of working with MSBuild < 16.0, so there’s really no scenario I can think of where StructuredLogger.dll should be used for recording. On the other hand it’s main usage is for reading binlogs (it is used by the viewer) as well as API (to read and analyze binlogs) It all stems from historical reasons when MSBuild didn’t have /bl built-in. Now /bl is ubiquitous in any MSBuild since 2017 so the copy in StructureLogger.dll is redundant. I still keep it up-to-date (poorly, as we have seen) just in case. |
Great, thanks! I have removed using it from |
Hi, I'm following the instructions from the README but I'm running into an MSBuild failure when I try to run
dotnet-releaser build
. I've put together a minimal repro here.Steps to Reproduce
Create a console app:
Initialize the
dotnet-releaser
config:Run
dotnet-releaser build
, observe the "Failing to run dotnet msbuild" error:Version Info
dotnet-releaser
v0.1.0, Windows 11, and the .NET 6 SDK that comes installed with VS 2022 17.1.0 Preview 4:P.S. Thank you so much for making
dotnet-releaser
- it fills a really important gap in the .NET OSS ecosystem!The text was updated successfully, but these errors were encountered: