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

JDK level compilation switched to v7 for Eclipse m2e #27

Closed
wants to merge 2 commits into from
Closed

JDK level compilation switched to v7 for Eclipse m2e #27

wants to merge 2 commits into from

Conversation

axel3rd
Copy link
Contributor

@axel3rd axel3rd commented Jun 24, 2017

Since commit ea5e38c, JDK v7 is used
for compilation.
Some class (Files, Path, ...) are now used and require JDK v7.
This change is mainly to link a JDK v7 when Eclipse m2e is used (v6 used
before, so compilation problem, manuel update required).

Since commit ea5e38c, JDK v7 is used
for compilation.
Some class (Files, Path, ...) are now used and require JDK v7.
This change is mainly to link a JDK v7 when Eclipse m2e is used (v6 used
before, so compilation problem, manuel update required).
@michael-o michael-o self-assigned this Jun 24, 2017
@michael-o michael-o added the bug label Jun 24, 2017
@michael-o michael-o added this to the 3.1.0 milestone Jun 24, 2017
@michael-o
Copy link
Member

The PR is incomplete to me. Look at 6397c3e, two new classes have been introduced. They can be easily deprecated or removed. They also maybe other spots.

@rfscholte
Copy link
Member

labelled as bug? I would say enhancement...

@michael-o
Copy link
Member

I don't thing so because enforcer requires 1.7. Why should MCOMPILER require 1.6 only? This looks like a mismatch or bug. Unless I do not fully understand the issue.

@axel3rd
Copy link
Contributor Author

axel3rd commented Jun 24, 2017

two new classes have been introduced

Good catch, thanks.

Unless I do not fully understand the issue.

Basically this feature was to have the correct JDK version linked in Eclipse when project is imported :-)

@axel3rd
Copy link
Contributor Author

axel3rd commented Jun 24, 2017

DirectoryScanner.isSymbolicLink seems bugged: only parent tested, not the complete file => should be

  1. fixed or deleted (NioFiles.isSymbolicLink could be now used natively by end users) ?
  2. and :
    1. In a dedicated PR
    2. In a separated commit of PR
    3. In the globale commit of PR
      ?

@michael-o
Copy link
Member

@rfscholte that's the bug:

$ mvn help:effective-pom | grep maven.compiler.
    <maven.compiler.source>1.6</maven.compiler.source>
    <maven.compiler.target>1.6</maven.compiler.target>

@michael-o
Copy link
Member

@axel3rd Incomplete tests should be separate PR. NioFiles shall be deprecated because they are public.

@rfscholte
Copy link
Member

I don't thing so because enforcer requires 1.7. Why should MCOMPILER require 1.6 only?

This is intended. By using source+target all classes get the right byte-code version and can be used by JDK. However, by compiling with JDK 7 we can classes from this distribution. Normally you would call it "leaking" and the animal-sniffer-maven-plugin prevents you from doing so, but now we are using this feature. Java 9 comes with a better option: JEP 247: Compile for Older Platform Versions

@axel3rd
Copy link
Contributor Author

axel3rd commented Jun 24, 2017

Incomplete tests should be separate PR.

Ok

NioFiles shall be deprecated because they are public.

After reviewing the content, perhaps could stay unchanged. This is an utility class who "simplify" the usage of Files and Path (with usage if File).

@michael-o
Copy link
Member

@rfscholte Makes sense now, though ugly. Will be leave at 6 now or move to 7? If not, we can close this PR.

@rfscholte
Copy link
Member

Let's call it forward compatibility :) I'm not sure if this is the right moment to require Java 7, Plexus Utils is used a lot.

@michael-o
Copy link
Member

@axel3rd I think this PR has to be dropped because it won't do what we want. I think the best is a profile for m2e action to override the javaVersion.

@axel3rd axel3rd changed the title JDK level compilation switched to v7 JDK level compilation switched to v7 for Eclipse m2e Jun 24, 2017
@axel3rd
Copy link
Contributor Author

axel3rd commented Jun 24, 2017

PR updated with java 1.7 compilation level restricted to m2e Eclipse plugin usage (title updated).
For , DirectoryScanner.isSymbolicLink bug, see #28.

@michael-o
Copy link
Member

@rfscholte Any objections to merge this? Looks good to me now.

@rfscholte
Copy link
Member

I see that Eclipse is trying to pick the best matching JDK. You'll only see this issue when you have JDK6 installed. However, if you run any goal from the IDE, its bytecode version will be wrong.
This profile is only there to fool Eclipse. Eclipse should respect the requireJavaVersion enforcer-rule instead.
So I'm -0. I don't think it's worth it (number of developers which also have JDK6 is to low). But to prevent the compilation error of NioFiles in Eclipse, I would prefer proper Javadoc in that class. The solution would be to manually change the JRE System Library.

@michael-o
Copy link
Member

@rfscholte Agreed. Though, Eclipse ignores the Enforcer rule here. @axel3rd Do you have JDK 6 installed?

@axel3rd
Copy link
Contributor Author

axel3rd commented Jun 25, 2017

@michael-o : Yes, I have the JDK 6 configured in my Eclipse. I have updated the profile comment with the exact cause. Even if this is a m2e bug, this workaround can't be added ? It is boring to configure the JRE System Library to JavaSE-1.7 manually under Eclipse on any checkout ; But I can continue to do that if this profile is not sustainable ...

@axel3rd
Copy link
Contributor Author

axel3rd commented Jun 28, 2017

Thanks :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants