Navigation Menu

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

[DROOLS-1170] drools-pmml: use 'xjc' binary instead of jaxb2-maven-pl… #814

Merged
merged 1 commit into from Jun 21, 2016

Conversation

psiroky
Copy link
Contributor

@psiroky psiroky commented Jun 21, 2016

…ugin

  • jaxb2-maven-plugin is not yet supported on JDK9. The original
    idea was to create new profile which would be jdk9-specific
    and use the exec-maven-plugin only there. However, this does not
    work because the classes generated directly by 'xjc' and the
    plugin are not exactly the same. The one big difference is that
    the plugin useis 'is' prefix for Boolean properties whereas
    xjc will use 'get' prefix (e.g. public Boolean isAdult() vs
    public Boolean getAdult()). It seems the later is correct as
    'is' should only be used for primitive boolean, and not for object
    type Boolean (http://download.oracle.com/otndocs/jcp/7224-javabeans-1.01-fr-spec-oth-JSpec/).
    To make the configuration easier, the plugin was removed
    and the jxc is called directly.

The only thing I am wondering about is if it save to assume the xjc binary is on PATH? This may not always be the case. Maybe we should use ${env.JAVA_HOME}/bin/xjc instead?

@mariofusco, @ge0ffrey could you please take a look and let me know what you think?

@ge0ffrey
Copy link
Contributor

I am not to found of these changes applying on a JDK 8 too.
I suspect that the maven-jaxb-plugin will be fixed some day in the future for JDK 9 at which time we 'll want to go back to the old method. Meanwhile it's indeed good to have a temporary workaround.

@psiroky
Copy link
Contributor Author

psiroky commented Jun 21, 2016

@ge0ffrey sure I agree with that -- once the jaxb2-maven-plugin works both on JDK8 and JDK9, then we should use it again. But in the meantime I don't see other option here. If you know of better option please let me know, I would be more than happy to use it.

@ge0ffrey
Copy link
Contributor

+1 to merge.
I am ok with this change - I don't know of a better option and we need to get the JDK9 builds running. I might add a comment TODO to state this is a temporary workaround until jaxb's normal goal supports it.

@psiroky psiroky force-pushed the jdk9-disable-jaxb2-maven-plugin branch from d96fb6e to 0482b9e Compare June 21, 2016 13:54
@psiroky
Copy link
Contributor Author

psiroky commented Jun 21, 2016

I tweaked the TODO comment a bit. Hopefully it is a bit more clearer now.

@ge0ffrey
Copy link
Contributor

+1 (I might have missed it the first time, sorry if I did)

…ugin

 * jaxb2-maven-plugin is not yet supported on JDK9. The original
   idea was to create new profile which would be jdk9-specific
   and use the exec-maven-plugin only there. However, this does not
   work because the classes generated directly by 'xjc' and the
   plugin are not exactly the same. The one big difference is that
   the plugin useis 'is' prefix for Boolean properties whereas
   xjc will use 'get' prefix (e.g. public Boolean isAdult() vs
   public Boolean getAdult()). It seems the later is correct as
   'is' should only be used for primitive boolean, and not for object
   type Boolean (http://download.oracle.com/otndocs/jcp/7224-javabeans-1.01-fr-spec-oth-JSpec/).
   To make the configuration easier, the plugin was removed
   and the jxc is called directly.
@psiroky psiroky force-pushed the jdk9-disable-jaxb2-maven-plugin branch from 0482b9e to 04fc082 Compare June 21, 2016 14:35
@psiroky psiroky merged commit 46c998a into apache:master Jun 21, 2016
@psiroky psiroky deleted the jdk9-disable-jaxb2-maven-plugin branch June 21, 2016 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants