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

csharpier as msbuild dependency doesn't fail the github action anymore #1357

Closed
OneCyrus opened this issue Oct 9, 2024 · 7 comments · Fixed by #1396 or #1397
Closed

csharpier as msbuild dependency doesn't fail the github action anymore #1357

OneCyrus opened this issue Oct 9, 2024 · 7 comments · Fixed by #1396 or #1397
Milestone

Comments

@OneCyrus
Copy link

OneCyrus commented Oct 9, 2024

Environments:

  • CSharpier Version: 0.29.2
  • Running CSharpier via: dotnet
  • Operating System: Ubuntu 22.04
  • .csharpierrc Settings:
  • .editorconfig Settings:

Steps to reproduce:

dotnet build --configuration Release /p:CSharpier_LogLevel=Error

Expected behavior:

we are running csharpier as msbuild dependency and want the build to fail of the code is not formatted. it used to work before but currently even when an error is detected the build succeeds and the github action workflow doesn't fail.

formatting error is detected correctly:

Error /home/runner/work/automation-data/automation-data/Poseidon/Poseidon/GqlSchema/ITSM/Itsm.cs - Was not formatted.
    ----------------------------- Expected: Around Line 1 -----------------------------
    using System.Linq.Expressions;
    using System.Reflection;
    ----------------------------- Actual: Around Line 1 -----------------------------
    using Aveniq.Poseidon.GqlSchema.CMDB;
    using Aveniq.Poseidon.GqlSchema.SMDB;

build fails:

Build failed.

    0 Warning(s)
    1 Error(s)

Actual behavior:

build succeeds and the exit code doesn't fail the github action job.

Build succeeded.

    0 Warning(s)
    0 Error(s)
@OneCyrus
Copy link
Author

OneCyrus commented Oct 9, 2024

this started to happen with 0.29.0. probably the issue is here:

@OneCyrus OneCyrus changed the title cshaprier as msbuild dependency doesn't fail the github action anymore csharpier as msbuild dependency doesn't fail the github action anymore Oct 14, 2024
@OneCyrus
Copy link
Author

OneCyrus commented Dec 6, 2024

@belav as this is still blocking us, are you aware of a workaround to fail the build when csharpier is reporting an error?

@belav
Copy link
Owner

belav commented Dec 6, 2024

@OneCyrus hey sorry I lost track of this one, I'll see if it's something I can resolve today for you

@belav belav added this to the 0.30.3 milestone Dec 6, 2024
belav added a commit that referenced this issue Dec 6, 2024
closes #1357
@belav
Copy link
Owner

belav commented Dec 6, 2024

@OneCyrus IgnoreExitCode is behaving inconsistently between linux and windows. That was added to cleanup the output.

With the new PR, windows outputs this when it runs into unformatted files

    Project -> D:\a\csharpier\csharpier\Tests\MsBuild\TestCases\UnformattedFileCausesError\bin\Release\net8.0\Project.dll
  
  Build FAILED.
  
  EXEC : error D: /a/csharpier/csharpier/Tests/MsBuild/TestCases/UnformattedFileCausesError/UnformattedFile.cs - Was not formatted. [D:\a\csharpier\csharpier\Tests\MsBuild\TestCases\UnformattedFileCausesError\Project.csproj]
      0 Warning(s)
      1 Error(s)
  

vs linux, which duplicates the error.

  /home/runner/.nuget/packages/csharpier.msbuild/0.0.1/build/CSharpier.MsBuild.targets(27,5): error MSB3073: The command ""/usr/share/dotnet/dotnet" exec "/home/runner/.nuget/packages/csharpier.msbuild/0.0.1/build/../tools/csharpier/net9.0/dotnet-csharpier.dll"  --check --no-msbuild-check --compilation-errors-as-warnings "/home/runner/work/csharpier/csharpier/Tests/MsBuild/TestCases/UnformattedFileCausesError" > /dev/null " exited with code 1. [/home/runner/work/csharpier/csharpier/Tests/MsBuild/TestCases/UnformattedFileCausesError/Project.csproj]
  
  Build FAILED.
  
  /home/runner/.nuget/packages/csharpier.msbuild/0.0.1/build/CSharpier.MsBuild.targets(27,5): error MSB3073: The command ""/usr/share/dotnet/dotnet" exec "/home/runner/.nuget/packages/csharpier.msbuild/0.0.1/build/../tools/csharpier/net9.0/dotnet-csharpier.dll"  --check --no-msbuild-check --compilation-errors-as-warnings "/home/runner/work/csharpier/csharpier/Tests/MsBuild/TestCases/UnformattedFileCausesError" > /dev/null " exited with code 1. [/home/runner/work/csharpier/csharpier/Tests/MsBuild/TestCases/UnformattedFileCausesError/Project.csproj]
      0 Warning(s)
      1 Error(s)

I'm thinking release this as is, and come back later and try to clean up the duplication.

@OneCyrus
Copy link
Author

OneCyrus commented Dec 6, 2024

I see. While it's not perfect I would still prefer duplicated messages if this would result in reporting issues as errors again.

@PetSerAl
Copy link
Contributor

PetSerAl commented Dec 6, 2024

I think inconsistency is in that on Windows error message, due to colon (:) in path, recognized as standard error message (https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-diagnostic-format-for-tasks?view=vs-2022). You should probably add IgnoreStandardErrorWarningFormat to Exec if it is not your intention. Or add command line option to format errors in MSBuild compatible way.

@PetSerAl
Copy link
Contributor

PetSerAl commented Dec 6, 2024

I suspect changing CSharpier output from Error path to Error: path should make it to be recognized as MSBuild error both on Windows and Linux, and also take care of extra space added after : on Windows, which make path unclickable in VS Code terminal in some cases.

belav pushed a commit that referenced this issue Dec 7, 2024
As CSharpier currently does not output error messages in MSBuild
compatible format, adapt Exec options to current error format. Should
fix #1357.
belav added a commit that referenced this issue Dec 7, 2024
belav added a commit that referenced this issue Dec 7, 2024
belav added a commit that referenced this issue Dec 7, 2024
)

The changes for #1311 seem to
work in windows, but cause problems in linux. Files that aren't
formatted are no longer treated as errors.

closes #1357
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants