SPECIFICATION_VERSION missing in manifest

[vbarua]

Following #406, @ingomueller-net reported issues when updating the consumer-testing library to use

See: de8bf2a#r159771706

It may make sense to revert the changes before the next release.

[nielspardon]

@ingomueller-net can you share a bit more context about your build environment? are you experiencing this locally or in Github Actions?

[nielspardon]

I think I understand the problem. The fix should be relatively easy. Since you are using the isthmus shadow JAR the MANIFEST.MF from the core module gets lost. We can simply add the Specification-Version entries to the shadow JAR MANIFEST and then I would expect it to work.

[nielspardon]

Please still provide some reproduction steps just to be sure I'm looking at the right problem.

[nielspardon]

I submitted a PR which I believe should fix the issue

[ingomueller-net]

[@ingomueller-net](https://github.com/ingomueller-net) can you share a bit more context about your build environment? are you experiencing this locally or in Github Actions?

I am using this script to build this Isthmus. Then I run any of the tests, for example:

pytest -v -m produce_substrait_snapshot --producer=isthmus substrait_consumer/ -k isthmus-function/arithmetic/aggregate/min

This triggers an exception but the exception is caught and compared to the snapshot; you'll probably only see that comparison failing. You can reraise all exceptions in substrait_consumer/functional/common.py of that repo to see them.

[nielspardon]

Good, then I think my PR will fix your issue.

[vbarua]

@ingomueller-net I've merged Niels' PR which should fix your issue hopefully. Let us know if it works (or doesn't).

[ingomueller-net]

Thanks a lot for taking care of this issue promptly! However, it doesn't seem to work yet; I still get the same error. A few things I observed:

• The manifest file is now in the jar and the specification version is set:

  $ jar -xf jars/isthmus-0.58.0-all.jar META-INF/MANIFEST.MF
  $ grep Specification META-INF/MANIFEST.MF 
  Specification-Title: substrait
  Specification-Version: 0.69.0
  

• I am not sure if that was the problem to begin with. getResourceAsStream returns null if no manifest file is found, so if that were the case, we should have gotten an NullPointerException on using the manifest, not on using the value looked up in the manifest. (It could be, it finds some other manifest though that I am not aware of...)
• getValue on the manifest attributes returns null if the attribute wasn't found. That seems to be happening still. Maybe it'd be worth testing that and throwing an exception to improve the error message?

[nielspardon]

• I am not sure if that was the problem to begin with. getResourceAsStream returns null if no manifest file is found, so if that were the case, we should have gotten an NullPointerException on using the manifest, not on using the value looked up in the manifest. (It could be, it finds some other manifest though that I am not aware of...)

ok, this is a bit strange but fine. we can try another workaround.

just FYI the way this is supposed to work is that when the Substrait Java core classes are packaged in a JAR they will never reach the code that uses getResourceAsStream but the first statement of the loadVersion() would have found the value already. The second part was just meant for the Gradle build when the compiled Java classes are not packaged in a JAR:

    private static String[] loadVersion() {
      // load the specification version from the JAR manifest
      String specificationVersion = Version.class.getPackage().getSpecificationVersion();


      // load the manifest directly from the classpath if the specification version is null which is
      // the case if the Version class is not in a JAR, e.g. during the Gradle build
      if (specificationVersion == null) {
        try {
          Manifest manifest =
              new Manifest(
                  Version.class.getClassLoader().getResourceAsStream("META-INF/MANIFEST.MF"));
          specificationVersion = manifest.getMainAttributes().getValue(Name.SPECIFICATION_VERSION);
        } catch (IOException e) {
          throw new IllegalStateException("Could not load version from manifest", e);
        }
      }


      return specificationVersion.split("\\.");
    }
  }

What we can do is that instead of using the MANIFEST.MF we simply create our own resource file and load that with getResourceAsStream all of the time.

[ingomueller-net]

OK, I understand. I can't really help with deciding what's best -- too little Java experience.

I have managed to do some debugging, though: interestingly, the manifest is found and the content I print is the expected one -- and includes the specification version. I am still in the process of finding out why the look-up doesn't work.