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

Behavior change when running on 9.0.0-preview.7.24405.7 #817

Closed
costin-zaharia-sonarsource opened this issue Sep 13, 2024 · 10 comments
Closed

Comments

@costin-zaharia-sonarsource
Copy link

costin-zaharia-sonarsource commented Sep 13, 2024

I've noticed a behavior change when the tool is running on the .NET 9 preview runtime.

When output task is used, the name of the parameter changes from <ItemName> to <ItemName> from parameter <TaskParameter>.

For example, we can have a .csproj file in the following form:

<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="12.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
    <Target Name="Test">
        <XmlPeek XmlInputPath="test.xml"
                 Query="//s:variable/@Name"
                 Namespaces="&lt;Namespace Prefix='s' Uri='http://nsurl' /&gt;">
            <Output TaskParameter="Result" ItemName="ResultOutput" />
        </XmlPeek>
    </Target>
</Project>

When running on an older runtime (like 6.0) the parameter is named ResultOutput

image

On the other hand when running on .NET 9.0.0-preview.7.24405.7, the parameter is named ResultOutput from parameter Result

image

You can find here the sources of the reproducer: reproducer.zip

I used a global.json file to change the runtime.

@KirillOsenkov
Copy link
Owner

@JanKrivanek any quick guess what might have changed in MSBuild?

@KirillOsenkov
Copy link
Owner

it's a good change (it gives more info), need to think whether to present it slightly differently in the viewer

btw love great bug reports! detailed and with a minimal repro!

@costin-zaharia-sonarsource
Copy link
Author

btw love great bug reports! detailed and with a minimal repro!

Thanks, @KirillOsenkov! I like the MSBuildStructuredLog tool and I find it very useful.

it's a good change (it gives more info), need to think whether to present it slightly differently in the viewer

On our side, we are more affected by this change when we're using the library since now BinaryLog.ReadBuild(filePath).VisitAllChildren<Property>(x => ..) returns slightly different values. Nothing major though.

@JanKrivanek
Copy link
Collaborator

JanKrivanek commented Sep 13, 2024

Thanks for report and thanks for tagging me here.

@ladipro was working on improving and enriching the info around task parameters:

It's comming from the first one.

To me this honestly feels as pure improvement.
Being dependent on specific order on the other hand feels too rigid to allow improving changes.
I might have very oversimplified PoV due to lack of knowledge of the specific scenario. If it would feel apropriate to accomodate the scenario, please try to elaborate more.

@KirillOsenkov
Copy link
Owner

I guess perhaps we can parse the message and just keep the item name as earlier, but add the parameter mapping on the side somewhere, because now the item name is not really the item name so analyzers looking for $additem for rxample won't match this. I can look later to see if I have ideas on how to represent this better.

@JanKrivanek
Copy link
Collaborator

Or this change can be reverted:

https://github.com/KirillOsenkov/MSBuildStructuredLog/pull/780/files#diff-c24cc7981706583e29dbaa574cfa45c811c418166d928225844132d8425c993aL196-R203

or applied just as a tooltip or so.

I'm happy to adjust this next week - just lm know

@KirillOsenkov
Copy link
Owner

Yes, I think we need to change this somehow and expose this information in a different way. The item name should be just item name.

@lambdageek
Copy link
Contributor

👍 on adding some more structure around this (maybe a subclass of AddItem for output items?)

FYI i'm working on a little tool that parses a binlog and creates a .proj that re-runs some selected tasks (https://github.com/lambdageek/monostump) an {item} from parameter {paramName} makes it tough to roundtrip a binlog into a project file

@KirillOsenkov
Copy link
Owner

How's this?

image

lambdageek added a commit to lambdageek/monostump that referenced this issue Sep 19, 2024
@KirillOsenkov
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants