siom79 / japicmp

Comparison of two versions of a jar archive
https://siom79.github.io/japicmp
Apache License 2.0
712 stars 107 forks source link

Report does not include changes to constant values #238

Open garethfiler opened 5 years ago

garethfiler commented 5 years ago

I have tried to get this working with 0.13.1 and 0.14.0 but having no luck. I have a class with a number of constants defined.

` public class ClassWithStaticConstants {

private ClassWithStaticConstants() {
}

public static final String CONSTANT_A = "constant.a";
public static final String CONSTANT_B = "constant.b";
public static final String CONSTANT_C = "constant.c";
public static final String CONSTANT_D = "constant.d";

} `

If I change the string in constant CONSTANT_A to "consistent.a" then I would expect this change to be picked up in the report. Unfortunately it doesn't. If I change the constant definition from CONSTANT_A to CONSISTENT_A then the report shows the expected values.

Are there any configuration values that may be incorrect? I have pasted the full plugin definition below

    <properties>  
        <previous.release.version>1.9.44</previous.release.version>
        <previous.release.groupId>${project.groupId}</previous.release.groupId>
        <previous.release.artifactId>${project.artifactId}</previous.release.artifactId>
        <previous.release.type>${project.packaging}</previous.release.type>
        <japicmp.plugin.version>0.14.0</japicmp.plugin.version>
        <japicmp.skip>false</japicmp.skip>
        <japicmp.break.build>false</japicmp.break.build>
        <japicmp.plugin.packaging>war</japicmp.plugin.packaging>
    <properties>

  <plugin>
                <groupId>com.github.siom79.japicmp</groupId>
                <artifactId>japicmp-maven-plugin</artifactId>
                <version>${japicmp.plugin.version}</version>
                <executions>
                    <execution>
                        <phase>verify</phase>
                        <goals>
                            <goal>cmp</goal>
                        </goals>
                    </execution>
                </executions>
                <configuration>
                    <skip>${japicmp.skip}</skip>
                    <oldVersion>
                        <dependency>
                            <groupId>${previous.release.groupId}</groupId>
                            <artifactId>${previous.release.artifactId}</artifactId>
                            <version>${previous.release.version}</version>
                            <type>${previous.release.type}</type>
                        </dependency>
                    </oldVersion>
                    <newVersion>
                        <file>
                            <path>${project.build.directory}/${project.artifactId}-${project.version}.${project.packaging}</path>
                        </file>
                    </newVersion>
                    <parameter>
                        <onlyModified>true</onlyModified>
                        <includeSynthetic>false</includeSynthetic>
                        <breakBuildOnBinaryIncompatibleModifications>${japicmp.break.build}</breakBuildOnBinaryIncompatibleModifications>
                        <breakBuildBasedOnSemanticVersioning>false</breakBuildBasedOnSemanticVersioning>
                        <onlyBinaryIncompatible>false</onlyBinaryIncompatible>
                        <ignoreMissingClasses>true</ignoreMissingClasses>
                        <skipPomModules>true</skipPomModules>
                        <noAnnotations>false</noAnnotations>
                        <ignoreNonResolvableArtifacts>true</ignoreNonResolvableArtifacts>
                        <ignoreMissingOldVersion>true</ignoreMissingOldVersion>
                        <ignoreMissingNewVersion>true</ignoreMissingNewVersion>
                        <packagingSupporteds>
                            <packagingSupported>${japicmp.plugin.packaging}</packagingSupported>
                        </packagingSupporteds>
                        <reportOnlyFilename>false</reportOnlyFilename>
                        <skipXmlReport>true</skipXmlReport>
                        <skipHtmlReport>false</skipHtmlReport>
                        <skipDiffReport>true</skipDiffReport>
                    </parameter>
                </configuration>
            </plugin>

Any thoughts to where things may be going wrong??

siom79 commented 5 years ago

No, tracking of values is not supported as it does not break compatibility. You can still use the new version of your library if you change the value of the string. It will show of course a different behaviour.

Gareth Filer notifications@github.com schrieb am Mi., 17. Apr. 2019, 10:44:

I have tried to get this working with 0.13.1 and 0.14.0 but having no luck. I have a class with a number of constants defined.

`public class ClassWithStaticConstants {

private ClassWithStaticConstants() { }

public static final String CONSTANT_A = "constant.a"; public static final String CONSTANT_B = "constant.b"; public static final String CONSTANT_C = "constant.c"; public static final String CONSTANT_D = "constant.d";

}`

If I change the string in constant CONSTANT_A to "consistent.a" then I would expect this change to be picked up in the report. Unfortunately it doesn't. If I change the constant definition from CONSTANT_A to CONSISTENT_A then the report shows the expected values.

Are there any configuration values that may be incorrect? I have pasted the full plugin definition below

1.9.44 ${project.groupId} ${project.artifactId} ${project.packaging} 0.14.0 false false war com.github.siom79.japicmp japicmp-maven-plugin ${japicmp.plugin.version} verify cmp ${japicmp.skip} ${previous.release.groupId} ${previous.release.artifactId} ${previous.release.version} ${previous.release.type} ${project.build.directory}/${project.artifactId}-${project.version}.${project.packaging} true false ${japicmp.break.build} false false true true false true true true ${japicmp.plugin.packaging} false true false true Any thoughts to where things may be going wrong?? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub , or mute the thread .
garethfiler commented 5 years ago

Thanks @siom79 for your response. I agree this does not break compatibility during normal usage but I do have a case where the String value would cause a breakage e.g. if the String was defining a Camel route and that route could potentially have references to that name in another component.

If I did want to extend the existing rules and customise japicmp is there any possible extension points you could recommend e.g. provide a custom comparator. Looking at ClassesComparator this is where the decision is made to mark the class as MODIFIED or UNCHANGED and highlighting the member/method that has changed. From looking at the code it looks like JarArchiveComparatorOptions provides filters but not 100% sure if a Filter would be the right choice?

Marcono1234 commented 2 years ago

No, tracking of values is not supported as it does not break compatibility. You can still use the new version of your library if you change the value of the string. It will show of course a different behaviour.

For compile time constants shown in this issue the problem is that the compiler inlines their values, as describe in the language specification:

A reference to a field that is a constant variable (§4.12.4) must be resolved at compile time to the value V denoted by the constant variable's initializer.

(JLS 17 §13.1)

If a field is a constant variable (§4.12.4), and moreover is static, then deleting the keyword final or changing its value will not break compatibility with pre-existing binaries by causing them not to run, but they will not see any new value for a usage of the field unless they are recompiled.

(JLS 17 §13.4.9)