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

[FLINK-924] Add automatic dependency retrieval for JarFileCreator #35

Closed
wants to merge 3 commits into from

Conversation

qmlmoon
Copy link
Contributor

@qmlmoon qmlmoon commented Jun 23, 2014

This improvement sets the package of user code as default package and let user to specify additional packages to be scanned.
Tested with inner class, anonymous class and additional package.
The current API could be used like:

JarFileCreator jfc = new JarFileCreator(jarFile);
jfc.addClass(X.class).addPackage("com.project").createJarFile();

@qmlmoon qmlmoon changed the title Add automatic dependency retrieval for JarFileCreator [FLINK-924] Add automatic dependency retrieval for JarFileCreator Jun 23, 2014
@StephanEwen
Copy link
Contributor

Wow, this looks like good stuff!

But: It has not a single test that covers the building, verifying that wanted dependencies ade fully added (includings things like inner classes, static references, all that) and that unwanted ones are not added.

I think we can only merge this once it has some tests.

@StephanEwen
Copy link
Contributor

Cool, the tests are a good start! I think it would be very good to add the following cases, that are tricky for the dependency visitor:

  • An anonymous inner class in a static method that accesses a local variable in its closure
  • An anonymous inner class in a non-static method that accesses a local variable in its closure
  • An anonymous inner class in a non-static method that accesses a field of the enclosing object
  • An anonymous inner class in an anonymous inner class that that accesses a field of the outermost enclosing object

@StephanEwen
Copy link
Contributor

Also: What is the impact of Java 8 lambdas on this. Would they work, i.e. would the dependency visitor find their dependencies?

@qmlmoon
Copy link
Contributor Author

qmlmoon commented Oct 18, 2014

Tests looks interesting! Java 8 lambdas is not tested. I'll test it in the next update.

@qmlmoon
Copy link
Contributor Author

qmlmoon commented Oct 18, 2014

Hey @StephanEwen , i'm a bit confused about your cases. Could you also post the correct dependencies here.

@StephanEwen
Copy link
Contributor

@qmlmoon Here are a few more details of what I think would be good to change / add as tests:

( updated, I forgot the core point ;-) )

  • The ASM dependency you added conflicts with other transitive ASM depencencies, which are 4.x . We need a dependency management entry for this, and verify that the other dependencies (that themselves depend on ASM 4.x) work well with that higher version of ASM.
  • A test in the java8 project that uses a lambda with dependency:
public class ThresholdFilter {
  public static boolean filter(Long value) {
    return value > 10;
  }
}
...
FilerFunction<Long> filter = (v) -> ThresholdFilter.filter(v);
DataSet<Long> = someData.filter(filter);
...

as well as

...
DataSet<Long> = someData.filter( (v) -> ThresholdFilter.filter(v) );
...

and

...
class UtilFunctions {
  public static FilterFunction<Long> getThresholdFilter(long threshold) {
    return (v) -> ThresholdFilter.filter(v);
  }
}

public static void main(String[] args) {
  ...
  DataSet<Long> = someData.filter(UtilFunctions.getThresholdFilter(10));
  ...
}
...
  • The last case could be added also as a test for functions through inner classes.

@qmlmoon
Copy link
Contributor Author

qmlmoon commented Nov 21, 2014

I tested it with Java 8 lambda today. ASM 4.x doesn't support Java 8. ASM 5 works as expected. I also checked other module that contains ASM dependency. It turns out that only flink-scala has a dependency to ASM 4.0, but I can't find a place that this library is used. Does anyone know where is this library used?

@aljoscha
Copy link
Contributor

ASM 4.x was used in the ClosureCleaner that was removed in the rewrite of the Scala API. I have an open pull request that re-adds the ClosureCleaner: #156. I will see whether this also works with ASM 5.x, then we can bump the version to this.

@aljoscha
Copy link
Contributor

Ok, seems to also work with ASM 5, so you can change the version there.

@StephanEwen
Copy link
Contributor

I think we need to remove the versions in both places and add a depedencyManagement entry in the root pom.xml file for this to work safely.

@aljoscha
Copy link
Contributor

Hi @qmlmoon,
sorry for the long wait on this PR. Could you please rebase on top of the current master and also get rid of the merge commits in the process?

@qmlmoon
Copy link
Contributor Author

qmlmoon commented Apr 24, 2015

I rebased the PR on the current master and removed the annoying merge commits.

@asfgit asfgit closed this in 6460743 Apr 28, 2015
@aljoscha
Copy link
Contributor

Thanks for working with this on me. Very nice contribution. 😄

@StephanEwen
Copy link
Contributor

Very good!

Is there a way we can integrate this deeper with the remote environment? Something like env.autoCollectDependencies() and then it internally build the jar by itself?

bhatsachin pushed a commit to bhatsachin/flink that referenced this pull request May 5, 2015
marthavk pushed a commit to marthavk/flink that referenced this pull request Jun 9, 2015
nltran pushed a commit to nltran/flink that referenced this pull request Jan 8, 2016
zhijiangW pushed a commit to zhijiangW/flink that referenced this pull request Jul 23, 2019
uce pushed a commit to uce/flink that referenced this pull request Aug 26, 2020
HuangZhenQiu pushed a commit to HuangZhenQiu/flink that referenced this pull request Sep 20, 2022
morozov pushed a commit to morozov/flink that referenced this pull request Apr 26, 2024
…273) (apache#35)

* [FLINK-30083] Bump maven-shade-plugin version

* [FLINK-24273][kubernetes] Relocate io.fabric8 dependency.

* change shade version bump

* undo shade plugin changes

Co-authored-by: David Moravek <dmvk@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants