Discussion about Solution files, filters and CI #2560
Comments
|
|
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: It's quite hard to reproduce CI problems locally due to this complexity. I strongly believe we should |
I think there should be as few filters as possible. 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. |
|
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: I was thinking we could define some custom 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 |
|
Would that work with a single solution? Today we need at least two solutions because of this hack: sentry-dotnet/Directory.Build.props Lines 18 to 20 in b3f81f1 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 |
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: So far I've tried to do it just for 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" |
|
Regarding the
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 WindowsCan't target iOS/MacCatalyst sentry-dotnet/src/Sentry/Sentry.csproj Lines 13 to 14 in b3f81f1 Developing on MacDon't want to deal with the complexity of setting up the dev environment to support mobile targets sentry-dotnet/src/Sentry/Sentry.csproj Lines 12 to 14 in b3f81f1 AlternativesThe 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-dotnet/src/Sentry.Maui/Sentry.Maui.csproj Lines 15 to 16 in b3f81f1 |
|
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. |
|
OK, latest commit here will disable mobile targets when running the build on machines that don't have the appropriate workloads installed: sentry-dotnet/Directory.Build.targets Lines 57 to 72 in 6c0467a 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? |
I found it hard to get my head around this:
Mainly because:
Sentry.slnvsSentry.Full.slnwas not very intuitive to me. Shouldn'tSentry.slnalready be "full"?In that case, could it be
Sentry.slnwith everything, and just the filters on top? I understand there's some logic based on the solution file to avoid compiling the mobile targets:sentry-dotnet/Directory.Build.props
Lines 18 to 20 in b3f81f1
Is there another approach?
Sentry-CI-Pack.slnf: What does give us? Sincepackhappens on the same workflow where things are built, I imagine this saves from looking what projects need to be backed (<IsPackable>true..)? Samples are all marked as not packable already:sentry-dotnet/samples/Directory.Build.props
Line 6 in b3f81f1
Same with tests:
sentry-dotnet/test/Directory.Build.props
Line 8 in b3f81f1
Is the 'speed up' worth having to maintain this additional filter file?
slfnfiles?SentryNoSamples.slnf,SentryAzureFunctions.slnf, etc. Could this be justUnload projectinstead or are these used often enough?The biggest problem I see is that everytime we add a new project, we need to deal with 3 solutions and all subsequent slnf because there's no way to use glob patterns, or negate (like everything except
/samples, or for the CodeQL filter, just add everything undersrc, to name a couple).Sentry.Samples.OpenTelemetry.AspNetclassic msbuild project in the repo. Becausedotnet builddoesn't work here, there was an effort to keep these out, since the creation of this repo. It doesn't seem like this is built in CI (and that's probably good because it'd be slow, and require a separate build script/flow). There was another repo (https://github.com/getsentry/sentry-dotnet-samples and later https://github.com/getsentry/examples/tree/master/dotnet) to keep such samples that are rarely used and don't need to be built in CI on each PR.The
.rootsituation. I find these Solution Files thing a bit annoying. We have to manually curate a list of files/scripts etc to get into the solution. And when in the editor we only see what's there. So if something new gets added to the repo, we need to 'remember' (my weakest point) to add. These days IDEs are smarter, Rider for example:Already shows you all the files in the repo overlaying with the soltion:
Now I can see everything in the file system, as god intended:
So I propose getting rid of all of those solution items copied over in 3 different
slnfilesHow about we stuff all of these filters in some folder?
It's hard to see the side effect of some changes. It builds on your filter. But you're actually breaking some sample or other part of the solution.
Or perhaps I'm overthinking, first time trying to contribute here in a year or so. That's possible. Thoughts?
The text was updated successfully, but these errors were encountered: