Add option to suppress warning about unresolved signature #83
Comments
|
Thanks for using & testing the (new) gradle plugin! I know about this issue, one possibility would be to just print a single-line warning and only print the other ones on verbose logging. Another possibility would be to make the signatures dependent on sub module. You could define and preconfigure the task in the parent module, but make the signatures part using a configuration (see https://docs.gradle.org/current/dsl/org.gradle.api.artifacts.ConfigurationContainer.html). As the signaturesFiles property is a FileCollection, you can define a In the sub module you could then redefine the configuration and adding files to it, or altenatively provide multiple configurations and choose the one you like. These are just suggestions! I will work on the idea to conflate the warning to a signle line warning with more info on verbose. |
With some Groovy Magic maybe you can make a condition in the configuration like...: Just as idea, not tested it's just pseudo code. But as Gradle is plain Groovy, you can use any language construct to build configurations or tasks. If I find some easy way, I could add that as howto to the wiki. In any case this is much cleaner than supressing warnings (and less error prone: If you have a typo in a signature, you would not recognize that!) |
|
Thank you for your reply. I think the single warning will go a long way. In the meantime, I'll look into the ideas you suggested:
|
|
I'd also love to see some kind of "quiet" mode for unresolved signatures. This way you could reuse a single definition forbidden APIs within a multi-module build without seeing tons of warnings. Yes, I know the risks, but I'd rather live with these than modify the plugin's config in every single sub-project. This would make me happy, but it's only applied to bundled signatures. Do you think it could be just switched to reporter = SILENT for all signature files? |
|
Hi Uwe. I know it's been a long time, but do you think it'd be a good time to reconsider allowing ignoreUnresolvable to work in non-bundled specs (see my comment above; it's essentially a one-liner modification). |
|
Another idea would be to make the failOnUnresolvableSigantures and 3state flag (new name), to disable the warning. Or maybe add a hideWarnings switch. So one can use warnings enabled during setup of build system and disable them when sigantures files were finalized. I am still a bit nervous about allowing to always hide the signatures lookup errors, because by a small typo your signatures get useless (and you have no way to test it). |
|
Would it be maybe ok, to allow to reduce the warning to a single line?: "WARNING: there are forbidden signatures that do not resolve. Use |
|
I understand the typo issue (and abuse of the potential silencing annotation). The truth is, however, that it's much, much easier to write a single set of rules in a multi-module project and import them once than configure the plugin in each and every module. Call it the "lazy" way. ;) I wonder if ignoreUnresolvable could take a package or class name as an argument: if this is non-resolvable then the rules for the entire block would be ignored. Something like this. This does increase the complexity of the definition files, unfortunately. |
|
The |
|
I am taking care of this now. My proposal (after lots of thinking): There are 2 types of problems in signatures: Missing classes and missing methods/fields. If the class is missing, it's enough to collect those non-existent classes and just long one warning message at the end. In contrast, if the class is there, but the method/field is missing, it's more serious. So we log the warning for each broken signature. If failures are enabled, nothing changes - it will bail out with error message. |
|
Will this include that "ignoreIfMissing com.google.common.collect.Lists" to allow (optional) silence on missing entire classes? :) |
|
No. Why? The warnings are reduced to a minimum (one warning per signatures file). |
|
Well, like I said -- I have one set of signatures and import them into multiple modules. Not all imports are available in each and every one, so I'd still get that warning in each of the modules. I think that ignoreIfMissing wasn't a bad idea, really, but if you want to have it your way I'll live with it. |
|
Sorry, if you do this, you have to get a warning, a single one. Use another build system that allows to do do if/else based on module names. Look at Elasticsearch. It produces no warnings, because they have a default set of signatures which is based on task name (if/else) and for some of the sub-modules they simply remove some signatures from the Gradle DSL. Maven is unfortunately not good at inheriting settings (this problem also affects the brokenness of having a common directory for signatures). |
|
I agree that having tons of warnings for every signature that fails is bad. But a single one because your build setup is not correct: It's fine. The completely silencing as done for bundled signatures is different: Those are not hand-edited. The "jdk-deprecated" ones are machine generated! They are completely silenced, because there is no risk and parsing them is a no-brainer - and it is likely that later JDK versions will cause warnings (like Java9 removing some very old deprecated stuff). |
|
Here is the pull request: #140 |
|
This is how it looks like with Apache TIKA, where This is how it looked before: This is much better. As discussed before, I don't agree to silence the warning completely, because it is still a problem in the build setup. |
|
@centic9, @tballison: how does that look to you with regards to TIKA? |
|
+1 Thank you! |
|
What do you think, maybe only show the first 2 or 3 classes? |
|
I'm good with leaving in all classes. I appreciate not including all methods :) |
|
Yeah... use a different build system doesn't help me much. Sure, I could override the config of the plugin in each and every module manually, but it's really a pain (there are 20+ modules in that build, all inheriting form a single parent, where forbidden-APIs is configured). |
|
Thanks for the comments. I now limited the number of classes displayed in the warning with a simple line length filter: |
|
I will merge #140 later. |
Fix for issue #83: Tone down warnings by missing classes in signatures files
|
I still live with this single-line warning and I still hate it. To the point of considering forking the project just to modify that single line that emits it... CI systems highlight anything that has "warning" in it; like the original poster said -- in multiproject maven setups reconfiguring for each and every project is a major pain. The missing signature classes are then not a warning, they could be legitimately ignored. Can you reconsider, Uwe? I can fork and modify locally, of course, but I'd need to publish this fork in maven central and this will eventually spread confusion. It's essentially about a single-liner that permits the use of ignoreUnresolvable outside of the bundled signature files. |
|
Hi Dawid, This is an improvement to before, because missing classes is separated from missing methods/fields. With the current default setting it would still ignore those signatures, but report them as warning (which is valid, as a wrongly declared method is a desaster). If the class is mssing because of classpath problems is a different story. I don't want to hide the "classes missing" warning completely, which is hard for debugging, but you may be able to switch it on/off. Let me think about it a little bit. I would also add a "antunit" test, because the current logic is hard to understand. |
|
Thanks Uwe. Perhaps something like what we already mentioned in this thread (ignoreIfMissing). I'm just saying that while in general you'd like to be warned about missing classes, there are cases when not being able to switch off the warning is problematic. |
|
Yeah, as said before: IgnoreIfMissing is easy to be implemented (at least for this level), you just need to hide the reportMissingClasses call. The question is: If you fail on missing signatures (the default), I think the missing classes should also fail. Maybe the ignoreMissingClasses should then trugger a warning in that case (as you want to be noisy anyways). |
|
I reopened for further discussion. |
|
If you mean a situation in which the class is there, but one (or more) of its method signatures is not then I think that should trigger an error. I don't think we need to care about emitting a warning in case some other signature is missing. Like I said, if somebody puts it explicitly in the signatures file (like me), they already know the class may not be available, so I don't think a warning is necessary then. My quick hack was not to eliminate the reportMissingClasses call but allow to use IGNORE_UNRESOLVABLE_LINE freely, as in: |
|
I just wanted to explain: Your patch is exactly not what I want to have. With te new code you can differentiate between "class not found" and "broken signature". |
|
The code above is like this for bundled signatures: In jdk-deprecated, there are signatures of classes and methods/fields that were removed in later Java versions. This is why it's completely silenced, because there is no risk: The signatures are machine-generated and you can be sure that that they are correct. |
When you implemented the hack, the reportMissingClasses were not yet available. That was added in this issue. This is the improvement here. So instead of your current "hack" the correct way would be to eliminate the reportMissingClasses if you want to silence that. |
|
In an effort to reduce the number of false warnings in our 250 project Maven module build, I rolled my own fork of this plugin that adds two configuration parameters: Setting the first one to false completely removes the "Some signatures were ignored" message. The second one will control the "Classes directory not found, forbiddenapis check skipped" warning, which typically triggers on projects without test classes for us. Coming to think of it, Maven, when asked to compile testClasses when there are none, will emit an INFO message that there is nothing to compile. So I'd actually expect this plugin to not throw a warning in that case as well. So my suggestion: make the missing signatures message configurable, and change the log level of the classes directory not found message to info. Would this be a feasible direction? Alternative solution: make use of a proper logging framework, so I can configure as an end-user to set the log level of the Signatures class to ERROR, thereby also silencing the warnings. |
|
Just wanted to jump in here with a vote for being able to suppress these warnings via configuration. While my team doesn't use maven modules we do own and manage close to 350 projects. All with varying dependencies. We use a parent-pom to configure plugins like these so they do not need to be declared in every project's pom. Creating multiple signature files and configuring the plugin per project is infeasible and, frankly, not very logical. Let's say we decided to manage multiple signature files grouped by API: jodaTimeSignatures, guavaSignatures, someCompanySignatures, etc. Project A today is configured to only use the jodaTimeSignatures and someCompanySignatures because it doesn't use guava. Now, as soon as someone adds the guava dependency they have to know to go add the guavaSignatures file to this project's configuration. No one is going to remember to do that. Not to mention all the mass updates that would be required if a new signature file is created. We have a large list of APIs that we want to ban in our codebase and we don't want to have to worry about whether or not a particular project has dependency X now or may sometime in the future. At this point, the maven logs for our projects are extremely clean and we don't like the fact that this plugin is spitting out a warning when there's really nothing wrong with the configuration. I can appreciate the fact that one of the reasons for logging this warning is to highlight misspelled APIs that may have crept into the signature files but something like that is not really a concern for my team. In summary, I've yet to see a compelling reason not to at least make this configurable. Log anything and everything that you want by default. That's no problem. But allow consumers to turn these features off if they desire even if they are doing so at their own risk. |
…aven). This is consistent with other Maven tasks and Gradle (@SkipIfEmpty effective). See #83
|
Hi @JoepWeijers
I changed this in #163. It no longer prints as warning. |
|
Hi, It will add some new setting The current option failOnUnresolvableSignatures would stay alive, but I tend to deprecate it. It's used internally for deprecated signatures (as they can tend to disappear in later Java versions), but by default it should always fail if a method or field signature is wrong, a warning printed otherwise. It's only used for backwards compatibility. I am not yet sure about deprecation, maybe we can leave it with a better name, but to me it sounds useless. During migration phase the only warning is printed if the deprecated setting is used, but thats easy to fix for the user. How does that sound? @dweiss? |
|
Sounds good to me, Uwe! |
…multi-module builds with common signatures). The old setting failOnUnresolvableSignatures was deprecated in favour of ignoreSignaturesOfMissingClasses. This closes #83
|
I created a pull request for this: #164 It needs some additional tests, but if anybody listening here wants to test it in production, go ahead! |
|
@uschindler I'd love to test this out in our code. Do you have a snapshot version deployed anywhere or would I need to build the dev branch locally myself? |
Allow to silently ignore signatures if the class is not found (e.g., multi-module builds with common signatures). The old setting failOnUnresolvableSignatures was deprecated in favour of ignoreSignaturesOfMissingClasses. This closes #83
|
Hi @mttjj, its now building and should be on sonatype snapshots soon. See https://jenkins.thetaphi.de/job/Forbidden-APIs/ for build status. Snapshots are here: https://oss.sonatype.org/content/repositories/snapshots/de/thetaphi/forbiddenapis/ (just wait for the date to change) |
…volving missing classes This should get rid of warnings. See policeman-tools/forbidden-apis#83 Signed-off-by: Yoann Rodière <yoann@hibernate.org>
…volving missing classes This should get rid of warnings. See policeman-tools/forbidden-apis#83 Signed-off-by: Yoann Rodière <yoann@hibernate.org>
I'm using the gradle plugin, version 2.0 . Please consider suppressing the warning about unresolved signature i.e. this signature:
Class 'com.google.inject.Inject' not found on classpath while parsing signature: com.google.inject.Inject [signature ignored]The use case is as follows:
The
failOnUnresolvableSignatures = falseoption makes the build still pass, but currently my build logs are polluted with messages about signatures for libraries from other projects.That being said, I just started using the plugin, and I'm open to ideas about how to better achieve my goals. Thank you for this useful plugin.
The text was updated successfully, but these errors were encountered: