oxsecurity / megalinter

🦙 MegaLinter analyzes 50 languages, 22 formats, 21 tooling formats, excessive copy-pastes, spelling mistakes and security issues in your repository sources with a GitHub Action, other CI tools or locally.
https://megalinter.io
GNU Affero General Public License v3.0
1.89k stars 227 forks source link

v6.18.0 breaks workflows in Unity projects #2248

Open sm-quasar opened 1 year ago

sm-quasar commented 1 year ago

Describe the bug v6.18.0 fails to locate an MSBuild project file or solution file and is unable to test our C# scripts and thus cannot validate them.

Unhandled exception: System.IO.FileNotFoundException: Could not find a MSBuild project file or solution file in '/github/workspace'

After the upgrade to the .18 release our megalinter workflow began reporting warnings instead of failing/fixing PRs. We currently run it just against changed files in a Unity project ( this potentially puts us in an odd position as we have no .csproj or .sln committed since Unity will auto gen these and regularly stomps over them making for messy commit history). This was all working fine for us in V6.17.0 But something in the updated version has broken the flow (.Net 6 support maybe?)

For now we've returned to the .17 release but that obviously isn't what we'd like to do long term

To Reproduce Steps to reproduce the behavior:

  1. Create a unity project
  2. Add some .cs scripts
  3. Add megalinter github action workflow to validate all changed files
  4. Observe the below error Unhandled exception: System.IO.FileNotFoundException: Could not find a MSBuild project file or solution file in '/github/workspace'

See attached logs

Expected behavior Dotnet Format should have ran against the .cs files without error or warning

Additional context Attached logs from the two versions in question and our yml file

v6.17.0 Log SUCCESS-CSHARP_DOTNET_FORMAT.log

v6.18.0 Log WARNING-CSHARP_DOTNET_FORMAT.log

Our .mega-linter.yml file (renamed for github's own sanity) mega-linter.txt

Kurt-von-Laven commented 1 year ago

Thank you for the report. What version of .NET are you on? What happens when you pass the workspace argument referenced in the error message? In the meantime, I will share how we use MegaLinter with .NET in case it proves helpful to you or others as either a temporary or long-term workaround. We have long found that CSharpier, just added in v6.18.0 thanks to the hard work of @bdovaz, is a much faster linter than dotnet-format. CSharpier automatically fixes all issues it detects, requires little to no configuration, and personally I find its style more readable than dotnet-format's defaults. The two linters are by-and-large redundant with one another, and dotnet-format was by far our slowest linter, so we disabled it to create a speedy .NET flavor that we can run locally on every commit and push. If you aren't wedded to dotnet-format but weren't put off by its performance and want something less opinionated than CSharpier, ReSharper's CleanupCode is extremely feature rich and flexible and not limited to C#/F#. It isn't included in MegaLinter, so it would need to be run separately or as a plugin.

sm-quasar commented 1 year ago

Unity is a pinch awkward in terms of .NET version, I believe I'm currently using .NET Standard 2.1 but could migrate to .NET 4.8 (Unity is quite out of date unfortunately)

I'll take a look at CSharpier, I'm not particularly married to either as long I can keep using it to apply standards automatically I'm very happy :)

Kurt-von-Laven commented 1 year ago

In that case, I believe your original hypothesis was correct, and you were broken by the upgrade to .NET 6. I don’t believe it will be feasible for you to run dotnet-format via recent versions of MegaLinter while on .NET Standard 2.1. I don’t know enough to advise whether migrating to .NET 4.8 would be sufficient, so that might be a question for Stack Overflow if it matters to you. Let us know if you have any feedback regarding running CSharpier via MegaLinter. Hope you find it as helpful as we have!

echoix commented 1 year ago

.NET Standard 2.1 is closer to .NET 6 than .NET Framework 4.8 is to .NET 6. .NET Framework is (part of) the original, old .NET. Then came .NET core, a complete rewrite from scratch of .NET, open source and cross platform, by using everything that they learned in the decades of software development.

So in the timeline, .NET core were released, versions 1.0, 1.1, 2.0, 2.1, 2.2, 3.0, 3.1, and there was some extra work on .NET framework versions like 4.7 and 4.8 before stopping at 4.8. In the time when both .NET core and .NET framework coexisted (and they still coexist in the wild), a way to have some code compatible between the two needed to exist. Imagine libraries for example, they should be able to be used by all. So that's where .NET standard came. .NET standard isn't really a release of .NET, it's a base specification of APIs that are common to both .NET core versions and .NET Framework versions. To run code that targets .NET Standard, it needs a .NET runtime that implements that .NET standard version.

That seemed like a good idea, until it was just more confusing than anything, and .NET core was really there to stay. At the same time, to prevent any confusion, the release following .NET core 3 was .NET 5, and the .NET core project was renamed to .NET (without changing pas releases). Now, since .NET 5, the concept of .NET Standard is left behind in favour of a new mechanism to assure compatibility with other releases starting with .NET 5. I don't understand that mechanism yet since a couple years.

There will be no more .NET Standard releases, but MS and Dotnet as an organization sticks to their old promises and keeps it working as promised.

image image

https://learn.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-1-0

https://dotnet.microsoft.com/en-us/platform/dotnet-standard

https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core

So following this, if the Unity project and code was made for .NET Standard 2.1, and worked with .NET 5 (.NET 5 was the implemented of .NET Standard 2.1 in the previous MegaLinter release 6.17), there is no real reason for it to not work with .NET 6 in this MegaLinter release 6.18. Thus, I suspect something else, or that the dotnet format needs absolutely to load the code and project with the specific version of the dotnet SDK that the project is defined.

echoix commented 1 year ago

I hope this makes a good fast overview of the history for @nvuillam and @bdovaz that were often wondering how the dotnet environnement worked.

sm-quasar commented 1 year ago

Thanks all for the info :) and a great reminder of .NETs history

This definitely seems to be a dotnet format issue rather than a Megalinter issue (that said a breaking change version may indicate the ramifications of the .NET 6 upgrade clearer).

The only thing I see left that could be an ML issue that might need fixing is that this all gets reported as a warning and thus our workflow continued passing despite unhandled exceptions when running dotnet format

For now I'll look at CSharpier and whether I agree with it's heavily opinionated format or if for now I'll stick on megalinter version 6.17.0 for now

The final option I can look at is to force the generation of the csproj files before running megalinter, it's possible with Unity to do this but it's slow as you incur the cost of a project import (several hours for big projects)

Thanks again all

nvuillam commented 1 year ago

@echoix that's indeed very interesting, I never did a single line in C# and never used .NET env, so I'm a total newbie in the topic, and i'm very glad to have you and MegaLinter community to provide support on it :)

@sm-quasar > @bdovaz is himself a Unity developer, and he's at the origin of CSharper integration within MegaLinter, so switching to CSharpier is probably a good idea :)

sm-quasar commented 1 year ago

Good to know :) Yea I ran a pass of it this morning and wow is it fast! I only did a quick check of it's formatting but it largely looked like I wanted anyway so I probably will switch to, though may still using an .editorconfig to help with naming conventions in IDEs at least

bdovaz commented 1 year ago

As @nvuillam says I am a professional Unity developer (+10 years) and in our workflow we use dotnet-format but as we already had it before Megalinter, we have it integrated outside of it.

Let's see if I find the time and test it from Megalinter to see if it gives us problems or not.

In our case the project is Unity 2021.3.x (LTS) + .NET Standard 2.1.

sm-quasar commented 1 year ago

@bdovaz Nice, I too am a professional unity Dev (just under 10 years).

I'm curious about how your setup looks, do you commit the csproj files (I tend not to as they generate a lot of noise in my experience), do you force the generation of them as part of the ci flow, are you using the dotnet format version that comes with .NET 6 or the version that shipped prior to its move to ship with .NET?

Any advice appreciated

My brief testing this morning seemed to indicate that the latest version of dotnet format shipped with .NET 6 doesn't work with a unity project either (with the .sln and .csproj files removed)

Also I appreciate this has now divulged from being about a Megalinter issue

bdovaz commented 1 year ago
  1. I use UseDotNet@2 to install .NET 6.x SDK
  2. I run Unity in batch mode to generate the necessary files (.sln + .csproj). The key is in: -executeMethod UnityEditor.SyncVS.SyncSolution.
  3. I execute the corresponding command of dotnet format.
  4. I create a branch, I commit the modified files and I create a PR that has to pass some policies.

As I say, we have this pipeline before integrating Megalinter, so it does not use the dotnet format linter of Megalinter. I have to test to see if doing it with Megalinter works the same way.

nvuillam commented 1 year ago

I don't know what I'm speaking about, but... maybe if it is boring to generate thise .csproj files, an option of MegaLinter (activated or not by default) could generate them during the CI before running dotnet format ?

sm-quasar commented 1 year ago

Ahhh okay, yeah I see how you're getting around the restriction I'm facing, the sync solution approach is one I've used in the past though I think last i tried that it still incurred the import cost, that may have changed now though which would be a big win

My use case is currently running this on all PRs as a GitHub action using GitHub hosted larger runners, so I think in order to avoid installing Unity as part of that particular workflow I may swap to CSharpier

Appreciate all the advice though and I'll check out the sync solution approach for some future automations I'm planning ☺️

sm-quasar commented 1 year ago

I don't know what I'm speaking about, but... maybe if it is boring to generate thise .csproj files, an option of MegaLinter (activated or not by default) could generate them during the CI before running dotnet format ?

The reason this probably isn't viable is because it's highly dependent on having the right version of unity installed

nvuillam commented 1 year ago

@sm-quasar with PRE_COMMANDS we can configure MegaLinter to install whatever we want before running linters, so we could define something like that (i don't know if the commands are easy to build)

PRE_COMMANDS:
  - command line to install unity with desired version
  - command line to compile the project

https://megalinter.io/latest/configuration/#pre-commands

sm-quasar commented 1 year ago

@sm-quasar with PRE_COMMANDS we can configure MegaLinter to install whatever we want before running linters, so we could define something like that (i don't know if the commands are easy to build)

PRE_COMMANDS:
  - command line to install unity with desired version
  - command line to compile the project

https://megalinter.io/latest/configuration/#pre-commands

So it's quite easy to grab the unity version from a unity project to help build those commands, the install is pretty large and would take a while maybe 15 minutes or so network speed depending. But once you know the unity version you've two options GameCI maintain docker images with each version&OS or you could install Unity Hub and use the totally undocumented but very useful cli it offers to install the version and required modules (possibly not needed for this purpose but would need testing)

bdovaz commented 1 year ago

Ahhh okay, yeah I see how you're getting around the restriction I'm facing, the sync solution approach is one I've used in the past though I think last i tried that it still incurred the import cost, that may have changed now though which would be a big win

My use case is currently running this on all PRs as a GitHub action using GitHub hosted larger runners, so I think in order to avoid installing Unity as part of that particular workflow I may swap to CSharpier

Appreciate all the advice though and I'll check out the sync solution approach for some future automations I'm planning ☺️

In our case, everything that has to do with Unity we do it in self-hosted agents because otherwise as you say, it is not very operative to have to download and install Unity in each build... I know it exists: https://game.ci/docs/docker/docker-images

But the truth is that with the complexity that Unity has + the N platforms with their particularities (UWP, iOS, WebGL, etc...) we prefer to have "more control" over what happens at all times and to be able to access by remote desktop to the agents in case something strange happens.

I don't know what I'm speaking about, but... maybe if it is boring to generate thise .csproj files, an option of MegaLinter (activated or not by default) could generate them during the CI before running dotnet format ?

Unity's compilation pipeline is "custom" and does not use msbuild, so there is always a double compilation: your IDE (VS, Rider, ...) + Unity IDE.

The generation of the .sln and .csproj files is the responsibility of Unity + in the case of VS, an extension managed by Microsoft: https://learn.microsoft.com/en-us/visualstudio/gamedev/unity/get-started/visual-studio-tools-for-unity?pivots=windows.

By not using Unity msbuild, the .sln and .csproj files are ignored with .gitignore because they are autogenerated under certain circumstances (when creating / deleting a script, when creating an asmdef (= assembly), etc...) so these files are "useless" in the sense that you cannot configure properties of the *.csproj files or use NuGet or practically anything with the .NET ecosystem.

All these problems come because historically Unity supported 3 languages (Boo, UnityScript and C#) which is why it could not be "coupled" to msbuild.

Now they are slowly solving all these problems: https://blog.unity.com/technology/unity-and-net-whats-next

But it is something that until at least 2024 I don't think they will solve.

sm-quasar commented 1 year ago

But the truth is that with the complexity that Unity has + the N platforms with their particularities (UWP, iOS, WebGL, etc...) we prefer to have "more control" over what happens at all times and to be able to access by remote desktop to the agents in case something strange happens.

I 100% agree with this, I used to operate and manage my previous studio's build farm which all ran Jenkins and a boat load of custom logic to keep things moving smoothly

Currently, for cost / infra reasons in a new studio, I'm actually using GameCI's github actions in combination with Github's beta Larger Runners which is working okay so far, but I definitely prefer a higher level of control when it comes to Unity

Now they are slowly solving all these problems: https://blog.unity.com/technology/unity-and-net-whats-next But it is something that until at least 2024 I don't think they will solve.

It's gonna be really interesting to re evaluate ci pipelines once this is done and see if there are more standard approaches that can be taken :)

Kurt-von-Laven commented 1 year ago

Good to know :) Yea I ran a pass of it this morning and wow is it fast! I only did a quick check of it's formatting but it largely looked like I wanted anyway so I probably will switch to, though may still using an .editorconfig to help with naming conventions in IDEs at least

A big bravo to @belav for writing CSharpier, who I thought might appreciate seeing this thread and knowing that his tool is distributed via MegaLinter.

jokay commented 1 year ago

I can confirm this for .NET 6.0.

(sample.csproj & sample.sln) -> sample.zip

sm-quasar commented 1 year ago

Btw I think we can close this issue as it has a couple of options for a solution

  1. If possible commit the .csproj or the .sln files
  2. If that is not possible switch to CSharpier

@nvuillam Should we open a new issue for the fact that the issues seen here only result in a warning in the action runner and thus could be failing relatively silently for folk not looking at the comment ML leaves and just seeing the workflow passed and moving on?

jokay commented 1 year ago

@sm-quasar well we have the csproj and sln comitted to the repo (since the beginning) and it does fail using 6.18.0 (does not using 6.17.0).

And do you think CSharpier can completely replace dotnet format?

sm-quasar commented 1 year ago

@sm-quasar well we have the csproj and sln comitted to the repo (since the beginning) and it does fail using 6.18.0 (does not using 6.17.0).

@jokay Ah my apologies I misunderstood your issue as resolved, that's very mysterious indeed, to confirm the csproj and sln files are in that '/builds/demo' folder?

And do you think CSharpier can completely replace dotnet format?

Completely, no. CSharpier is opinionated and so has very few config options. However it's opinion is by and large inline with ms standards and so it's working pretty well for me :) I adjusted my .editorconfig to match it's rules so I could still use it for things CSharpier doesn't care about like my naming standards (at least in my IDE anyway, nothing actually forces those now

nvuillam commented 1 year ago

As I don't know dotnet, i can't help a lot :/ Isn't there an option somewhere that would format even if those files are not here ? :/

jokay commented 1 year ago

to confirm the csproj and sln files are in that '/builds/demo' folder?

It runs on a GitLab pipeline. The source code is in a subfolder src where the path is changed to before script / execution of MegaLinter.

sm-quasar commented 1 year ago

to confirm the csproj and sln files are in that '/builds/demo' folder?

It runs on a GitLab pipeline. The source code is in a subfolder src where the path is changed to before script / execution of MegaLinter.

In your error message, it seems to be specifically indicating it's looked at the '/builds/demo' folder not the 'builds/demo/src' folder which is probably why it cannot find the csproj or sln

The reason this used to work for you is that prior to be integrated to the dotnet sdk, dotnet format was happy to not need a csproj or sln, and would just recursively look for .cs files. The .NET 6.0 version now requires a .sln or .csproj which is breaking various workflows.

You mention the path is changed to that 'src' subfolder but the error message provided doesn't seem to be searching the path so I suspect however you're running megalinter it is not listening to that path change. How are you changing the path? Are you using the DEFAULT_WORKSPACE config value in your '.megalinter.yml' file?

@Kurt-von-Laven @echoix @nvuillam IMO I think the issue caused by this upgrade to .net 6.0 warrants a breaking change version increment. It is by definition breaking the API since previous functionality won't work in a variety of scenarios without intervention by users of the action

nvuillam commented 1 year ago

I get your point but we can't release a new MegaLinter version everytime one of the 100 linters adds a breaking change, else we'd release 50 major versions every year :/

We realease a major version when there is a breaking change in .mega-linter.yml configuration

But I agree that we need to do something about dotnet issues... if csproj os in a sub folder, maybe search for it and change the current directory before calling dotnet format?

echoix commented 1 year ago

@sm-quasar Since I didn't use dotnet-format when it was separate from the dotnet sdk (or the .net 5 era), could you help me by telling what did that linter do? Was it simply whitespace checks? Now, I know that dotnet format has to build with MSBuild to do its thing, so that why we can't have a standalone tool for dotnet format for the latest versions, it really needs the SDK. Or else we could have tried to reduce the image by not needing the SDK (now, dotnet format needs about 90% of all the contents of the SDK, I don't remember with upstream issue I read that).

If before, it was really only that, and not really advanced checks, I tried out and the closest to the old behaviour I found was dotnet format whitespace --folder --exclude / --include ./csharp_bad_02.cs instead of dotnet-format --folder --exclude / --include ./csharp_bad_02.cs So the dotnet format whitespace seems to work.

But not dotnet format style, dotnet format analyzers nor dotnet format. https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-format

Kurt-von-Laven commented 1 year ago

dotnet format does more than whitespace checks. It is highly configurable and checks a wide range of formatting issues. One approach might be to remove dotnet format altogether. I suspect it is a major contributor to the image size, and it is certainly the slowest linter in the dotnet image by far in our experience, which gives the inaccurate impression that MegaLinter is very slow in general. I am curious what our users want though after giving CSharpier a try.

echoix commented 1 year ago

One approach might be to remove dotnet format altogether.

I'm not against that for the vast majority of cases. Like I don't think the main Megalinter image benefits from it, from a maintenance side.

I suspect it is a major contributor to the image size, and it is certainly the slowest linter in the dotnet image by far in our experience, which gives the inaccurate impression that MegaLinter is very slow in general.

But maybe the dotnet specific flavor is still a good candidate to contain it, where the size increase might justify it.

As a quick comparaison, I looked up the sizes of the dotnet/sdk and dotnet/runtime for alpine, and the sdk images are 228 MB vs 36.6 MB. I don't know why but I had more like a 700-800 MB difference in head from my previous investigations on the megalinter sizes.

echoix commented 1 year ago

to confirm the csproj and sln files are in that '/builds/demo' folder?

It runs on a GitLab pipeline. The source code is in a subfolder src where the path is changed to before script / execution of MegaLinter.

In your error message, it seems to be specifically indicating it's looked at the '/builds/demo' folder not the 'builds/demo/src' folder which is probably why it cannot find the csproj or sln

@Kurt-von-Laven Is it common for people that were using dotnet-format previously, that would maybe have .sln or .csproj or maybe not, but that have some megalinter configuration errors? Does the 6.18 default behaviour assume that a .sln or .csproj are in the root of the repo being checked? If yes, and the error is that the bad root of scanning is set as configuration variable, could we write out a clue in the output that we found .sln/.csproj files in X folder, and the configuration should be Y?

Also, how does it work when there are multiple .sln files? Does each folder tree get scanned? Do we treat the dotnet-format linter as a linter that "fixes" .sln files, and we call the .sln files as what to lint?

jokay commented 1 year ago

You mention the path is changed to that 'src' subfolder but the error message provided doesn't seem to be searching the path so I suspect however you're running megalinter it is not listening to that path change. How are you changing the path? Are you using the DEFAULT_WORKSPACE config value in your '.megalinter.yml' file?

It's set to DEFAULT_WORKSPACE: ${CI_PROJECT_DIR}

variables:
  BASE_PATH: "src"
  NUGET_PACKAGES_DIRECTORY: ".nuget"

stages:
  - lint

before_script:
  - cd ${BASE_PATH}
  - dotnet restore --packages ${NUGET_PACKAGES_DIRECTORY}

megalinter:
  stage: lint
  image: megalinter-dotnet:v6.17.0
  script: ["true"]
  variables:
    APPLY_FIXES: "none"
    DEFAULT_WORKSPACE: ${CI_PROJECT_DIR}
    CSHARP_DOTNET_FORMAT_DISABLE_ERRORS: "false"
    FILTER_REGEX_EXCLUDE: .*.gitlab/.*|.*${NUGET_PACKAGES_DIRECTORY}/.*
    LINTER_RULES_PATH: .gitlab/linters
    LOG_LEVEL: WARN
    VALIDATE_ALL_CODEBASE: "true"

File structure looks like this:

root
\_ .gitlab-ci.yml
\_ src
   \_ demo
      \_ demo.csproj
   \_ demo.tests
      \_ demo.tests.csproj
   \_ demo.sln
Kurt-von-Laven commented 1 year ago

But maybe the dotnet specific flavor is still a good candidate to contain it, where the size increase might justify it.

The main drawback I see to including dotnet format in the dotnet flavor but not the "all" flavor is that losing linters when switching from a flavor to the all flavor seems like a bad and counter-intuitive user experience that could easily go unnoticed in light of how many linters we have. (I admit though that dotnet format specifically is so slow that its absence may be more glaring, and of all the linters to consider excluding from "all," it would be the top candidate.) For these reasons and the fact that we don't get the maintenance savings unless dotnet format is completely removed, I lean towards removing it from both.

As a quick comparaison, I looked up the sizes of the dotnet/sdk and dotnet/runtime for alpine, and the sdk images are 228 MB vs 36.6 MB. I don't know why but I had more like a 700-800 MB difference in head from my previous investigations on the megalinter sizes.

My impression of dotnet format's size comes from the fact that the other flavors we use are all around 2 GB, whereas the dotnet flavor is 3. I don't know exactly where that's coming from beyond the packages you investigated, but 700-800 MB total seems more plausible than 265 MB.

@Kurt-von-Laven Is it common for people that were using dotnet-format previously, that would maybe have .sln or .csproj or maybe not, but that have some megalinter configuration errors?

Is your question whether it's common for dotnet-format to fail to find the .sln/.csproj when run via MegaLinter v6.18.0?

Does the 6.18 default behaviour assume that a .sln or .csproj are in the root of the repo being checked? If yes, and the error is that the bad root of scanning is set as configuration variable, could we write out a clue in the output that we found .sln/.csproj files in X folder, and the configuration should be Y?

No, neither the .sln nor .csproj have to be in the repository root.

Also, how does it work when there are multiple .sln files? Does each folder tree get scanned? Do we treat the dotnet-format linter as a linter that "fixes" .sln files, and we call the .sln files as what to lint?

I think each subtree gets scanned, but if someone happens to know of or have a repo with multiple .sln files that this experiment can be conducted on, I'm all ears.

echoix commented 1 year ago

But maybe the dotnet specific flavor is still a good candidate to contain it, where the size increase might justify it.

The main drawback I see to including dotnet format in the dotnet flavor but not the "all" flavor is that losing linters when switching from a flavor to the all flavor seems like a bad and counter-intuitive user experience that could easily go unnoticed in light of how many linters we have. (I admit though that dotnet format specifically is so slow that its absence may be more glaring, and of all the linters to consider excluding from "all," it would be the top candidate.) For these reasons and the fact that we don't get the maintenance savings unless dotnet format is completely removed, I lean towards removing it from both.

So a MegaLinter 7 thing.

@Kurt-von-Laven Is it common for people that were using dotnet-format previously, that would maybe have .sln or .csproj or maybe not, but that have some megalinter configuration errors?

Is your question whether it's common for dotnet-format to fail to find the .sln/.csproj when run via MegaLinter v6.18.0?

Yes, I think it's better worded this way.

Does the 6.18 default behaviour assume that a .sln or .csproj are in the root of the repo being checked? If yes, and the error is that the bad root of scanning is set as configuration variable, could we write out a clue in the output that we found .sln/.csproj files in X folder, and the configuration should be Y?

No, neither the .sln nor .csproj have to be in the repository root.

That's interesting to document here.

I think each subtree gets scanned, but if someone happens to know of or have a repo with multiple .sln files that this experiment can be conducted on, I'm all ears.

It's not the repo I was thinking of, but that it's a structure that I sometimes see. Some code in a src folder, and examples in another one: two .sln, and one doesn't contain all the csharp code to scan https://github.com/CommunityToolkit/Maui

jokay commented 1 year ago

CSharpier is opinionated and so has very few config options. However it's opinion is by and large inline with ms standards and so it's working pretty well for me

Ok, you convinced me.

We disabled dotnet format and are using CSharpier only.

Amazing how much faster it is.

nvuillam commented 1 year ago

So... dear dotnet experts... should we disable dotnet format by default? ^^ What do we do to close this topic ? :)

Note: it is also used for VB .net... but are still people using this language?

nvuillam commented 1 year ago

cc @bdovaz

bdovaz commented 1 year ago

As I said I have no experience with 'dotnet format` from Megalinter but I do have experience using it from outside Megalinter.

dotnet format is intended for parsing .NET solution or project files. More information for those who don't know what we are talking about:

https://learn.microsoft.com/en-us/visualstudio/extensibility/internals/solution-dot-sln-file

https://learn.microsoft.com/en-us/visualstudio/extensibility/internals/projects

It does not work like other linters that directly scan source files (in this case .cs), but with solutions or projects that host these source files.

In the following script you can see how it looks for .sln or .*proj files:

https://github.com/dotnet/format/blob/v5.1.225507/src/Workspaces/MSBuildWorkspaceFinder.cs#L25

And here it talks a bit about different project types (.csproj, .vbproj, .dbproj, ...):

https://learn.microsoft.com/en-us/aspnet/web-forms/overview/deployment/web-deployment-in-the-enterprise/understanding-the-project-file#msbuild-and-the-project-file

If we want to integrate the linter in the best possible way and that the speed of the linter is adequate, we must use it as it was originally created, and it is to scan solutions (.sln) or projects (.*proj).

Of course we must allow these solutions or projects to be at different levels of the workspace. In the following example you can see how each folder is equivalent to a solution (.sln) with its different C# projects (.csproj):

https://github.com/dotnet/runtime/tree/main/src/libraries

bdovaz commented 1 year ago

In fact, the linter I want to introduce #2269 is also based on analyzing solutions and projects so we would have the same problem again.

https://github.com/JosefPihrt/Roslynator/blob/main/docs/cli/analyze-command.md

jokay commented 1 year ago

but are still people using this language?

Not sure if someone still using VB.NET is interested in linting code at all :wink:

bdovaz commented 1 year ago

@nvuillam @echoix @Kurt-von-Laven what do you think about what I said? https://github.com/oxsecurity/megalinter/issues/2248#issuecomment-1399013833

I need your opinion to make a PR to modify this behavior in the dotnet format linter and to start with the Roslynator PR which will also be affected by this problem.

nvuillam commented 1 year ago

@bdovaz this is chinese for me as I don't know dotnet, if you agree together, I'll agree with you haha :)

Roslynator can run with the files in the repo, no need to have strange files that are not committed ? If yes, that would be great :)

bdovaz commented 1 year ago

@nvuillam by "strange files" you mean the sln and csproj files we mentioned? They are necessary but the OP wanted to try to analyze the source files without them. As I told him https://github.com/oxsecurity/megalinter/issues/2248#issuecomment-1381037301, there is a method in Unity to force to generate those files and that is the correct way to do it.

And about Roslynator, as I have already said, it is necessary to do it through the sln or csproj files.... There is no way to indicate a folder with files or a list of files.

TommyE123 commented 1 year ago

Hi @bdovaz,

I’ve been following this thread with interest as we currently are now experiencing random issues and dramatic slow down in our .net repositories when running megalinter over them with dotnet-format enabled. We have a number of small and large legacy repositories that still need to be maintained which are a mixture of C# and VB.bet with different levels of sln and proj files unfortunately. Can I ask how you see it working when you make the changes?

Will you only scan each proj file once regardless of how many files have changed in that proj? If I’m understanding how it works correctly it scans all files regardless of which one has changed in that project?

How do you suggest we scan sln files? Does it add a big overhead to performance or should we only scan sln files where we have scanned a proj file linked to it?! (Could get messy if you then started going back down to other proj files!)

Do we need to consider the VALIDATE_ALL_CODEBASE here as you could, I’m thinking, search all sln and proj files and pass them one at a time if true? I refer to this here when I also made some points on this. https://github.com/oxsecurity/megalinter/issues/2249 What I didn’t mention at the time was it was creating the same warnings over and over again for the same files so I’m guessing its calling the same proj file and scanning it again for every file changed?

Either way I’m guessing you will need to pass a workspacePath argument as this is obviously an automated process and a user can’t specify manually?

Sorry just my thoughts and mumblings if its helps to bounce some ideas off. Sorry if I’ve misunderstood or this has already been explained somewhere.

Tom

bdovaz commented 1 year ago

@TommyE123 the reality is that both dotnet format and Roslynator (not yet integrated) only work through sln/csproj files and not like other linters to which you pass lists of source code files.

By this I mean that I see complicated to optimize the fact that it only analyzes the projects/solutions that have changed files and not all of them, since there is no easy way to relate which files belong to which projects, etc...

What I don't know is how it would work (@nvuillam ???) with VALIDATE_ALL_CODEBASE because if it is set to false and in a specific commit only a .cs file has been changed and not the .sln (file that hardly suffers modifications), it will not run the analysis because the .sln file has not been modified and it is the input of the linter.

Kurt-von-Laven commented 1 year ago

But maybe the dotnet specific flavor is still a good candidate to contain it, where the size increase might justify it.

The main drawback I see to including dotnet format in the dotnet flavor but not the "all" flavor is that losing linters when switching from a flavor to the all flavor seems like a bad and counter-intuitive user experience that could easily go unnoticed in light of how many linters we have. (I admit though that dotnet format specifically is so slow that its absence may be more glaring, and of all the linters to consider excluding from "all," it would be the top candidate.) For these reasons and the fact that we don't get the maintenance savings unless dotnet format is completely removed, I lean towards removing it from both.

So a MegaLinter 7 thing.

Yeah, I agree that removing dotnet format would have to wait until v7.

@Kurt-von-Laven Is it common for people that were using dotnet-format previously, that would maybe have .sln or .csproj or maybe not, but that have some megalinter configuration errors?

Is your question whether it's common for dotnet-format to fail to find the .sln/.csproj when run via MegaLinter v6.18.0?

Yes, I think it's better worded this way.

I suspect so because of this issue and the failed experiment below, but it's difficult to know since we don't have telemetry (not that I'm advocating for it).

I think each subtree gets scanned, but if someone happens to know of or have a repo with multiple .sln files that this experiment can be conducted on, I'm all ears.

It's not the repo I was thinking of, but that it's a structure that I sometimes see. Some code in a src folder, and examples in another one: two .sln, and one doesn't contain all the csharp code to scan https://github.com/CommunityToolkit/Maui

It didn't work out of the box at least. I reproduced one of the many similar error messages below.

❌ [ERROR] samples/CommunityToolkit.Maui.Sample/App.xaml.cs
    Unhandled exception: System.IO.FileNotFoundException: Could not find a MSBuild project file or solution file in '/tmp/lint'. Specify which to use wit
h the <workspace> argument.
       at Microsoft.CodeAnalysis.Tools.Workspaces.MSBuildWorkspaceFinder.FindWorkspace(String searchDirectory, String workspacePath)
       at Microsoft.CodeAnalysis.Tools.Workspaces.MSBuildWorkspaceFinder.FindWorkspace(String searchDirectory, String workspacePath)
       at Microsoft.CodeAnalysis.Tools.FormatCommandCommon.ParseWorkspaceOptions(ParseResult parseResult, FormatOptions formatOptions)
       at Microsoft.CodeAnalysis.Tools.Commands.RootFormatCommand.FormatCommandDefaultHandler.InvokeAsync(InvocationContext context)
       at System.CommandLine.Invocation.InvocationPipeline.<>c__DisplayClass4_0.<<BuildInvocationChain>b__0>d.MoveNext()
    --- End of stack trace from previous location ---
       at System.CommandLine.CommandLineBuilderExtensions.<>c__DisplayClass16_0.<<UseParseErrorReporting>b__0>d.MoveNext()
    --- End of stack trace from previous location ---
       at System.CommandLine.CommandLineBuilderExtensions.<>c__DisplayClass11_0.<<UseHelp>b__0>d.MoveNext()
    --- End of stack trace from previous location ---
       at System.CommandLine.CommandLineBuilderExtensions.<>c__DisplayClass21_0.<<UseVersionOption>b__0>d.MoveNext()
    --- End of stack trace from previous location ---
       at System.CommandLine.CommandLineBuilderExtensions.<>c__DisplayClass18_0.<<UseTypoCorrections>b__0>d.MoveNext()
    --- End of stack trace from previous location ---
       at System.CommandLine.CommandLineBuilderExtensions.<>c.<<UseSuggestDirective>b__17_0>d.MoveNext()
    --- End of stack trace from previous location ---
       at System.CommandLine.CommandLineBuilderExtensions.<>c__DisplayClass15_0.<<UseParseDirective>b__0>d.MoveNext()
    --- End of stack trace from previous location ---
       at System.CommandLine.CommandLineBuilderExtensions.<>c.<<RegisterWithDotnetSuggest>b__4_0>d.MoveNext()
    --- End of stack trace from previous location ---
       at System.CommandLine.CommandLineBuilderExtensions.<>c__DisplayClass7_0.<<UseExceptionHandler>b__0>d.MoveNext()

If we want to integrate the linter in the best possible way and that the speed of the linter is adequate, we must use it as it was originally created, and it is to scan solutions (.sln) or projects (.*proj).

Of course we must allow these solutions or projects to be at different levels of the workspace. In the following example you can see how each folder is equivalent to a solution (.sln) with its different C# projects (.csproj):

https://github.com/dotnet/runtime/tree/main/src/libraries

Good point! Yes, this sounds like the best path forward, particularly since, as @bdovaz pointed out, we will need this feature for Roslynator anyways. It's nice that this can be done in v6, and we can revisit whether to remove dotnet format when upgrading to v7 in my opinion. I think it may be possible to achieve what you are proposing with file_names_regex and/or active_only_if_file_found. Worst case, we may need a custom Python class (e.g., DotnetProject).

For the first iteration, I feel we should focus on getting it to work optimally with VALIDATE_ALL_CODEBASE: true. I think it's a worthwhile sacrifice if dotnet format and Roslynator often get skipped when VALIDATE_ALL_CODEBASE: false. We already deliberately exclude all project linters from the megalinter-incremental pre-commit hook since project linters inherently aren't incremental in nature. Maybe a subsequent iteration could run dotnet format and Rolsynator on the modified projects when VALIDATE_ALL_CODEBASE: false with a custom Python class, but I'm not sure this is even necessarily desirable for most projects. After all, the point of incremental runs is that they're supposed to be significantly faster, and if someone is willing to wait for a slower run, they can just use VALIDATE_ALL_CODEBASE: true. Curious what others think though.

echoix commented 1 year ago

Of course we must allow these solutions or projects to be at different levels of the workspace. In the following example you can see how each folder is equivalent to a solution (.sln) with its different C# projects (.csproj):

https://github.com/dotnet/runtime/tree/main/src/libraries

Another common pattern is shared projects, where it is more like a "code bucket" and has no code, but it groups files together that can be included in the compilation of another project.

You can see a small one here :

https://github.com/CommunityToolkit/dotnet/tree/main/src/CommunityToolkit.Mvvm.SourceGenerators.Roslyn431

Someone correct me, it's either UWP projects or packaging projects that use this pattern all the time. So a csproj is not the root folder of the code files it contains. This shouldn't be a problem if dotnet-format is given only the project files like now.

Kurt-von-Laven commented 1 year ago

Another common pattern is shared projects, where it is more like a "code bucket" and has no code, but it groups files together that can be included in the compilation of another project.

Good to know; that seems like a good test case.

Someone correct me, it's either UWP projects or packaging projects that use this pattern all the time. So a csproj is not the root folder of the code files it contains. This shouldn't be a problem if dotnet-format is given only the project files like now.

I'm not sure, but at least in our UWP project, the .csproj is in the root directory of the code files it contains.

bdovaz commented 1 year ago

Ok, seeing that we agree that dotnet format and Roslynator linters are designed to scan *.sln files and not *.cs source files, we are going to encounter the problem of wanting to use it in VALIDATE_ALL_CODEBASE: false mode because as you said, there are many cases.

A project file (*.csproj) does not have to have hanging below it all the source files (*.cs) can be at different levels of the hierarchy. By default, the *.csproj in the new format (sdk style) adds all *.cs files that are at the same level or below but then within the csproj file you can explicitly exclude files, folders, etc or add new ones relative to other levels such as folders that are above the csproj file.

I don't know what you think @Kurt-von-Laven @echoix but the way I see it, at least these 2 linters would have to ignore the value of VALIDATE_ALL_CODEBASE and look up all *.sln files and run the linters regardless of whether there have been changes or not. And indicate in the documentation that these linters are not affected by this parameter.

I also have my doubts that with the current architecture you can ignore or force to different modes depending on the linter, the value of VALIDATE_ALL_CODEBASE. That answer should be given by @nvuillam.

nvuillam commented 1 year ago

About linters that can not handle file or list_of_files mode, it's normal that the value of VALIDATE_ALL_CODEBASE is ignored :)