Fix forbiddenapis causing travis failing #12158
Conversation
|
Thanks for making this PR. The change seems reasonable to me as a temporary workaround. We can figure out later how to resolve this issue without using the deprecated option. LGTM failed with the same error. I'm not sure why it failed though as the patch works well in my machine. I retriggered LGTM.
Another strange thing is that Travis was not triggered for this PR. I checked the "Requests" history in Travis and found that you seem to not have the permission to run Travis jobs as seen in the screenshot. Apparently, the reason that Travis didn't run for this PR before also seems related to you not having permissions because Travis is running after I close and reopen this PR. This is strange because you should have enough permissions as you are a committer. Can you please check your permission with the ASF infra team first? This seems also a critical issue as Travis has not been running for not only the PRs you create but also the PRs you merged. Please fix your permission first before you merge any PRs. |
|
Forgot to mention, thank you for creating labels that could be useful for classifying and searching for PRs. However, Also, I think labels should be general enough because having a large number of labels does not help with either classifying PRs or searching for PRs. It will rather increase the maintenance cost for labels. I don't think I remember all labels we have today. If all those labels are legit and reasonable, then we should document them. However, I don't think all of them are worth to document because many of them are specialized for particular topics. For example, I see labels of "Apache Pulsar", "Apache Spark", "Apache RocketMQ", and "Apache Avatica". Do we really need all these labels? How many PRs have been being created for each topic? To me, they have a very narrow scope of topic. "Apache Pulsar", "Apache Spark", and "Apache RocketMQ" can be just one label "extension" or "ecosystem". "Apache Avatica" should be "JDBC". This is why we have instructions documented for creating a new label on GitHub. These instructions explicitly say that you should be able to find at least 3 PRs that are relevant to the new label. This implies that the new label should be general enough to be applied to multiple PRs. The instructions also say that you have to update the doc for labels and announce the new label you created, so that other committers can be aware of the new label and what it is for. Please follow these instructions when you create a label. |
|
Travis has passed now but LGTM is still failing after a couple of retriggers. As seen in the screenshot, it seems like it failed while building the master branch (the commit d2ac146). The master branch build cannot pass until this PR is merged. So I'm going to merge this PR to unblock all other PRs. If LGTM is still unhappy even after this PR is merged, we can fix it in a follow-up PR. |
|
Hi @jihoonson , thanks for your comments, and you are welcome. Yes, the LGTM seems unstable, and I tried to restart it and it seems not work. And after you merged this PR to master branch, the status of CI shows pending but actually the job of CI already completed successfully, which is odd, we can get the details from the log of the job of CI. However, I also passed the test locally, so the PR should be OK. The entire project shares the same Token instead of multi tokens for each one, so there should be no permission issues, right. In the long term, we can use Github Action to trigger instead to ensure the stability of the CI process. Anyway, I'll go check it out recently.
About the label part, I got your point, SGTM. However, I remember that the BTW, this blocker problem caused by third-part maven plugin, so I just report this to the author of the |
|
After merging this PR, everything seems to be working fine. After about four hours, it finally went from the
|
It's not LGTM being unstable. LGTM was trying to run the forbidden API check against the master branch without your change in this PR which must fail. That's why it never passed. But now other PRs seem to be passing LGTM after this PR is merged which is expected. I'm not sure why LGTM was building the master branch though. Maybe we should fix the LGTM script or configuration.
I'm not sure if you understand me correctly. I'm not talking about flaky tests. I'm talking about Travis not running for the PRs you create or the master branch after you merge a PR into it. This seems important as we don't want to skip relevant CI in Travis in any case after the master branch is updated. I'm not entirely sure what's the issue. I'm just guessing it could be a permission issue because that's what I see in Travis as seen in my previous comment. Please contact the ASF infra team as soon as possible and fix it first whatever the issue is before you merge any PRs.
So, I don't know what you had in mind when you create those labels. My point is, please share your thought for those labels when you create them. And this is all what our instructions say too. For example, why do we need the "Kubernetes" label? I know it can be added to PRs that are related to k8s. My question is what is the use of this label? How do you use it? Your intention about this should be shared with other developers. This is also why I'm asking you to document those labels you created with your intention together. So, let me clarify my thought again here. I'm not saying "don't create labels". What I mean is, you can create whatever labels you want, but please share with other developers what label you created and how we can use it. If someone disagrees with you, then we can discuss. But we should first know why you created the label before we discuss. |
|
Hi @jihoonson, thanks for your comment. I understand your point and will raise up an issues to describe the purpose of new label before creating it. Beside, I have created a Jira ticket to ASF infra team to take a look at it. BTW, the status of CI of the master branch is failed now, which was successful about 1 hour ago, which is weird. Any thoughts on this?
|
|
Hi @jihoonson, the ASF infra team has resolved the permissions issue, thanks for your guidance 👍
|
|
Thanks for making the Jira issue. Gavin seems still waiting for you to verify the fix. Please let him know if it works well.
I really didn't mean for you to create an issue for labels. That's really unnecessary. What you should do is documented in the doc linked in my comment. Please read the doc.
I have no idea what UI you are looking at. You should be able to see why they failed by yourself. Please check those failures and raise an issue if they are legitimate. |
|
Hi, my personal optinion here: From what I understand by default you use Guava 16.0 in your main dependecies. This one has the method still available (although deprecated after 18). In opentelemetry-emitter you seem to use another more modern Guava version 30.1.1. So it can't parse the signatures file, because the class is available, but not the method. I would cleanup by signatures file and put the Guava one in a separate file (or maybe inline it directly in the pom.xml on top-level scope). For https://github.com/apache/druid/blob/6a938725861f0cafbe97e5ff8c3417fe65405cca/extensions-contrib/opentelemetry-emitter/pom.xml, I'd change the forbiddenapis config to not use the extra guava signatures file, but for the default config in root pom use the signatures file. Uwe (from forbiddenapis) |
|
Or maybe update Guava at all, 16.0 is quite old? :-) |
|
In Apache Lucene's Gradle build we have a separate signatures file for every external dependency. In the Gradle script that setups forbiddenapi we check the dependencies of the subproject and only add files based on their filename pattern (so it translates the dependency coordinates to signatures filename and includes it if found in the signatures folder): https://github.com/apache/lucene/blob/0b517573a469acadd97ce7327b2d708ef1ef79d8/gradle/validation/forbidden-apis.gradle#L30-L39; https://github.com/apache/lucene/blob/0b517573a469acadd97ce7327b2d708ef1ef79d8/gradle/validation/forbidden-apis.gradle#L73 Unfortunately with Maven this is not possible, I suspect. But maybe some automatism like this is possible. |
| @@ -1299,6 +1299,7 @@ | |||
| <artifactId>forbiddenapis</artifactId> | |||
| <version>3.1</version> | |||
| <configuration> | |||
| <failOnUnresolvableSignatures>false</failOnUnresolvableSignatures> | |||
There was a problem hiding this comment.
can we add a comment to explain why?
|
@jihoonson can we try @uschindler 's suggestion to use separate signature files instead of reverting to using the deprecated configuration flags? |
|
Yeah, his suggestion sounds good to me. @uschindler are you interested in making a PR for this issue? 🙂 |
|
Let me check it out, Maven is unfortunately very unflexible with inheriting plugin configs. In Gradle you can just "modify config" in a subproject, but I am not sure if this works with Maven, too. |
|
Here is a PR fixing the issue, but to me it still feels like a hack (copypaste of a whole section of the plugin config): #12170 |
* Fix forbiddenapis causing travis failing * Use failOnUnresolvableSignatures instead (cherry picked from commit b55f7a2)
Description
Fixed the problem that the
forbiddenapisplugin cannot recognize multiple versions after shade. Because theMoreExecutors#sameThreadExecutor()method is not found in the project code, it cannot be solved from the code level. It can only be solved at the Maven level. Even the latest version 3.2 of theforbiddenapisplugin does not support this situation yet, so simply skip this check in the new extension first to quickly fix blocker issues.This PR has: