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

Update dotnet linters to .NET 7 #2402

Merged
merged 7 commits into from
Dec 4, 2023
Merged

Update dotnet linters to .NET 7 #2402

merged 7 commits into from
Dec 4, 2023

Conversation

bdovaz
Copy link
Collaborator

@bdovaz bdovaz commented Feb 28, 2023

Upgrade dotnet linters to .NET 7 whenever possible.

Unfortunately we can't install only the .NET 7 SDK, we have to keep the .NET 6 one for these linters that don't have .NET 7 compatibility yet:

https://www.nuget.org/packages/TSQLLint#supportedframeworks-body-tab

https://www.nuget.org/packages/Microsoft.CST.DevSkim.CLI#supportedframeworks-body-tab

@bdovaz
Copy link
Collaborator Author

bdovaz commented Feb 28, 2023

@nvuillam @echoix what do you think? Do we keep this PR indefinitely open until every dotnet linter has .NET 7 compatibility or merge it with the hassle of having to have 2 SDKs coexisting at the same time?

@nvuillam
Copy link
Member

nvuillam commented Feb 28, 2023

🦙 MegaLinter status: ⚠️ WARNING

Descriptor Linter Files Fixed Errors Elapsed time
✅ BASH bash-exec 6 0 0.01s
✅ BASH shellcheck 6 0 0.14s
✅ BASH shfmt 6 0 0 0.39s
✅ COPYPASTE jscpd yes no 2.71s
✅ DOCKERFILE hadolint 114 0 18.08s
✅ JSON eslint-plugin-jsonc 21 0 0 2.28s
✅ JSON jsonlint 19 0 0.25s
✅ JSON v8r 21 0 13.77s
⚠️ MARKDOWN markdownlint 309 0 230 7.05s
✅ MARKDOWN markdown-link-check 309 0 5.48s
✅ MARKDOWN markdown-table-formatter 309 0 0 15.62s
✅ OPENAPI spectral 1 0 1.75s
⚠️ PYTHON bandit 183 47 2.17s
✅ PYTHON black 183 0 0 4.35s
✅ PYTHON flake8 183 0 1.99s
✅ PYTHON isort 183 0 0 1.58s
✅ PYTHON mypy 183 0 7.24s
✅ PYTHON pylint 183 0 12.14s
⚠️ PYTHON pyright 183 254 17.83s
✅ REPOSITORY checkov yes no 31.6s
✅ REPOSITORY git_diff yes no 0.38s
✅ REPOSITORY secretlint yes no 17.26s
✅ REPOSITORY trivy yes no 26.34s
✅ SPELL cspell 745 0 20.16s
✅ SPELL misspell 566 0 0 0.93s
✅ XML xmllint 3 0 0 0.39s
✅ YAML prettier 81 0 0 3.17s
✅ YAML v8r 23 0 63.43s
✅ YAML yamllint 82 0 1.14s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by OX Security

@nvuillam
Copy link
Member

I'm still lost when we talk about dotnet, I let the professionals give advices ^^

@Kurt-von-Laven
Copy link
Collaborator

I'm guessing the .NET Docker image is noticeably bigger with both SDKs installed? Might help guide the decision to quantify how much bigger.

@nvuillam
Copy link
Member

Wouldn't be good to add 400 megas to the main docker image, indeed ^^

@nvuillam
Copy link
Member

🦙 MegaLinter status: ⚠️ WARNING

Descriptor Linter Files Fixed Errors Elapsed time
✅ BASH bash-exec 6 0 0.01s
✅ BASH shellcheck 6 0 0.16s
✅ BASH shfmt 6 0 0 0.05s
✅ COPYPASTE jscpd yes no 3.44s
✅ DOCKERFILE hadolint 114 0 18.05s
✅ JSON eslint-plugin-jsonc 21 0 0 2.21s
✅ JSON jsonlint 19 0 0.27s
✅ JSON npm-package-json-lint yes no 0.79s
✅ JSON v8r 21 0 14.1s
⚠️ MARKDOWN markdownlint 309 2 230 7.27s
✅ MARKDOWN markdown-link-check 309 0 6.53s
✅ MARKDOWN markdown-table-formatter 309 2 0 21.62s
✅ OPENAPI spectral 1 0 2.0s
⚠️ PYTHON bandit 183 47 2.54s
✅ PYTHON black 183 0 0 3.99s
✅ PYTHON flake8 183 0 2.12s
✅ PYTHON isort 183 0 0 0.5s
✅ PYTHON mypy 183 0 8.44s
✅ PYTHON pylint 183 0 13.49s
⚠️ PYTHON pyright 183 251 19.59s
✅ REPOSITORY checkov yes no 35.25s
⚠️ REPOSITORY devskim yes 61 1.53s
✅ REPOSITORY dustilock yes no 2.24s
✅ REPOSITORY git_diff yes no 0.05s
✅ REPOSITORY secretlint yes no 9.57s
✅ REPOSITORY syft yes no 1.15s
✅ REPOSITORY trivy yes no 24.41s
✅ SPELL cspell 745 0 23.09s
✅ SPELL misspell 566 2 0 0.68s
✅ XML xmllint 3 0 0 0.03s
✅ YAML prettier 81 0 0 3.08s
✅ YAML v8r 23 0 65.63s
✅ YAML yamllint 82 0 1.38s

See detailed report in MegaLinter reports

You could have same capabilities but better runtime performances if you request a new MegaLinter flavor.

MegaLinter is graciously provided by OX Security

@bdovaz
Copy link
Collaborator Author

bdovaz commented Mar 6, 2023

I have taken some time to build the flavor of dotnet in the 2 branches (main and features/net7) and the differences are remarkable:

image

So I understand that we will have to wait until all the dotnet linters are available for .net7... 😞

@bdovaz bdovaz added the nostale This issue or pull request is not stale, keep it open label Mar 6, 2023
@Kurt-von-Laven
Copy link
Collaborator

Or we can convince Microsoft to reduce the size of the SDK!

@echoix
Copy link
Collaborator

echoix commented Mar 6, 2023

I think they reduced to a minimum on .net 5 or .net 6, but its really hard to go less. And it's cumulative, they can't take away stuff. Only add.

For the increased size, is it because you have two SDKs? (And do you remember, if what you shown was the compressed or uncompressed size?)

@bdovaz
Copy link
Collaborator Author

bdovaz commented Mar 6, 2023

@echoix I have only made a flavors/dotnet/Dockerfile build in each branch and I have run locally docker images (screenshot).

If that is not enough, give me some guidelines please. I am not a specialist in docker commands 😅

Thanks!

And yes, the size increase is because it has .NET 6 / 7 SDK at the same time.

@Kurt-von-Laven
Copy link
Collaborator

I was just kidding, but that's helpful to know. In case anyone reading this thread in the future is confused, the issue is that we can't get rid of the .NET 6 SDK until all tools are on .NET 7. Those are uncompressed image sizes according to this helpful guide. The v6.19.0 dotnet flavor is 3.31 GB uncompressed for comparison. It compresses down to 1.0 GB when using docker-cache, which uses zstd compression. That probably isn't the compressed size you were referring to though.

@Kurt-von-Laven
Copy link
Collaborator

Kurt-von-Laven commented Mar 12, 2023

In the case of MegaLinter flavors, the zstd compression we are using produces compressed sizes of about 88% of the standard compressed sizes I assume you were referring to @echoix.

@echoix
Copy link
Collaborator

echoix commented Oct 14, 2023

We might need to plan soon about .NET 8.
It will be released near November 14, 2023. It will be a LTS release. It is now in "go-live", with its 8.0.0-RC2 release being the latest one. I suggest another PR for that work. I don't think we need to be the first to have it, but probably have our linters support it first.

@bdovaz
Copy link
Collaborator Author

bdovaz commented Oct 15, 2023

@echoix of course, we will have to consider upgrading to .NET 8 LTS.

And to put some context on this PR.

DevSkim supports .NET 7: https://www.nuget.org/packages/Microsoft.CST.DevSkim.CLI#supportedframeworks-body-tab

TSQLLint does not support it (only .NET 6): https://www.nuget.org/packages/TSQLLint#supportedframeworks-body-tab

That's why I haven't been able so far to merge anything.

It's a bit frustrating because in that repo I created a PR (tsqllint/tsqllint#324) but nobody has been able to communicate with me why it hasn't been merged, I don't know if it's that it only has LTS support policy (.NET 7 is not) or it's that the project is half abandoned (in this year there are 3 commits).

@Kurt-von-Laven
Copy link
Collaborator

Given that there is a high cost in disk space (at least) to shipping multiple versions of .NET, should we consider removing TSQLLint?

@nvuillam
Copy link
Member

About removing linters. I think about DevSkim, who is very chatty, triggers strange git errors, and it historically not very relevant compared to the other linters ...

Maybe we should add some anonymous telemetry to know which linters are used and which ones are not, so we know the impact of removing one ?

@echoix
Copy link
Collaborator

echoix commented Oct 16, 2023

Well, for the specific case of tsqllint I'm not too concerned, since it can be easily used standalone, like a 40 mb tgz download. It doesn't require to be used as a dotnet tool. They even have a package on npm to call it and the install.js simply downloads the archive and extracts it. So we are able to use it on its own. Since the npm way of "using" it doesn't talk about having .net installed, I assume the releases are compiled as "self-contained", that means that the runtime shouldn't be needed. In the other case we could always compile ourselves and cache it.

For me it's the one that is really tightly integrated with the dotnet dev sdk, I think it's dotnet format. Does it require that all projects that will be linted to be a .NET project too?

What other linters are written in a dotnet language? (Not necessarily the ones that are in the dotnet flavor)

@Kurt-von-Laven
Copy link
Collaborator

Well, for the specific case of tsqllint I'm not too concerned, since it can be easily used standalone, like a 40 mb tgz download. It doesn't require to be used as a dotnet tool. They even have a package on npm to call it and the install.js simply downloads the archive and extracts it. So we are able to use it on its own. Since the npm way of "using" it doesn't talk about having .net installed, I assume the releases are compiled as "self-contained", that means that the runtime shouldn't be needed. In the other case we could always compile ourselves and cache it.

I think you're right that TSQLLint, while written in C#, does not depend on .NET.

For me it's the one that is really tightly integrated with the dotnet dev sdk, I think it's dotnet format. Does it require that all projects that will be linted to be a .NET project too?

I have never experimented with this, but you might be able to abuse --include to run dotnet-format on a non-.NET project. I'm skeptical that this is a significant use case we should account for though. I've never heard of anyone running dotnet-format on non-.NET projects.

What other linters are written in a dotnet language?

I believe it's just CSharpier, PSScriptAnalyzer, dotnet-format, and TSQLLint that are written in C#, and the first two use .NET. dotnet-format is literally part of the .NET SDK.

@nvuillam
Copy link
Member

Would this PR deserve to be revived ? :)

@echoix
Copy link
Collaborator

echoix commented Oct 29, 2023

I think if it would be, it would be for .Net 8. And maybe want to only have the dotnet-format only to be run like a dotnet tool. Thus we could decouple the .net sdk version from the tools.

All of this assuming that a project must already be upgraded to .NET 8 to work with a new release. That's something that needs to be checked

@Kurt-von-Laven
Copy link
Collaborator

I know CSharpier can be run as a dotnet tool, but it does still require a compatible version of the .NET SDK to be installed.

@bdovaz
Copy link
Collaborator Author

bdovaz commented Oct 31, 2023

Well, for the specific case of tsqllint I'm not too concerned, since it can be easily used standalone, like a 40 mb tgz download. It doesn't require to be used as a dotnet tool. They even have a package on npm to call it and the install.js simply downloads the archive and extracts it. So we are able to use it on its own. Since the npm way of "using" it doesn't talk about having .net installed, I assume the releases are compiled as "self-contained", that means that the runtime shouldn't be needed. In the other case we could always compile ourselves and cache it.

For me it's the one that is really tightly integrated with the dotnet dev sdk, I think it's dotnet format. Does it require that all projects that will be linted to be a .NET project too?

What other linters are written in a dotnet language? (Not necessarily the ones that are in the dotnet flavor)

I just tested it but the problem created by @nvuillam is still valid....

tsqllint/tsqllint#267

I have tried what @jberkers42 mentions of adding with apk gcompat, libunwind and libuuid but it keeps failing me....

In general I see that this repository is half abandoned because my attempts to update it to .NET 7 are still stopped since February... I have tried to mention @nathan-boyd several times in that PR with no result....

tsqllint/tsqllint#324

@bdovaz
Copy link
Collaborator Author

bdovaz commented Oct 31, 2023

I have created another PR that tries to solve what @jberkers42 mentioned about RIDs

tsqllint/tsqllint#330

@Kurt-von-Laven
Copy link
Collaborator

Personally, I am all for trimming the least maintained/highest false positive linters from our dependencies. Anyone who wants to keep running them can do so with relatively little effort, and we can probably deliver the best experience to our ecosystem if we don't have to spend disproportionate effort patching up/working around/waiting on such linters.

Maybe we should add some anonymous telemetry to know which linters are used and which ones are not, so we know the impact of removing one?

Sorry, I missed this question, @nvuillam. As the bulk of my career historically has been dedicated to the development of ethical anonymous telemetry, I would certainly never object to having more insight into patterns of use. I also don't feel telemetry is entirely necessary to answer this specific question of whether to keep TSQLLint. Regardless of their usage patterns, it's to be expected that any tool that attempts to aggregate many others may occasionally make strategic deprecations of some as their popularities' ebb and flow. The average lifespan of an open-source project in days tends to be approximately the same as the number of files in its repository. If we were to track something, the best metric I have thought of would be the counts of explicitly disabled linters. The number of unique users of each flavor might also be helpful if we don't already have access to that, but I don't have as concrete of a use case for that personally. As much as I love them, there are several major obstacles to making a data-driven decision here:

  1. TSQLLint runs on any .sql files by default, and I suspect that very few users will disable it even if it adds no value to them or they don't even use TSQL.

  2. The rate with which a linter is disabled may serve as a proxy for its troublesome-ness to the end user, but less so for its maintenance burden.

  3. Even if we had perfect insight into the value users place on keeping TSQLLint in MegaLinter, that is only one side of the coin. Users have no non-painful option to upgrade MegaLinter to .NET 7 or 8 if they require this support. Some may have already abandoned us because of it without communicating with us, and I haven't thought of any clever ways to use telemetry to shed light on how many users care about this. The best I have come up with is polling the community using GitHub Discussions. I imagine this draft would need to be revised, but for the sake of concreteness, I am imagining something like this:

    Dear MegaLinter + .NET Community,

    We are facing a crossroads in Update dotnet linters to .NET 7 #2402 and request your guidance. As a linter aggregator, we endeavor to avoid removing linters when possible while keeping dependencies up-to-date, and occasionally these goals come in conflict with one another. For example, we have been attempting to upgrade from .NET 6 to .NET 7 or 8, but TSQLLint does not currently support .NET 7+. Which of the following options best reflects the views of you or your team?

    • "We'd rather keep TSQLlint in MegaLinter, and remain on .NET 6 for now."
    • "We'd rather keep TSQLLint in MegaLinter, and increase the uncompressed disk size of the dotnet flavor by 0.4 GB from 3.1 to 3.5 GB by installing .NET 7 alongside .NET 6."
    • "We'd rather run TSQLLint separately if desired, but have MegaLinter support .NET 7 within the next few months."
    • "We'd rather run TSQLLint separately if desired, but have MegaLinter support .NET 8 within the next few months."
    • "We either don't use TSQLLint or don't have an opinion on this matter."

    Please feel free to clarify, propose alternatives, etc. in the comments below. Thank you for using MegaLinter and for helping us make it better.

@jberkers42
Copy link

While I initially suggested the inclusion of tsqllint into MegaLinter, I cannot speak for anyone else regarding how it may be being used, however, our use-case for tsqllint was that the existing SQL linters did not cover the TSQL syntax variant used by Microsoft SQL Server. We found that it had enough variation that the existing ones did not suit our requirements entirely.

In the context of what we are doing with it, the *.sql files are not part of a .NET project, per se. We are using a collection of scripts (pwsh/PowerShell) to automate some ETL of data from a SQL Server instance into ElasticSearch for aggregation of statistical data generated by multiple instances of LogRhythm on separate servers. This allows us central visibility and reporting of key statistical data.

We have also expanded to use this for similar cases where we are doing an ETL, so our use-case is somewhat niche, I would guess. Rather than being part of a software development project that entails stored procedures, database triggers, schema files, etc. among frontend application components, we are using it for very select SQL scripts/statements.

If it were to be removed from MegaLinter it would not be a show stopper, but would be a loss to us nonetheless. We can appreciate that integrations that are themselves not maintained frequently can present their challenges.

@nvuillam
Copy link
Member

nvuillam commented Nov 1, 2023

Thanks to all of you for your valuable inputs !

@Kurt-von-Laven > About telemetry, I'm convinced that with anonymous stats it would help us improve MegaLinter for everyone, for example by diminishing image sizes by removing unused linters, but... would users be ok with that, even if we are very transparent about what info we collect ?
Or maybe we could disable it by default, and at installation ask users if they are ok to collect anonyous usage data, and collect it only if MEGALINTER_TELEMETRY == "true" for example ?

@jberkers42 @Kurt-von-Laven @bdovaz About tsqllint (and dotnet tools in general), from the beginning we kind of wait for all of them to be compliant with the latest .NET version before upgrading it...
I see the last commit is in june, is it really abandonware ? ( @bdovaz doesn't seem to have replies to his PR)
If it is really abandonware, at some point we'll have to choose between:

  • removing it while upgradig to .NET 8 (because we can't force all our users to use .NET 6 and old linter versions for one single linter)
  • hard-fork tsqllint, to maintain it ourselves ? Or maybe find a benevolent maintainer, because we already have so much on our MegaLinter plate ? ^^

@Kurt-von-Laven
Copy link
Collaborator

@Kurt-von-Laven > About telemetry, I'm convinced that with anonymous stats it would help us improve MegaLinter for everyone, for example by diminishing image sizes by removing unused linters, but... would users be ok with that, even if we are very transparent about what info we collect ?

It probably won't come as a surprise to anyone that I am a huge believer in the power of data, and I wholeheartedly agree that it could be leveraged to help us help our community, likely in many more ways than I have yet stopped to consider. The experience of the U.S.-based telemetry teams I have worked on has been that many users and open-source developers initially have a strong negative reaction to any form of telemetry because of the ubiquity with which it is deployed to suit the interests of the product owners at the expense of its users' privacy, informed consent, etc. Making the telemetry opt-in, fully anonymous, and clearly communicating these facts seems likely to address most concerns.

I see the last commit is in june, is it really abandonware ? ( @bdovaz doesn't seem to have replies to his PR)

It's a little tough to know where to draw the line on the continuum, and maybe I am being overly harsh here, but I begin to have doubts about TSQLLint's future given that we are a week shy of .NET 7's first birthday and two weeks shy of .NET 8's launch. I don't have any deeper knowledge of the project though, so I'm all ears.

hard-fork tsqllint, to maintain it ourselves ? Or maybe find a benevolent maintainer, because we already have so much on our MegaLinter plate ? ^^

I know nothing about the current maintainer or their perspective, but if anyone feels inspired to reach out to them to volunteer their time in whatever capacity, they certainly have my thanks. I am not thrilled about maintaining our own dependencies as a long-term strategy. I assume a few of them will inevitably die off every few years, so I expect that would limit what we can offer in the core and our ability to continue to offer the best linter aggregation features and performance. Personally, I prefer keeping appraised of new linters (a benefit we receive for free from our lovely community), and gradually replacing the dying ones with the highest quality upstarts over the years. I think this harmonizes the most efficiently with our expertise and brand and helps us focus. I assume there are already a number of fabulous, well-maintained linters out there that we don't currently distribute, and we would offer more user value per unit of maintenance cost by adding them than by taking ownership of a TSQLLint fork. I have been impressed by how many linters have been added on account of user demand.

@bdovaz
Copy link
Collaborator Author

bdovaz commented Dec 3, 2023

Very recently .NET 8 came out but I don't want to mix PRs, I prefer first to be sure that all the linters work on .NET 7 and when it is merged I will create another PR for .NET 8, since it came out recently we may find some surprises.

@echoix
Copy link
Collaborator

echoix commented Dec 3, 2023

@nvuillam the only thing I see that fails is trivy, shall I exclude it?

image

It also blocks the auto updates PRs, even npm-groovy-lint is one of the linters causing it ;)

@echoix
Copy link
Collaborator

echoix commented Dec 3, 2023

Installing from apk is such a great addition! It seems quite new and will probably mean smaller images and faster building, since it was quite long and sometimes buggy

@nvuillam
Copy link
Member

nvuillam commented Dec 3, 2023

@nvuillam the only thing I see that fails is trivy, shall I exclude it?

image

It also blocks the auto updates PRs, even npm-groovy-lint is one of the linters causing it ;)

Go :)

@bdovaz
Copy link
Collaborator Author

bdovaz commented Dec 3, 2023

@nvuillam the only thing I see that fails is trivy, shall I exclude it?
image

It also blocks the auto updates PRs, even npm-groovy-lint is one of the linters causing it ;)

Go :)

Done!

@bdovaz
Copy link
Collaborator Author

bdovaz commented Dec 3, 2023

@nvuillam ready for review!

@bdovaz bdovaz marked this pull request as ready for review December 3, 2023 23:17
@bdovaz bdovaz changed the title Update some dotnet linters to .NET 7 Update dotnet linters to .NET 7 Dec 3, 2023
@nvuillam
Copy link
Member

nvuillam commented Dec 3, 2023

@bdovaz plz solve CI jobs errors and we're all good, it seems to be great job :)

@bdovaz
Copy link
Collaborator Author

bdovaz commented Dec 3, 2023

@nvuillam I don't know what is going on....

The 4 workflows that start with "MegaLinter" complain about "ModuleNotFoundError: No module named 'megalinter.linters.DotnetLinter'" but that class no longer exists, I have removed it in this PR.

And the remaining "Build & Deploy - DEV" the lychee one is:

image

@echoix
Copy link
Collaborator

echoix commented Dec 4, 2023

The first of the two lychee errors, I started handling them, the guy's blog he changed something and all his pages have shifted by a number, there's already an issue. I also tried to check about the other certificate failures, it seems to be either inside the linter or lychee, since on another project this afternoon, building lychee from the crate and running it (in Gitpod) I got the same thing for some other links. Maybe something of ça-certificate. Or that we need to have a new beta image that uses the python 3.11.6 alpine 3.18.5 instead of the python 3.11.6 alpine 3.18.4 that is the latest working build we had.
So, for now, we have to add ignores for them since we are stuck

@bdovaz
Copy link
Collaborator Author

bdovaz commented Dec 4, 2023

@nvuillam How do I unblock the 2 problems? The one mentioned by @echoix and the one mentioned by me of "module not found"

Copy link
Member

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome :)
Let's have a leap of faith and fix tiny linting issues in a next PR if necessary :)

@nvuillam nvuillam merged commit 1c3420e into main Dec 4, 2023
125 of 130 checks passed
@nvuillam nvuillam deleted the features/net7 branch December 4, 2023 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nostale This issue or pull request is not stale, keep it open
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants