Discussion about Solution files, filters and CI about sentry-dotnet HOT 9 CLOSED

I've been operating on the assumption there's a good reason for most stuff so haven't challenged any of this.
I'm in the same boat.

I think there should be as few filters as possible. Mobile vs. No Mobile comes to mind. I have not yet used a single .slnf for local development.

Regarding CI: I'm of the strong opinion that whatever happens in CI should be as close to what we have in our local environment. Ideally, those should be identical. I'm really not buying into speeding up CI at the cost of complexity.

from sentry-dotnet.

1. potentially we could rename Sentry.sln -> SentryNoMobile.sln and then rename Sentry.Full -> Sentry.sln
2. Sounds reasonable. I've been operating on the assumption there's a good reason for most stuff so haven't challenged any of this. The IsPackable build property sounds good instead if that's the only reason we have a separate solution for CI though.
3. I don't use those personally... happy to delete
4. Is there an issue having them in the repo? If they're not part of any of the solutions that get built by CI or that we use when developing, I don't think they get in the way... and it sounds like a pain maintaining a separate repo for these. If we ever need to debug anything or look into issues with Classic ASP.NET, the sample project is a good way of quickly bootstrapping things for investigation, so it's nice to have these next to the source...
5. Maybe worth thinking about the experience for folks who are new to the repo... as long as the key files they'd need (ready, contrib etc.) are obvious and easy to find, I'm happy with another approach. If they need to "know" that you have to show all files in the IDE to see relevant stuff, it starts to become a bit of a gotcha that will trip people up (repeatedly). So maybe there's an argument to keep those things referenced in the solution files.
6. I don't mind having the filters in the root directory. They're only really convenient if you're using Visual Studio anyway... in Rider you don't have a solution filter drop down to easily apply different filters in the same way.

from sentry-dotnet.

I added a feature request for msbuild btw, but I see they've got 1.2k issues on their backlog so I'm guessing we'll need to come up with another plan in the mean time.

I'll dig into the possibility of disabling target frameworks based on whether workloads are installed or not. Not quite the same but gives us maybe most of what we want/need.

from sentry-dotnet.

After trying to figure out why a specific test project wasn't running, sadly took me too long to check the build yaml. There's a solution filter for that too that has to be manually changed:

        run: dotnet test Sentry-CI-Test.slnf -c Release --no-build --nologo -l GitHubActions -l "trx;LogFilePrefix=testresults_${{ runner.os }}"

It's quite hard to reproduce CI problems locally due to this complexity.

I strongly believe we should dotnet build and dotnet test on the same target (sln or slnf) and avoid having to maintain two files for these two steps unless there's a strong reason (several minutes gain in CI?) not to

from sentry-dotnet.

Re getting rid of the solution filters, there's a StackOverflow thread that looks interesting but I haven't quite wrapped my head around it yet:

• How do I put a condition on msbuild built-in targets like Build/Rebuild?

I was thinking we could define some custom BeforeBuild logic in our Directory.Build.targets file that could check something like the matrix.os from GitHub Actions and use that to skip/exit the build for any projects that weren't relevant to a particular CI build. That way we could map projects to the CI builds in build props rather than solution filters.

However, I think what Mengdi Liang is suggesting would return an exit code of 1, which would stop the project being built but would implicitly break our builds in CI as it would look like an error to GitHub.

Maybe it's easier to keep using the solution filters to determine which projects to include in CI, but we get rid of some of the other unnecessary filters:

One note about the above is that if we had both Sentry.Bindings.Cocoa.csproj and Sentry.Samples.Wpf in the same solution file, you wouldn't be able to build that solution anywhere... It wouldn't build on Windows (due to the Cocoa stuff) and it wouldn't build on Mac (due to the WPF stuff)... So we'd have to start doing our regular development using one of the Sentry-CI-matrix.os slnf files. That might not be a bad things, as it brings CI closer to what we're doing day to day. And we could maybe take the -CI- bit out of the name of those *.slnf files.

from sentry-dotnet.

Would that work with a single solution? Today we need at least two solutions because of this hack:

We'd need a different approach to avoid building those.

I wonder if a script that generates these slnf are also an approach. Because it's useful to have less projects loaded depending on the work you do, it's just hard to maintain all of those filters/slns

from sentry-dotnet.

I wonder if a script that generates these slnf are also an approach. Because it's useful to have less projects loaded depending on the work you do, it's just hard to maintain all of those filters/slns

I took a look at rosenbjerg/slnf-gen for this but I think it's more of a job for a script than a program that needs to be compiled and slnf-gen has some limitations. So I put together a powershell script that could do this for us based on a YAML config:
https://github.com/getsentry/sentry-dotnet/pull/2576/files

So far I've tried to do it just for Sentry-CI-Build-Linux.slnf and it identified various files that I suspect should be part of that build on Linux.

The tentative config I have looks like this (all open for debate - I'm not sure I have the most context for which files should be in/out of each slnf):

# Sentry filter configuration

solution: Sentry.Full.sln

filterConfigs:

  # Core filter for CI builds on Linux
  - outputPath: Sentry-CI-Build-Linux.slnf

    # Include all projects
    includes:
      - "**/*.csproj"

    # Exclude iOS/MacCatalyst and .NET Framework projects
    excludes:
      - modules/**/*.csproj
      - "**/*Ios.csproj"
      - "**/*Cocoa*.csproj"
      - "**/*MacCatalyst.csproj"
      - "**/*SourceGen.csproj"
      - "**/*.AspNet.csproj"

from sentry-dotnet.

Regarding the NO_MOBILE hack, that flag is set for the following solutions/filters:

• Sentry.sln
• SentryAspNetCore.slnf
• SentryAzureFunctions.slnf
• SentryCore.slnf
• SentryDiagnosticSource.slnf
• SentryNoSamples.slnf

None of those are used in CI... essentially it disables mobile target frameworks for developers who are working on any of those solutions/filters, in the following scenarios (I think):

Developing on Windows

Can't target iOS/MacCatalyst

Developing on Mac

Don't want to deal with the complexity of setting up the dev environment to support mobile targets

Alternatives

The Mac use case is valid (particularly if we're thinking about non-Sentry contributors)... it took me ages to get my dev envioronment setup for mobile. It was not easy.

I think the Windows one is necessary as you literally can't build the solution on Windows unless you omit those target frameworks. Although, if that's true, I'm wondering how we do this on CI since Sentry-CI-Build-Windows.slnf is based on Sentry.Full.sln (so presumably includes those targets and builds successfully on Windows)?

It looks like we have a different solution for Tizen, and we might be able to leverage this to determine which targets to include, instead of using the Sentry.sln hack:

from sentry-dotnet.

OK, latest commit here will disable mobile targets when running the build on machines that don't have the appropriate workloads installed:

That's not exactly the same behaviour we had before... possibly we'd want to ensure the appropriate workloads are installed when running the build on our CI machines.

One possible alternative would be to leave the builds running pretty much as they are but copy Sentry_Full.sln to a SentryNoMobile.sln prior to running the build (overwriting whatever SentryNoMobile.sln exists before we do that). That way we could continue to have some Solution Filters that filter against Sentry_Full.sln and other that filter against SentryNoMobile.sln, and we could continue to use the hack we're currently using (check to see what the solution file is) as a way to determine whether or not mobile targets should be enabled.

Any other ideas?

from sentry-dotnet.