pepstock-org / Charba

J2CL and GWT Charts library based on CHART.JS
https://pepstock-org.github.io/Charba-Wiki/docs
Apache License 2.0
62 stars 6 forks source link

Duplicate Class Names #35

Closed elatoskinas closed 5 years ago

elatoskinas commented 5 years ago

In certain build environments, a Java error is thrown with regards to duplicate classes when using Charba.

The error typically looks like: error: duplicate class: org.pepstock.charba.client.options.Scales

Followed by (pointing to the other duplicate class): error: cannot access Scales

An example of such classes are the Point class in org.pepstock.charba.client.options.Point and org.pepstock.charba.positioner.Point.

Renaming (and refactoring) these classes (at least one of the duplicates) fixes the problem:

stockiNail commented 5 years ago

@lightingft I'm trying to reproduce the issue but I can not. Even if the class name are the same, the package is different therefore I don't know why you get that error.

Before renaming the classes (I would like to add "Options" suffix to all classes into options package), could you share more info about the build environments you mentioned?

And if possible, also a sample of your code in order to reproduce the issue?

stockiNail commented 5 years ago

@lightingft Having a look to the fork and the branch, be aware that you have to change ONLY the class name and not the property value because that string is the property name used by Chart.js.

For instance here in the scale class.

elatoskinas commented 5 years ago

@stockiNail Thanks for the response and the heads up for refactoring the classes.

It is typically a very specific circumstance where the error happens. The issue in my case was that all the classes would be compiled into one directory temporarily, causing conflicts between class names. I believe it is possible to fix on my end by imposing a structure.

Right now, I could not exactly describe how to reproduce an error, but I have an example in a branch of the App Inventor source.

The following error occurs when running the 'ant' command: 2019-06-03

I would guess this is not a problem on a larger scale, but it can be problematic in some rare scenarios.

stockiNail commented 5 years ago

@lightingft If you agree, I would like to have a look to app inventor and your example, before proceeding to change all class names.

Honestly, the package is so important on Java that sounds strange to me that it will be ignored.

I'm gonna give a feedback asap.

elatoskinas commented 5 years ago

@lightingft If you agree, I would like to have a look to app inventor and your example, before proceeding to change all class names.

Honestly, the package is so important on Java that sounds strange to me that it will be ignored.

I'm gonna give a feedback asap.

Cheers for taking a look. This is the original branch that I forked from, by the way, in case you would like to replicate the error manually.

But then again, it also could be the case that this is one of the rare, exceptional cases where this happens and it is not exactly the fault of the Charba library, just requires some extra steps in the build environment.

This could be a consideration for the library as well, though, since renaming the classes does not cause the issue. But then again, renaming the classes might be a breaking change to current users, right?

stockiNail commented 5 years ago

@lightingft it depends ... The "options" are usually used without taking the reference to the object.

Let me say, it could.... we could think to change in new major version (3.x) even if it's not planned yet because we are waiting the new version of CHART.JS and maybe (and hopefully) GWT 3.

But I need to take time to see better we we can do. Maybe there is any way

stockiNail commented 5 years ago

@lightingft I think I have found why there is that error.

First of all, there are 2 JDK issues related to this issue:

Having a look how it could be bypassed, I tried to apply the workaround to your fork of AppInventor.

I did the following steps:

  <macrodef name="ai.javac">
    <attribute name="sourcepath" default=""/>
    <attribute name="srcdir" default="${src.dir}"/>
    <attribute name="destdir"/>
    <attribute name="encoding" default="utf-8"/>
    <attribute name="nowarn" default="off"/>
    <attribute name="source" default="7"/>
    <attribute name="target" default="7"/>
    <attribute name="deprecation" default="off"/>
    <attribute name="debug" default="${debug.compile}"/>
    <attribute name="debuglevel" default="lines,vars,source"/>
    <element name="implicit-elements" implicit="yes"/>
    <sequential>
      <depend srcdir="@{srcdir}"
              destdir="@{destdir}">
        <implicit-elements/>
      </depend>
      <javac sourcepath="@{sourcepath}"
             srcdir="@{srcdir}"
             destdir="@{destdir}"
             includeantruntime="false"
             encoding="@{encoding}"
             source="@{source}"
             target="@{target}"
             deprecation="@{deprecation}"
             nowarn="@{nowarn}"
             debug="@{debug}"
             debuglevel="@{debuglevel}">
        <implicit-elements/>
      </javac>
    </sequential>
  </macrodef>

Let me know if works for you as well.

elatoskinas commented 5 years ago

@stockiNail Thanks for the thorough write-up. It actually seems that one of my problems was that I used the normal .jar instead of the sources .jar. Switching the .jar to sources works well, however, I get an error when embedding resources as follows: ResourcesType.setClientBundle(EmbeddedResources.INSTANCE);

Do the resources need to be included separately?

stockiNail commented 5 years ago

@lightingft good to hear you that it works.

Setting the client bundle, you don't include any resources but just setting the method how to load CHART.JS.

That statement is usually invoked at the beginning of application (for instance into EntryModule), before calling whatever Charba API.

What is the exception that you are getting?

elatoskinas commented 5 years ago

@stockiNail Here is the exception that I get whenever I invoke setClientBundle with either Embedded or Deferred Resources:

     [java]    Computing all possible rebind results for 
'org.pepstock.charba.client.resources.EmbeddedResources'

     [java]       Rebinding org.pepstock.charba.client.resources.EmbeddedResources

     [java]          Invoking generator com.google.gwt.resources.rebind.context.InlineClientBundleGenerator

     [java]             Creating assignment for charbaHelper()

     [java]                Finding resources

     [java]                   [ERROR] Resource js/charba.helper.min.js not found. Is the name specified as ClassLoader.getResource() would expect?

     [java]             Creating assignment for chartJs()

     [java]                Finding resources

     [java]                   [ERROR] Resource js/chart.bundle.min.js not found. Is the name specified as ClassLoader.getResource() would expect?

     [java]    [ERROR] Errors in 'org/pepstock/charba/client/resources/EmbeddedResources.java'

     [java]       [ERROR] Line 32: Failed to resolve 'org.pepstock.charba.client.resources.EmbeddedResources' via deferred binding

     [java]    [WARN] For the following type(s), generated source was never committed (did you forget to call commit()?)

     [java]       [WARN] org.pepstock.charba.client.resources.EmbeddedResources_de_InlineClientBundleGenerator`

The issue would not occur in the full .jar (with renamed classes).

stockiNail commented 5 years ago

The error looks like that GWT compiler is not able to link the resources to javascript files.

It seems that the Charba jar file does not include the javascript files. The "sources" jar does not contain JS files.

Which jar file have you added to GWT compiler?

elatoskinas commented 5 years ago

@stockiNail I have added the sources .jar, which indeed does not seem to include the Javascript files. But using this .jar does not give any other compile time errors, so it seems like a better option.

Is there a way to manually add the resources alongside the .jar?

EDIT: Accidentally clicked Close the issue, my bad.

stockiNail commented 5 years ago

@lightingft Let me recap for a while, I'm getting old and I'm lost.

We have 2 JAR files:

  1. charba-2.5.jar (with .class and .java files, and javascript ones as well)
  2. charba-2.5-sources.jar (only .java files)

We have 2 ANT targets:

  1. AiClientLib which compiles java code. In this target you should use charba-2.5-sources.jar as described to bypass the issue "duplicate class". In this target you don't need any javascript sources
  2. YaClientApp which invoke GWT compiler to create javascript code. In this target you should use charba-2.5.jar which contains the javascript files too (and not charba-2.5-sources.jar)

I'm sorry for that but JDK issue is complicating this procedure....

elatoskinas commented 5 years ago

@stockiNail Oh right, I did not realize the part about the 2 ANT targets. With these in mind, I finally got it to work as expected.

For my specific issue, this issue can be closed, but renaming the class names could still be a consideration due to the JDK bug you mentioned earlier, I believe.

Thank you very much for the help!

stockiNail commented 5 years ago

@lightingft happy to hear that!

About the names of classes, I want to be very honest... I would not like to change them due to a JDK bug... I know that it could be annoying but it's part of the JAVA language...

Nevertheless I put in my TODO list but I can not promise you anything ;)

Thank you for this issue because I learned about this JDK issue which could happen again.

stockiNail commented 4 years ago

@elatoskinas we are going to new Charba version, with some breaking changes. I didn't forget the request to change class names where are duplicated and checking the status of OpenJDK issue, I see that it should have been solved into OpenJDK 9 (if I'm not wrong).

Are you still interested in this use case? If yes, we can try to change class names trying to reduce as much as possible the effort to whom is using Charba.

What do you think?

elatoskinas commented 4 years ago

@stockiNail Thanks for getting back to me;

In my case, using the two different jar files for the targets seemed to have solved the problem, so it is not an issue in the end.

I think as long as the documentation clearly specifies to use the two different targets, it should not be much of an issue (especially if the bug is fixed in a later JDK). Also due to this being a breaking change, it's probably not worth it to cater to the very few use cases where this does indeed break something.

stockiNail commented 4 years ago

@elatoskinas Thank you very much and I agree.

Let's stay as is.