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

PublishAot doesn't respect TreatWarningsAsErrors #94178

Closed
eerhardt opened this issue Oct 30, 2023 · 4 comments
Closed

PublishAot doesn't respect TreatWarningsAsErrors #94178

eerhardt opened this issue Oct 30, 2023 · 4 comments

Comments

@eerhardt
Copy link
Member

Description

When publishing an app for AOT, and TreatWarningsAsErrors=true, if there are trim/AOT warnings, the publish doesn't fail as errors.

However, when I <PublishTrimmed>true</PublishTrimmed> the same app (with the same trimming warnings), the publish fails.

We should be consistent in our behavior between these 2 publish operations and TreatWarningsAsErrors.

Reproduction Steps

In your .csproj:

    <PublishAot>true</PublishAot>
    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>

And have trim warnings in your app.

Expected behavior

The publish should fail because I have warnings and TreatWarningsAsErrors=true.

Actual behavior

The trim/AOT warnings are emitted as warnings, but these warnings were not treated as errors, and my publish succeeds.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

cc @vitek-karas @agocke @MichalStrehovsky

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 30, 2023
@ghost
Copy link

ghost commented Oct 30, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

When publishing an app for AOT, and TreatWarningsAsErrors=true, if there are trim/AOT warnings, the publish doesn't fail as errors.

However, when I <PublishTrimmed>true</PublishTrimmed> the same app (with the same trimming warnings), the publish fails.

We should be consistent in our behavior between these 2 publish operations and TreatWarningsAsErrors.

Reproduction Steps

In your .csproj:

    <PublishAot>true</PublishAot>
    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>

And have trim warnings in your app.

Expected behavior

The publish should fail because I have warnings and TreatWarningsAsErrors=true.

Actual behavior

The trim/AOT warnings are emitted as warnings, but these warnings were not treated as errors, and my publish succeeds.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

cc @vitek-karas @agocke @MichalStrehovsky

Author: eerhardt
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@ivanpovazan
Copy link
Member

/cc: @simonrozsival

@vitek-karas
Copy link
Member

internal static bool IsWarningAsError(int _/*code*/)
{
// TODO: warnaserror
return false;
}

😞

This brings up an interesting behavior of MSBuild - Setting TreatWarningsAsErrors=true only applies to things which specifically react to that property, it's not a MSBuild intrinsic feature.
On the other hand passing /warnaserror on the command line to MSBuild is an intrinsic feature which makes the build fail if there are warnings. Running the repro with dotnet publish /warnaserror will actually report an error and fail the build (although it does still produce output, unlike TreatWarningsAsErrors=true which will make illink return unsucessful exit code and thus stop the build).

I do agree that we should implement this to get parity between the tools, regardless of the confusing behavior of MSBuild in this case.

@agocke agocke added this to AppModel Nov 28, 2023
@agocke agocke added this to the 9.0.0 milestone Dec 2, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Dec 2, 2023
vitek-karas added a commit that referenced this issue Jan 19, 2024
Both roslyn and trimmer implement "warn as error" as a compiler feature - specifically the `TreatWarningsAsErrors` property only affects tools which explicitly react to it. There's a related MSBuild command line option which will affect all warnings, but that's not the one VS sets from the UI and what most customers use. See the comments in #94178.

This implements the same behavior as the trimmer. It adds 3 new command line options to `ilc` and enable "warn as error" globally and to enable/disable specific warning codes. Note that `illink` implements a more complex command line parsing where order of parameters matters and sometimes the later ones can override the earlier ones. This was trying to emulate `csc` behavior. For `ilc` I don't see a reason to emulate that because running `ilc` on its own is not supported, only going through the SDK, which will never pass the new parameters multiple times.

For testing this enables the existing trimmer tests for this functionality. This uncovered a small issue in substitution parser, which is also fixed here. For simplicity the test infra emulates the `illink` command line behavior over `ilc` (which is easy as they're very similar).
@vitek-karas
Copy link
Member

#96567 implements the same behavior as illink has, so while still not perfect (the whole discrepancy between compilers and msbuild ways of doing this), NativeAOT should now behave basically the same as the compiler and trimmer.

tmds pushed a commit to tmds/runtime that referenced this issue Jan 23, 2024
…6567)

Both roslyn and trimmer implement "warn as error" as a compiler feature - specifically the `TreatWarningsAsErrors` property only affects tools which explicitly react to it. There's a related MSBuild command line option which will affect all warnings, but that's not the one VS sets from the UI and what most customers use. See the comments in dotnet#94178.

This implements the same behavior as the trimmer. It adds 3 new command line options to `ilc` and enable "warn as error" globally and to enable/disable specific warning codes. Note that `illink` implements a more complex command line parsing where order of parameters matters and sometimes the later ones can override the earlier ones. This was trying to emulate `csc` behavior. For `ilc` I don't see a reason to emulate that because running `ilc` on its own is not supported, only going through the SDK, which will never pass the new parameters multiple times.

For testing this enables the existing trimmer tests for this functionality. This uncovered a small issue in substitution parser, which is also fixed here. For simplicity the test infra emulates the `illink` command line behavior over `ilc` (which is easy as they're very similar).
@github-actions github-actions bot locked and limited conversation to collaborators Feb 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

No branches or pull requests

4 participants