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

Add path filtering support to version.json #399

Merged
merged 26 commits into from Jan 20, 2020

Conversation

saul
Copy link
Contributor

@saul saul commented Oct 23, 2019

Goes part of the way to solving #160 (superseding #223 which has been quiet for most of the year).

Opening this now in a WIP state to get early feedback.

This PR adds a new pathFilters property to version.json. The array defines the paths to include in version height calculations. If a commit does not affect any path in pathFilters, it does not increment the version height. This feature is entirely opt-in - if pathFilters is not defined, NB.GV calculates version height as it previously did.

Todo:

  • Update JSON schema
  • Update existing tests
  • Write new tests
  • Write monorepo guidance explaining how to use this feature

Closes #160

@saul
Copy link
Contributor Author

saul commented Nov 11, 2019

@AArnott mind giving this another review? I've added some tests now and I'm happy that it's working as expected.

@saul saul changed the title [WIP] Add path filtering support to version.json Add path filtering support to version.json Nov 11, 2019
@shana
Copy link

shana commented Nov 18, 2019

I was testing out the latest build from this PR on one of my projects, and as soon as I use bump the package version to the one from this PR, the generated version stops incrementing (it's always 1.0.0), so something here breaks the default behaviour. I can't get it to increment with pathFilters, either.
Nevermind, there was an unrelated typo that was breaking the build. This is working great now!

@saul
Copy link
Contributor Author

saul commented Nov 19, 2019

@AArnott this is ready for review when you get a moment.

@AArnott AArnott self-assigned this Nov 20, 2019
Copy link

@Smaug123 Smaug123 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@AArnott Did you get any time to read over this again?

src/NerdBank.GitVersioning/FilterPath.cs Show resolved Hide resolved
Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Thanks. Your changes look pretty good, but I have a couple concerns which I've shared in comments.

src/NerdBank.GitVersioning/FilterPath.cs Outdated Show resolved Hide resolved
src/NerdBank.GitVersioning/FilterPath.cs Show resolved Hide resolved
src/NerdBank.GitVersioning/FilterPath.cs Outdated Show resolved Hide resolved
src/NerdBank.GitVersioning/FilterPath.cs Show resolved Hide resolved
src/NerdBank.GitVersioning/GitExtensions.cs Outdated Show resolved Hide resolved
src/NerdBank.GitVersioning/GitExtensions.cs Outdated Show resolved Hide resolved
src/NerdBank.GitVersioning/GitExtensions.cs Outdated Show resolved Hide resolved
src/NerdBank.GitVersioning/version.schema.json Outdated Show resolved Hide resolved
src/NerdBank.GitVersioning/GitExtensions.cs Outdated Show resolved Hide resolved
@shana
Copy link

shana commented Dec 1, 2019

I found that if I set a path filter to the current directory as "./", it won't properly update the version. Same for "../[name of directory]". It really looks like relative paths to directories is not working.

The only way to monitor the current directory is to set its absolute path, which is kind of annoying, since it's nice to have a set of path filters that don't have hardcoded directory names that I can copy around to all the separate projects in the repo, like:

"pathFilters": ["./", "../Directory.Build.props", "/icon.png", "/LICENSE.md", ":/common"],

@saul
Copy link
Contributor Author

saul commented Dec 10, 2019

@shana I find that surprising. There is already a test GetVersionHeight_IncludeFilter which uses a relative path to include the project directory, and it passes.

Can you confirm that you're definitely seeing this problem as you describe?

@saul
Copy link
Contributor Author

saul commented Dec 10, 2019

Hi @AArnott - sorry for taking so long to get back to you.

I think I've covered all of your review comments - I've left a couple of them unresolved as I'm still waiting for a response from you on that.

When you get a moment I think this is ready for re-review, thanks.

@shana
Copy link

shana commented Dec 13, 2019

@saul Now I can't reproduce it anymore. It's really confusing. It might be some cached state in the dotnet build server itself - build tasks dlls don't get unloaded between builds, and if one project is using one version of a build task and another project another version, it causes all kinds of horrible build failures. I'm having to shutdown the build-server every time I switch projects, so maybe that's what's happening... 😕

@AArnott
Copy link
Collaborator

AArnott commented Dec 20, 2019

Just FYI I'm hoping to review this in the next few days. My vacation has started so I'm catching up on OSS repo work.

@AArnott
Copy link
Collaborator

AArnott commented Dec 21, 2019

OK, these changes look good. Thanks for thoroughly addressing my concern about respecting filters at each individual commit.

My only remaining concern is perf of the GetCommitHeight method, which now does lot more work per commit and allocates lots more memory (and thus produces GC pressure that can hurt overall throughput too). I think we can likely mitigate my concerns through caching the VersionOptions and PathFilters based on the blob sha of version.json so that we only pay the tax on commits that actually changed the file.
Also the continueStepping delegate already calls VersionFile.GetVersion and your code adds yet another call to it. So to minimize perf impact (particularly to users who are not even using filters) I think we need to apply this cache such that it can be shared with that continueStepping delegate as well.

Do you want to try resolving this? If not, I will try to find time over the next couple weeks.

@saul
Copy link
Contributor Author

saul commented Dec 21, 2019

Thanks for looking at this again @AArnott. I'm away from a dev machine for the holidays so can't contribute in a meaningful way until January. I'm happy to resolve the perf concerns when I return (unless you have some free time before then).

Another thing that crossed my mind which needs resolving (and testing) is invalid pathFilters in the version.json file. We need to be resilient to that. At the moment I think an exception would cause the height methods to crash.

@AArnott
Copy link
Collaborator

AArnott commented Dec 21, 2019

I don't think I'm too concerned about invalid paths. If the json can't parse we throw as well. People need to edit any commits that introduce a failure, so they are unlikely to ever push such commits in the first place.

@saul
Copy link
Contributor Author

saul commented Jan 3, 2020

@AArnott I've implemented a partially working cache, but I'm having some problems. The cache (as written) assumes that the object returned by VersionFile.GetVersion is not mutated. If it's mutated, then the cached VersionOptions are clobbered. This is the reason that those 2 tests are failing in CI - version.json inheritance causes the object returned by GetVersion to be mutated.

With this in mind there are a couple of options:

  1. If we find a cached version.json in VersionFile.GetVersion, we clone it before returning. This leaves the caller to mutate the object as they wish. We can implement the cloning by:
    1. Implementing ICloneable/MemberwiseClone on VersionFile. This is brittle as it must be manually updated when new properties are added to VersionFile.
    2. Serialise the VersionFile, then deserialise it. This completely defeats the purpose of caching in the first place (to save JSON deserialisation).
  2. We don't cache in VersionFile.GetVersion at all.

I'll implement the 1st option with MemberwiseClone so you have something to review that works and implements your comments above. However, I personally think that the 2nd option (no caching) is better as it's introducing complexity and brittleness for what I imagine will be very little gain.

Thanks

@AArnott
Copy link
Collaborator

AArnott commented Jan 19, 2020

My concern about caching is that without it, You're adding another call to VersionFile.GetVersion for every commit that NB.GV has to walk in its discovery of the version height. I expect this will double the time for all NB.GV version calculations even without any path filters being used. That would be unacceptable as some (large) repos already experience a significant perf hit to their build from using NB.GV. We can't let this feature degrade perf for existing users.

I'll take a look at what you've done though and see how we can best mitigate it.

@AArnott
Copy link
Collaborator

AArnott commented Jan 19, 2020

I replaced your cache work with something simpler that I think will do the trick.
At this point I think we can merge.

@AArnott
Copy link
Collaborator

AArnott commented Jan 19, 2020

Oh whoops. I forgot about documentation. Can you write it up, @saul?
I added links to an empty doc/pathFilters.md file for you to fill in.

@saul
Copy link
Contributor Author

saul commented Jan 19, 2020

I've written documentation for this - let me know what you think.

Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks.

@AArnott
Copy link
Collaborator

AArnott commented Jan 20, 2020

Do you mind if I squash some of these commits before merging? It looks a bit noisy. :)

@AArnott
Copy link
Collaborator

AArnott commented Jan 20, 2020

Never mind. I wouldn't want to squash all the commits and I guess there was a merge in there, which makes squashing certain commits a bit harder.

@AArnott AArnott merged commit 4079d89 into dotnet:master Jan 20, 2020
@saul
Copy link
Contributor Author

saul commented Jan 22, 2020

Any chance we could get a fresh beta release on to NuGet so we can start using this? Thanks!

@AArnott
Copy link
Collaborator

AArnott commented Jan 22, 2020

3.1.51-beta on its way to nuget now.

@cluen
Copy link

cluen commented May 25, 2020

I've written documentation for this - let me know what you think.

@saul Is it necessary to have a version.json in the monorepo root?

I've a monorepo with these folders:

  • ConsumerA/Reports/ReportA/ (containing csproj and additional sourcefiles)
  • Main/ClassLibraries/Core/ (containing csproj and additional sourcefiles)
  • Main/Business/Business/ (containing csproj and additional sourcefiles)

I've configured my version.json in any subdir like this.

{
  "$schema": "https://raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/master/src/NerdBank.GitVersioning/version.schema.json",
  "version": "1.0",
  "publicReleaseRefSpec": [
    "^refs/heads/master$",
    "^refs/heads/v\\d+(?:\\.\\d+)?$"
  ],
  "pathFilters": ["./"],
  "cloudBuild": {
    "buildNumber": {
      "enabled": true
    }
  }
}

An update / add in any of the subfolders leads to a new version printed out by

nbgv get-version in that specific subfolder.

For example:
I've added an file to Main/ClassLibraries/Core, commited it (hash: 9dceee5c92)

Afterwards i tried nbgv get-version in Main/Business/Business/
AssemblyInformationalVersion: 1.0.5+**9dceee5c92**

Is this the expected behaviour or is it an configuration fault?

@AArnott
Copy link
Collaborator

AArnott commented May 25, 2020

Ya, I guess we'd call this by design behavior. The path filters only keeps the version height value from incrementing from irrelevant changes. Nothing was ever done to the git commit that shows up for "non-public" releases to make it show the last commit that changed within that path filter.
My first thought is that this is a good design. It doesn't impact public release versions (where the git commit wouldn't show up anyway), and the whole point of it in non-public releases is to be able to determine exactly which commit it came from (rather than the last commit that changed relevant source code for it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All packages in repo get version bump for every commit, even irrelevant ones
5 participants