Add problem filtering capability to cli frontend. #30
Conversation
Normalized the repos, otherwise my patches will be sprinkled with whitespace noise.
This commit introduces a way to configure MiMa to filter out
reported binary incompatibilities. Configuration is done in by defining
filtering rules in configuration file that is handled by Typesafe
Config library. Path to configuration file is set with `--filters` command
line option.
The resulting syntax is a bit verbose but we get future configuration
extensibility for free. We also get a way to put comments in configuration
file which is very important if we want to document why given problem can
be safely ignored.
The other command line option this commit introduces is
`--generate-filters` that generates configuration for filters to ignore
all of currently reported problems. It can be combined with `--filters`
option.
The two additional highlights of this change:
* Add factory method to `ProblemFilters` that creates problem filter
out of string definition using reflection.
* Add dependency on Typesafe Config library
NOTE: This change does not introduce filtering by packages but only by
specific problems. However, most of the necessary functionality to
implement that feature is being introduced by this change so it should be
relatively easy to add it if needed.
NOTE: This change does not come with tests. There are no tests for CLI
and I tried to introduce but it turned out to be too hard. The basic
problem is that we have to operate on paths because CLI takes paths
to files as arguments but there's no easy way to do that in traditional
tests. Probably the best way would be to write custom test interfaces
where we have access to all necessary paths but that's beyond this
contribution.
Fixes lightbend-labs#13.
By calling Config.resolve we enable substitution mechanism from Typesafe Config library. This is handy if one wants to break long list of filters into smaller variables (e.g. focused on one package) and concatenate them into one big list.
|
Please review my 3 commits individually, as the overall diff is big due to whitespace normalization. |
|
awesome -- this is crucial for maintaining binary compatibility in scala |
|
pinging @dotta. We need to get this through ASAP. |
|
LGTM - Should still check with @dotta |
|
WIll look at it by tomorrow. Unless this is dead-or-alive urgent. If that's the case, I'll do it tonight. |
|
@dotta: tomorrow is fine. |
|
Cool! :) |
|
Am I right to think that the sbt-plugin and the CLI will have two different ways for filtering? If yes, what's the rationale? |
|
@dotta: there's already a split, so there are options that are available through plugin and are not available through CLI. I looked into unifying those things but the code is rathe messy. There are at least 3 ways to configure/setup mima internally and they override each other. I think that it requires major effort to clean it up and I wasn't feeling like doing that at the moment. Also, my changes do not make situation that much worse. There's also more fundamental problem. With sbt plugin, you configure filters by passing lambdas. Until we have eval mechanism in Scala this technique cannot be used to configure filters from a text file. Even if we have eval, I don't think using lambdas for configuration is a good idea. |
|
Adding yet another way of configuring things is not making the codebase any simpler to maintain. So, if before there were already 2 ways to configure MiMa, now they are 3, which is not an improvement. That being said, I think that using the Typesafe Config library is a good idea, so I'd say that your solution is superior to the others, which is why I'm ok in merging this even without cleaning up the rest. However, before merging, I would like to see tests for avoiding breakages wrt the filter configuration file. In fact, it relies on the problems' classname to 1) not change, and 2) not be moved in a different package. What you could do is extracting the logic that loads the filters and returns the sequence of By the way, we also need to document the new features. (I know the wiki is empty at the moment, but I'll work on it right away) |
|
|
Given the urgency of BC testing for the 2.10.1 release, I would really like to see this merged and a new version of MiMa published. I agree we should do testing on our testing infrastructure, but let's first get the actual testing done so we can efficiently ensure binary compatibility of our releases. /cc @gkossakowski and @dotta |
|
@adriaanm My last comment was made 6 days ago, and I believe I've been very reasonable, i.e., I'm happy to merge this PR the moment the few comments I made are adressed. |
|
@dotta: First of all, problems cannot be moved because they are part of sbt plugin's api: https://github.com/typesafehub/migration-manager/blob/master/sbtplugin/src/main/scala/com/typesafe/tools/mima/plugin/Keys.scala#L13 That's the reason I decided that reflection is ok because those classes are being exposed externally anyway. I forgot to note that in a commit. Sorry. When it comes to testing I tried to write tests for cli but there are none so far and it's actually non-trivial to introduce. There are no existing tests for CLI that I could follow for testing those changes. The problem I faced is that you need to pass full paths to cli as arguments and I don't know how to resolve them in a test. I spent some time poking around but I couldn't find any good way around that so I gave up. If you have specific suggestion how to overcome that problem I'm all ears. |
| /** | ||
| * Parses Config definition into sequence of problem filters. | ||
| * | ||
| * @exception ConfigException |
|
You have missed a couple of places where you should replace Anyway, amends commits as needed and I'll merge the PR. Next week I'll have a look at how to write tests for the CLI, and if I have a practical way to do so, I'll ask you to create tests for this PR. Ok? One more thing, could you update https://github.com/typesafehub/migration-manager/wiki/MiMa-Command-Line-Tool and shortly explain how the new options are supposed to be used (also show how a filter file looks like). The moment this is done I'll do the merge. |
As @dotta suggested, it should be `@throws` and not `@exception`.
|
Sorry for letting this hang for so long. To sum it up:
|
|
@dotta: oh, I missed your latest comment. Yes, if you figure out how to write tests for cli I'm happy to write for filters. I'll update wiki page tomorrow. |
|
Awesome. Thanks a lot Greg for your good will, appreciate it. |
Add problem filtering capability to cli frontend.
|
Thanks, Mirco! Who can release version 0.1.5? It's blocking the 2.10.1-RC2 release, which is scheduled for Feb 22nd. |
|
Today I'm busy with IDE stuff. Since the fix is in the CLI tool, can't you just build master and use the SNAPSHOT? I know it's not ideal, but I think it's practical for everyone. Also, doing a full release for just this additional fix looks like a bit of a overhead to me. |
|
We unfortunately can't use snapshot as this runs as part of our PR-validation, checkin and nightly builds on jenkins. |
|
As we are coordinating 2.10.1-RC2 with your RC1, this can wait until then. Have a nice weekend! (A compromise would be to have nightly builds of projects like this, so that we can at least use them. It's not practical to use manual builds as we have jenkins slaves all over who need to be able to get the artifact. However, nightly builds are still not ideal because you lose repeatability of your build when it relies on SNAPSHOT builds of other tools (as pointed out by @retronym).) |
|
Yes, using nightly builds is a bad idea. Is realasing mima hard? I thought it would be just one command |
|
It goes to maven central. Let's chat release when I'm in SF
|
|
Oh, also you should update the Typesafe website, and all that annoyance.
|
|
@jsuereth: sure but this can be done as a second step of a release. The most important thing is that we shouldn't be blocked by lack of mima release. |
|
@gkossakowski I can't currently do it myself because I lack the credentials (I just found out about this ;-)). Hence, I need @jsuereth to do it this time. I've already done the tagging and created a PR for bumping the version number for releasing (the 0.1.5 tag includes all commits in that PR). Getting back to your point about releasing, it's not hard, but it does require someone (currently Josh or myself) to take the time to make sure it is properly done. As you know, we are involved in other tasks as well, so it is not always desiderable to context switch. To shorten reaction time in the future, how would you feel about becoming responsible of releasing MiMa whenever you, Compiler Team, need a new release of MiMa? By the way, thanks a lot for editing the Wiki! |
|
I think it's good to rotate responsibilities for this so more people know how to do it. In order to enable rotation, we'll need docs. Ideally, those docs would be in the form of a jenkins job so that can just push the button. /cc @jsuereth |
|
ping @jsuereth -- could we please do the minimal sbt publish so that we can use our build automation to stay on top of binary compatibility? the rest of the announcement can come later |
|
Yeah, done now. On Thu, Feb 28, 2013 at 1:25 PM, Adriaan Moors notifications@github.comwrote:
|
This pull request introduces a way to configure MiMa to filter out
reported binary incompatibilities. Configuration is done in by defining
filtering rules in configuration file that is handled by Typesafe
Config library. Path to configuration file is set with
--filterscommandline option.
The resulting syntax is a bit verbose but we get future configuration
extensibility for free. We also get a way to put comments in configuration
file which is very important if we want to document why given problem can
be safely ignored.
The other command line option this commit introduces is
--generate-filtersthat generates configuration for filters to ignoreall of currently reported problems. It can be combined with
--filtersoption.
The two additional highlights of this pull request:
ProblemFiltersthat creates problem filterout of string definition using reflection.
NOTE: This pull request does not introduce filtering by packages but only by
specific problems. However, most of the necessary functionality to
implement that feature is being introduced by this pull request so it should be
relatively easy to add it if needed.
NOTE: This pull request does not come with tests. There are no tests for CLI
and I tried to introduce but it turned out to be too hard. The basic
problem is that we have to operate on paths because CLI takes paths
to files as arguments but there's no easy way to do that in traditional
tests. Probably the best way would be to write custom test interfaces
where we have access to all necessary paths but that's beyond this
contribution.
Fixes #13.