-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
[SPARK-3433][BUILD] Fix for Mima false-positives with @DeveloperAPI and @Experimental annotations. #2285
Conversation
@@ -187,7 +187,7 @@ object OldDeps { | |||
Some("org.apache.spark" % fullId % "1.0.0") | |||
} | |||
|
|||
def oldDepsSettings() = Defaults.defaultSettings ++ Seq( | |||
def oldDepsSettings() = Defaults.coreDefaultSettings ++ Seq( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just fixes a deprecated warning.
@@ -25,12 +25,16 @@ FWDIR="$(cd `dirname $0`/..; pwd)" | |||
cd "$FWDIR" | |||
|
|||
echo -e "q\n" | sbt/sbt oldDeps/update | |||
rm -f .generated-mima* | |||
|
|||
./bin/spark-class org.apache.spark.tools.GenerateMIMAIgnore new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run with just new jars first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the args
array being used in GenerateMIMAIgnore.scala, where are the "new" and "old" arguments being used anywhere?
QA tests have started for PR 2285 at commit
|
QA tests have finished for PR 2285 at commit
|
Can you change an old API to make sure this is reporting correctly? |
test this please. |
QA tests have started for PR 2285 at commit
|
QA tests have finished for PR 2285 at commit
|
@rxin The test failed as expected. I will fix it once you confirm the code change. |
LGTM (although I don't really understand this part very well). |
I will let @JoshRosen take a look too, since he reported this issue as well. |
QA tests have started for PR 2285 at commit
|
QA tests have finished for PR 2285 at commit
|
@@ -25,11 +25,15 @@ FWDIR="$(cd `dirname $0`/..; pwd)" | |||
cd "$FWDIR" | |||
|
|||
echo -e "q\n" | sbt/sbt oldDeps/update | |||
rm -f .generated-mima* | |||
|
|||
./bin/spark-class org.apache.spark.tools.GenerateMIMAIgnore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be called twice? It's now called here and below as well. If it is needed twice, it would be good to document why,
QA tests have started for PR 2285 at commit
|
QA tests have finished for PR 2285 at commit
|
Jenkins, retest this please. |
QA tests have started for PR 2285 at commit
|
QA tests have finished for PR 2285 at commit
|
@ScrapCodes is this good to merge now? |
Yes, it should fix this problem. |
Jenkins, retest this please. |
QA tests have started for PR 2285 at commit
|
QA tests have finished for PR 2285 at commit
|
Ok merging this one. Thanks! |
I've backported this to |
@experimental annotations. Actually false positive reported was due to mima generator not picking up the new jars in presence of old jars(theoretically this should not have happened.). So as a workaround, ran them both separately and just append them together. Author: Prashant Sharma <[email protected]> Author: Prashant Sharma <[email protected]> Closes #2285 from ScrapCodes/mima-fix and squashes the following commits: 093c76f [Prashant Sharma] Update mima 59012a8 [Prashant Sharma] Update mima 35b6c71 [Prashant Sharma] SPARK-3433 Fix for Mima false-positives with @DeveloperAPI and @experimental annotations. (cherry picked from commit ecf0c02) Signed-off-by: Josh Rosen <[email protected]> Conflicts: project/MimaExcludes.scala
Actually false positive reported was due to mima generator not picking up the new jars in presence of old jars(theoretically this should not have happened.). So as a workaround, ran them both separately and just append them together.