Filesystem usage analyzer #3725

RRode · 2024-11-02T19:04:41Z

Resolves #3643

Implements analyzer that searches for file system usage and suggests IFileSystem usage. Currently checks for usages of File, FileInfo, Directory and DirectoryInfo. No warning is produced, if these are used within IFileSystem implementation.

Analyzer was added to Sentry project and finds usage of file system outside of IFileSystem implementations, which then breaks the build. Usages found in PortablePdbReader.cs, CFunctions.cs and FileDiagnosticLogger.cs.

@bitsandfoxes Is this roughly what you had in mind?

#skip-changelog

bruno-garcia · 2024-11-04T01:51:51Z

This is really awesome and promising. Thank you!

Is there a way to ignore some places where we it would be safe to call from, for example the CFunction one perhaps will be

RRode · 2024-11-04T20:16:31Z

Is there a reason why you could not use #pragma warning disable / enable? IMO, with additional comment, this would make it explicit and well documented why the warning should be ignored. But, if you want, I could add some exceptions to the rule. Just let me know what you need.
Also is it OK that I placed the new projects under src and test? For me the analyzer project feels a bit odd there, since it's not a deliverable. I thought about putting both projects under modules, but that seemed a bit off too, since those are git submodules that get pulled in.
Other thing is, would it make sense to put the analyzer into Directory.Build.props in src, so it is added in all src files? I assume that you want to enforce the warning in those projects as well.

bruno-garcia · 2024-11-04T22:11:35Z

Is there a reason why you could not use #pragma warning disable / enable? IMO, with additional comment, this would make it explicit and well documented why the warning should be ignored. But, if you want, I could add some exceptions to the rule. Just let me know what you need.

That makes sense, #pragma sounds like the right solution

Also is it OK that I placed the new projects under src and test? For me the analyzer project feels a bit odd there, since it's not a deliverable. I thought about putting both projects under modules, but that seemed a bit off too, since those are git submodules that get pulled in.

I also think it's kind of odd, but it's not a big deal. Alternatively it could be a new top level directory: analyzers or build-src I dunno. I think as you did it's fine. But worth adding a README.md on the project directory to clarify this analyizer is only for consumption from Sentry itself, for development of the SDK.

Other thing is, would it make sense to put the analyzer into Directory.Build.props in src, so it is added in all src files? I assume that you want to enforce the warning in those projects as well.

We only really need this on Sentry since this is a requirement coming from Sentry.Unity. So, technically we'd want that analyizer there too.

We'd include that here too: https://github.com/getsentry/sentry-unity/tree/main/src/Sentry.Unity
Using relative paths to the sentry-dotnet submodule

src/Sentry.Analyzers/Resources.resx

bruno-garcia · 2024-11-04T22:13:37Z

src/Sentry/Sentry.csproj

@@ -49,6 +49,10 @@
    <PackageReference Include="System.Reflection.Metadata" Version="5.0.0" />
  </ItemGroup>

+  <ItemGroup>
+    <ProjectReference Include="..\Sentry.Analyzers\Sentry.Analyzers.csproj" OutputItemType="Analyzer" ReferenceOutputAssembly="false" />


Does this need PrivateAssets>all ?

If I understand the docs correctly, it could have even been fine, but I added it anyway. I also added private assets in analyzer references.

Analyzer will only be used internally within SDK, so warning translation is not needed

RRode · 2024-11-06T23:24:20Z

That makes sense, #pragma sounds like the right solution

Can you provide some comments or should I just disable the warning in currently offending files?

I also think it's kind of odd, but it's not a big deal. Alternatively it could be a new top level directory: analyzers or build-src I dunno. I think as you did it's fine. But worth adding a README.md on the project directory to clarify this analyizer is only for consumption from Sentry itself, for development of the SDK.

Then I will keep it simple for now and leave it as it is. I added the README, but was not able to remove the one added via Directory.Build.props to the project, so I hope that is OK.

We only really need this on Sentry since this is a requirement coming from Sentry.Unity. So, technically we'd want that analyizer there too.

We'd include that here too: https://github.com/getsentry/sentry-unity/tree/main/src/Sentry.Unity
Using relative paths to the sentry-dotnet submodule

I guess it makes sense to first merge it here and then add it in the other repo, right?

bruno-garcia

Looks good.

Doesn't seem to be draft anymore, ready for a final pass and merge?

bruno-garcia · 2024-11-10T05:52:28Z

src/Sentry.Analyzers/Sentry.Analyzers.csproj

+
+        <RootNamespace>Sentry.Analyzers</RootNamespace>
+        <AssemblyName>Sentry.Analyzers</AssemblyName>


afaik these default to the project name so are redundant

Suggested change

<RootNamespace>Sentry.Analyzers</RootNamespace>

<AssemblyName>Sentry.Analyzers</AssemblyName>

I removed them

bruno-garcia · 2024-11-10T05:57:28Z

Can you provide some comments or should I just disable the warning in currently offending files?

The current ones can be skipped, thanks

Then I will keep it simple for now and leave it as it is. I added the README, but was not able to remove the one added via Directory.Build.props to the project, so I hope that is OK.

Sounds good

I guess it makes sense to first merge it here and then add it in the other repo, right?

Definitely, that was just a note for reference, doesn't affect this PR. Can be a follow up if you'd like to contribute further 🙏

RRode · 2024-11-10T10:04:31Z

The current ones can be skipped, thanks

I added the pragmas in all the places, but this requires changes to this submodule. If I understand correctly this needs to be merged first to main and then reference in this PR adjusted? I am not used to this submodule workflow, so I hope I am doing it right. 😅

Definitely, that was just a note for reference, doesn't affect this PR. Can be a follow up if you'd like to contribute further 🙏

I will give it a try. 🙂

bruno-garcia

I love this! thank you so much

bruno-garcia · 2024-11-10T17:41:49Z

src/Sentry/Infrastructure/FileDiagnosticLogger.cs

@@ -30,7 +30,10 @@ public FileDiagnosticLogger(string path, bool alsoWriteToConsole = false)
    public FileDiagnosticLogger(string path, SentryLevel minimalLevel, bool alsoWriteToConsole = false)
        : base(minimalLevel)
    {
+        // Allow direct file system usage
+#pragma warning disable SN0001


Ah good point, @bitsandfoxes anything we need to do to make this "Not available on Switch" ?

Edge case since someoen needs to get out of their way to use this and its' just for debugging, but this analyzier really help us make these calls now.

Not doing anything about this (just ignoring it) is fine by me too but we should add a note to the docs that this thing isn't supported on Switch anyway:

https://docs.sentry.io/platforms/unity/configuration/diagnostic-logger/#FileDiagnosticLogger

src/Sentry/Platforms/Native/CFunctions.cs

bruno-garcia · 2024-11-10T17:43:21Z

I'll leave it for @jamescrosswell to do the merging as he's the maintainer but LGTM! 🙏

Improved comment Co-authored-by: Bruno Garcia <[email protected]>

jamescrosswell · 2024-11-12T03:02:46Z

I added the pragmas in all the places, but this requires changes to this submodule. If I understand correctly this needs to be merged first to main and then reference in this PR adjusted? I am not used to this submodule workflow, so I hope I am doing it right. 😅

Hey @RRode, up until now we've been maintaining the Sentry fork of Ben.Demystifier purely for use in the sentry-dotnet repository so the workflows for changing code in there are a bit obscure and not very contributor friendly (the code used by sentry-dotnet isn't in the main branch, for example). I'll do a bit of head scratching to see if we could make this easier.

Worst case, I can cherry pick the change from your PR and get it in there but I'd rather tidy up the submodule so it's easier to accept contributions. Give me a couple days.

jamescrosswell · 2024-11-13T07:37:50Z

Worst case, I can cherry pick the change from your PR and get it in there but I'd rather tidy up the submodule so it's easier to accept contributions. Give me a couple days.

Hi @RRode, I ended up just cherry picking your changes and pushing these up.

RRode · 2024-11-13T21:28:16Z

@jamescrosswell I pushed the missing warning disable for AndroidHelpers

jamescrosswell · 2024-11-14T09:47:39Z

@jamescrosswell I pushed the missing warning disable for AndroidHelpers

Thanks @RRode - apologies in advance if the build breaks on this one. GitHub just changed their macos-14 runners (no longer support XCode 16.0) so we need to bump to macos-15 but that's missing a bunch of stuff we need. Once this is merged, we can pull the changes into this branch and the build should work again:

Pin macos-15 build runner #3754

RRode · 2024-11-14T20:57:46Z

@jamescrosswell no problem. What about this format code check? Is there some doing from my side? If I understand this correctly, git fails to do something with my branch.

jamescrosswell · 2024-11-15T05:17:05Z

@jamescrosswell no problem. What about this format code check? Is there some doing from my side? If I understand this correctly, git fails to do something with my branch.

Hm, possibly it's getting confused because there are multiple remotes defined for the Ben.Demystifier submodule (getsentry/Ben.Demystifier and benadams/Ben.Demystifier). The benadams (upstream) seems to be dead - it hasn't been updated forever and we can't get in touch with Ben so maybe we just remove that remote.

I'll look into it once we've fixed the macos builds.

codecov · 2024-11-15T09:25:28Z

Codecov Report

Attention: Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.

Project coverage is 76.47%. Comparing base (495e742) to head (dde783f).
Report is 377 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Sentry.Analyzers/FileSystemAnalyzer.cs	93.54%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3725      +/-   ##
==========================================
+ Coverage   75.73%   76.47%   +0.74%     
==========================================
  Files         357      388      +31     
  Lines       13466    14196     +730     
  Branches     2671     2857     +186     
==========================================
+ Hits        10198    10857     +659     
- Misses       2593     2642      +49     
- Partials      675      697      +22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

jamescrosswell · 2024-11-15T09:49:40Z

@jamescrosswell no problem. What about this format code check? Is there some doing from my side? If I understand this correctly, git fails to do something with my branch.

Hm, actually I think it might be an issue with scripts/commit-formatted-code.sh:

sentry-dotnet/scripts/commit-formatted-code.sh

Line 13 in 133e1a5

git checkout ${GITHUB_BRANCH}

In your case, the PR is coming from a fork so it's not finding the filesystem-usage-analyzer branch (it needs to checkout RRode:filesystem-usage-analyzer).

I'm in GMT+12 so it's kind of late on a Friday night here, but I'll look into it on the weekend or early next week.

…sentry-dotnet into pr/3725

jamescrosswell · 2024-11-20T08:13:44Z

Finally got it over the line. Thanks for your patience and huge thank you for the PR @RRode !

RRode · 2024-11-20T20:52:52Z

Happy to help! 🙂

Rok Rode added 3 commits November 2, 2024 13:28

Analyzer implementation

5c22791

Add analyzer to solution

645fee1

Use analyzer in Sentry.csproj

e90d924

bruno-garcia reviewed Nov 4, 2024

View reviewed changes

Rok Rode added 3 commits November 6, 2024 23:32

Make analyzer and it's dependencies private

db0d375

Remove localization

5500310

Analyzer will only be used internally within SDK, so warning translation is not needed

Add README.md describing the purpose of analyzer.

b962995

bruno-garcia reviewed Nov 10, 2024

View reviewed changes

Rok Rode added 2 commits November 10, 2024 09:17

Remove redundant properties

8a84d64

Disable direct file system warnings

0583aab

RRode marked this pull request as ready for review November 10, 2024 10:04

RRode requested review from bitsandfoxes and jamescrosswell as code owners November 10, 2024 10:04

bruno-garcia approved these changes Nov 10, 2024

View reviewed changes

Update src/Sentry/Platforms/Native/CFunctions.cs

50e5aa2

Improved comment Co-authored-by: Bruno Garcia <[email protected]>

jamescrosswell mentioned this pull request Nov 13, 2024

Disable file system analyzer warning getsentry/Ben.Demystifier#6

Closed

jamescrosswell added 2 commits November 13, 2024 20:34

Merge branch 'main' into pr/3725

f8ce052

Update Ben.Demystifier

1a480f7

Disable file system warning

868133f

Merge branch 'main' into pr/3725

9b42d99

bruno-garcia mentioned this pull request Nov 15, 2024

fix: Logger argument order getsentry/sentry-unity#1901

Merged

Update FileSystemAnalyzer.cs

34fcd8e

jamescrosswell mentioned this pull request Nov 16, 2024

NETSDK1139: The target platform identifier android was not recognized #3759

Closed

Rok Rode and others added 3 commits November 17, 2024 21:43

Improve test coverage and target all deriving type declarations

29cdf08

Merge branch 'main' into pr/3725

c1a505c

Merge branch 'filesystem-usage-analyzer' of https://github.com/RRode/…

dde783f

…sentry-dotnet into pr/3725

jamescrosswell merged commit 1928a48 into getsentry:main Nov 20, 2024
21 checks passed

RRode deleted the filesystem-usage-analyzer branch November 20, 2024 20:52


		<RootNamespace>Sentry.Analyzers</RootNamespace>
		<AssemblyName>Sentry.Analyzers</AssemblyName>

Uh oh!

Filesystem usage analyzer #3725

Filesystem usage analyzer #3725

Uh oh!

Conversation

RRode commented Nov 2, 2024 • edited by bruno-garcia Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bruno-garcia commented Nov 4, 2024

Uh oh!

RRode commented Nov 4, 2024

Uh oh!

bruno-garcia commented Nov 4, 2024 • edited by bitsandfoxes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bruno-garcia Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

RRode Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

RRode commented Nov 6, 2024

Uh oh!

bruno-garcia left a comment

Choose a reason for hiding this comment

Uh oh!

bruno-garcia Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

RRode Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

bruno-garcia commented Nov 10, 2024

Uh oh!

RRode commented Nov 10, 2024

Uh oh!

bruno-garcia left a comment

Choose a reason for hiding this comment

Uh oh!

bruno-garcia Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bruno-garcia commented Nov 10, 2024

Uh oh!

jamescrosswell commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamescrosswell commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RRode commented Nov 13, 2024

Uh oh!

jamescrosswell commented Nov 14, 2024

Uh oh!

RRode commented Nov 14, 2024

Uh oh!

jamescrosswell commented Nov 15, 2024

Uh oh!

codecov bot commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jamescrosswell commented Nov 15, 2024

Uh oh!

Uh oh!

jamescrosswell commented Nov 20, 2024

Uh oh!

RRode commented Nov 20, 2024

Uh oh!

Uh oh!

RRode commented Nov 2, 2024 •

edited by bruno-garcia

Loading

bruno-garcia commented Nov 4, 2024 •

edited by bitsandfoxes

Loading

jamescrosswell commented Nov 12, 2024 •

edited

Loading

jamescrosswell commented Nov 13, 2024 •

edited

Loading

codecov bot commented Nov 15, 2024 •

edited

Loading