cincheo / jsweet

A Java to JavaScript transpiler.
http://www.jsweet.org
Other
1.46k stars 160 forks source link

Bug in CandyProcessor.processCandies #334

Open Eclesia opened 7 years ago

Eclesia commented 7 years ago

Hi, there seems to be a problem in the CandyProcessor.processCandies line 155 .

if (newStore.equals(candiesStore)) {
            logger.info("candies are up to date");
            return;
        }

This condition checks if the candys are present in the .jsweet folder. The .jsweet folder is never deleted, this acts like a cache and after the first clean/build this condition will always be true. The problem is that in the maven plugin we define : target/js/candies which is not processed after the first time because of the above condition which always exit the function.

Or there process flow has an issue or the .jsweet folder should be placed in the /target/ folder and be erased at each build.

renaudpawlak commented 7 years ago

This code is quite tricky but I believe it is ok though. If you look at the implementation of CandyStore.equals, it uses a containsAll, which in turn will compare the CandyDescriptor instances (stored in the store). The CandyDescriptor.equals() function contains a test on the last update timestamp...

All this means that newStore.equals(candiesStore) will be false if any of the candy jar in the classpath has been updated (timestamp-wise).

@lgrignon, I am missing something? @Eclesia, does it make sense?

Eclesia commented 7 years ago

what you say is right but it's not the issue here. Imagine the following :

From what I understand the target/js/candies parameter is passed down to the candy-management classes which uses it to extract the .js if it's not already done. The candy-management classes should not take the 'target/js/candies' and always extract the .js in the .jsweet folder, and only afterward if a 'target/js/candies' is given then it should copy the *.js to the requested folder. (I believe it should be the maven plugin which makes the copy, not the candy-management classes)

renaudpawlak commented 7 years ago

Right. I get it now. I don't think that it is too critical for now but it should be fixed at some point. Thanks for your insights!