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

Exclude jars imported into the project for IntelliJ Junit/Scala runner #331

Merged
merged 7 commits into from Feb 28, 2018

Conversation

Projects
None yet
2 participants
@wisechengyi
Copy link
Contributor

wisechengyi commented Feb 27, 2018

Problem

Currently IntelliJ Junit/Scala runner will bring in all the jars imported into the project to classpath. This is not correct because not all jars imported are needed at runtime, thus causing classpath clashing issues sometimes, e.g. slf4j bindings.

Solution

Exclude all jars in project except jars on the JDK path, IntelliJ installation paths, and plugin installation path

This patch also disables dynamic classpath for intellij runner since jars in project are excluded from the runner, the command should never be too long.

@wisechengyi

This comment has been minimized.

Copy link
Contributor

wisechengyi commented Feb 27, 2018

trying to see where I can add some tests..

@wisechengyi wisechengyi requested a review from baroquebobcat Feb 27, 2018

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

This looks reasonable to me. I'd like to see some tests, before 👍 .

Is the javadoc comment on updateJavaParameters still accurate, or do we only support manifest jars now?

Optional.ofNullable(PluginManager.getPlugin(PluginId.getId("JUnit")))
.map(s -> s.getPath().getParentFile().getParentFile().getParentFile().getAbsolutePath()).get()
);
}

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 27, 2018

Contributor

I think this would be easier to follow if the code that builds pathsAllowed were broken out into a separate method.

This comment has been minimized.

@wisechengyi

wisechengyi Feb 27, 2018

Contributor

Done

wisechengyi added some commits Feb 27, 2018

@wisechengyi wisechengyi requested a review from cosmicexplorer Feb 27, 2018

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Thanks for the tests. Looks good to me. I've got one minor text suggestion.

* <p/>
* There are two ways to do so:
* 1. If Pants supports `--export-classpath-manifest-jar-only`, then only the manifest jar will be
* Pants needs to support `--export-classpath-manifest-jar-only`, then the manifest jar will be

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 27, 2018

Contributor

Maybe,

This function will fail if the projects' Pants doesn't support `--export-classpath-manifest-jar-only`.
The function assumes that Pants has created a manifest jar that contains all the classpath links for the particular test that's being run.

This comment has been minimized.

@wisechengyi

wisechengyi Feb 28, 2018

Contributor

Done. Thanks!

@wisechengyi wisechengyi merged commit 43ec02d into pantsbuild:master Feb 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wisechengyi wisechengyi deleted the wisechengyi:cp_clash branch Feb 28, 2018

@wisechengyi wisechengyi referenced this pull request Mar 11, 2018

Merged

Prepare 1.8.1 release #334

wisechengyi added a commit that referenced this pull request Mar 12, 2018

Prepare 1.8.1 release (#334)
### Notable Changes

* Fix "Unrecognized command line flags on global scope: --async" (#323)
* Pick up .ij.import.rc file for dynamic pantsrc insertion (#324)
* Exclude jars imported into the project for IntelliJ Junit/Scala runner (#331)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment