Deleted file does not affect project

[batkaevruslan]

I've noticed that if file deletion is the only change, dotnet-affected does not include the project in the list of affected.

File deletion can break compilation of the project, so it must be returned as affected.

Repro:
I've created a test to show the issue (test fails):

        [Fact]
        public async Task When_file_is_deleted_from_project_project_should_have_changed()
        {
            // Create a project
            var projectName = "InventoryManagement";
            var msBuildProject = Repository.CreateCsProject(projectName);
            // Create a file with some changes
            var targetFilePath = Path.Combine(projectName, "file.cs");
            await this.Repository.CreateTextFileAsync(targetFilePath, "// Initial content");

            // Make a commit and keep track of the sha
            this._fromCommit = Repository.StageAndCommit()
                .Sha;

            this.Repository.DeleteFile(targetFilePath);
            // Commit the changes
            this._toCommit = Repository.StageAndCommit()
                .Sha;

            Assert.Single(AffectedSummary.ProjectsWithChangedFiles);
            Assert.Empty(AffectedSummary.AffectedProjects);

            var projectInfo = AffectedSummary.ProjectsWithChangedFiles.Single();
            Assert.Equal(projectName, projectInfo.GetProjectName());
            Assert.Equal(msBuildProject.FullPath, projectInfo.GetFullPath());
        }

[leonardochaia]

Interesting! thank you so much for reproducing with a test. I honestly though we already had this dialed in.

I will take a look as soon as I have the time, which unfortunately is on the low end right now.

Leo.

[leonardochaia]

Hey all! I've been taking a look at this and I've found the culprit, still don't know how we are gonna fix it but wanted to provide an update. This is happening since we introduced MSBuild Predictions to determine a project's inputs. It is not an MSBuild.Predictions bug, but it is in how we use it.

dotnet-affected/src/DotnetAffected.Core/Prediction/PredictionChangedProjectsProvider.cs

Lines 62 to 91 in 2de16bc

	public IEnumerable<ProjectGraphNode> GetReferencingProjects(
	IEnumerable<string> files)
	{
	var hasReturned = new HashSet<string>();
	var collector = new FilesByProjectGraphCollector(this._graph, this._repositoryPath);
	_executor.PredictInputsAndOutputs(_graph, collector);
	// normalize paths so that they match on windows.
	var normalizedFiles = files
	.Where(f => !_fileExclusions.Any(f.EndsWith))
	.Select(Path.GetFullPath);
	foreach (var file in normalizedFiles)
	{
	// determine nodes depending on the changed file
	var nodesWithFiles = collector.PredictionsPerNode
	.Where(x => x.Value.Contains(file));
	foreach (var (key, _) in nodesWithFiles)
	{
	if (hasReturned.Add(key.ProjectInstance.FullPath))
	{
	yield return key;
	}
	}
	}
	}
	}
	}

You can see here how we use MSBuild.Predictions to determine which files belongs to which project. However, since the file is deleted, it is not predicted as a project input.

This wouldn't have been an issue with the old behavior before predictions, where we simply checked if the file path was a subpath of the project path, but references outside of the project's directory were ignored..

Not sure what the fix is yet.. but that is why it is broken.

[tanordheim]

Not sure what the fix is yet.. but that is why it is broken

Yeah I just took a crack at this too. I guess one option is to walk all the commits here and get a list of all files ever being in that project across the commit range; that should detect files being removed at least. But that might have other bad side effects.

[leonardochaia]

Hi @tanordheim , deleted files are already being discovered by git diff, however, we fail to map them to a project due to the project not having the deleted file predicted as an input. I think doing the path contains thing may be enough for now, at least as a workaround.

[tanordheim]

Hi @tanordheim , deleted files are already being discovered by git diff, however, we fail to map them to a project due to the project not having the deleted file predicted as an input. I think doing the path contains thing may be enough for now, at least as a workaround.

Yeah, what I meant is if we walk all the commits to detect any files that have been present in the project at any given time we would then find a project that referenced the deleted file in one of the commits before the file was deleted and pick up on the affected project that way.

Your way sounds simpler to implement though, maybe its the way to go. I guess that leaves a bug where deleted files in a project that was referenced outside of the project root will not cause the project to be affected. As far as I understand, the reason for using MSBuild Predictions is to cover cases where files are referenced outside the project root. That is still a fairly minor thing I guess, I have no use cases like that personally.

[leonardochaia]

guess that leaves a bug where deleted files in a project that was referenced outside of the project root will not cause the project to be affected.

That is correct yep. I think it is hard to fix it in a performant way. Still need to think about it.

v4 will get released today with net8 support and we'll leave this one for a feature release soon.

[leonardochaia]

Hi,

The MsBuildGitFileSystem is only being used to load projects when Nuget packages changes. I think that if we use the MsBuildGitFileSystem (EagerCachingMsBuildGitFileSystem,until dotnet/msbuild#7956 ) when creating the ProjectGraph, providing a custom ProjectInstanceFactoryFunc, which uses a shared EvaluationContext with our custom file system implementation. Since MSbuild.Predictions uses the ProjectGraph, I think if we use the git based FS we should be able to detect those deleted files using predictions!

However, the constructor we need to use to provide the custom file system is currently internal, dotnet/msbuild#9678 (comment)

[dotTrench]

Hello!

I've been looking into this a bit. I think a possible workaround would be to pass deleted files using a custom file system. Similar to the MsBuildGitFilesystem. This way the files would still be passed to the Collector during the prediction process. I think this works fine for most cases, excluding project files. e.g. Directory.Build.props/targets, where the issue of MSBuild not using the provided filesystem really becomes an issue. These would be handled separately similar to how Directory.Packages.props is handled currently.

I only think this would work when comparing against Index | WorkingTree though, i.e. --from(?) not being set. I'm not smart enough to figure out how that would work :).

I have a fork with a very naive and incomplete PoC here

[krokofant]

Isn't this a major issue with this project? Is removing files not such common occurrence that a workaround won't be implemented? I don't quite understand how this is acceptable to use in a CI process where a removal (that can change a lot) is not considered affected. Am I missing something? Feels like I'm gaslighting myself thinking about this 😅

Is the scenario when a file is removed and no edits are made unlikely enough that it can most often be ignored?

In places where whomever used this, is it expected that all consumers just know that some file removals won't trigger build/deploy/test/whatever-you've-chained? Should the issue be mitigated by sometimes building everything, or manually building (via assumption) the thing that should be detected as affected? 🤔😕

[leonardochaia]

Hi @krokofant , it is a major issue and that's why it is pinned. We accept pull requests if you want to give it a go 💯

@dotTrench thanks for this! I'll be taking a look during next week. Could you please submit a PR? Thank you!!

Kind regards,
Leo.