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

Add FileMap options to put_filemap() Fix function. #266

Merged
merged 6 commits into from
Nov 18, 2022

Conversation

blackwinter
Copy link
Member

WIP until a stable metafacture-core release artifact becomes available.

Resolves #265.

@blackwinter
Copy link
Member Author

blackwinter commented Oct 20, 2022

There seems to be an issue with the newly built metafacture-core artifacts (see metafacture/metafacture-core#473).

All integration tests are failing:

SLF4J: Class path contains multiple SLF4J bindings.
SLF4J: Found binding in [jar:file:~/.gradle/caches/modules-2/files-2.1/org.slf4j/slf4j-log4j12/1.7.21/7238b064d1aba20da2ac03217d700d91e02460fa/slf4j-log4j12-1.7.21.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: Found binding in [jar:file:~/.gradle/caches/modules-2/files-2.1/org.slf4j/slf4j-simple/1.7.21/be4b3c560a37e69b6c58278116740db28832232c/slf4j-simple-1.7.21.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation.
SLF4J: Actual binding is of type [org.slf4j.impl.Log4jLoggerFactory]
Exception in thread "main" org.metafacture.commons.reflection.ReflectionException: class could not be instantiated: class org.metafacture.metafix.Metafix
        at org.metafacture.commons.reflection.ConfigurableClass.newInstance(ConfigurableClass.java:149)
        at org.metafacture.commons.reflection.ObjectFactory.newInstance(ObjectFactory.java:105)
        at org.metafacture.flux.parser.FluxProgramm.createElement(FluxProgramm.java:78)
        at org.metafacture.flux.parser.FluxProgramm.addElement(FluxProgramm.java:90)
        at org.metafacture.flux.parser.FlowBuilder.pipe(FlowBuilder.java:736)
        at org.metafacture.flux.parser.FlowBuilder.flowtail(FlowBuilder.java:514)
        at org.metafacture.flux.parser.FlowBuilder.flow(FlowBuilder.java:226)
        at org.metafacture.flux.parser.FlowBuilder.flux(FlowBuilder.java:122)
        at org.metafacture.flux.FluxCompiler.compileFlow(FluxCompiler.java:66)
        at org.metafacture.flux.FluxCompiler.compile(FluxCompiler.java:54)
        at org.metafacture.runner.Flux.main(Flux.java:85)
Caused by: java.lang.reflect.InvocationTargetException
        at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
        at org.metafacture.commons.reflection.ConfigurableClass.newInstance(ConfigurableClass.java:144)
        ... 10 more
Caused by: java.lang.NoSuchFieldError: EOF_TOKEN
        at org.eclipse.xtext.parser.antlr.Lexer.nextToken(Lexer.java:60)
        at org.antlr.runtime.BufferedTokenStream.fetch(BufferedTokenStream.java:143)
        at org.antlr.runtime.BufferedTokenStream.sync(BufferedTokenStream.java:137)
        at org.antlr.runtime.BufferedTokenStream.fill(BufferedTokenStream.java:286)
        at org.antlr.runtime.BufferedTokenStream.toString(BufferedTokenStream.java:251)
        at org.eclipse.xtext.parser.antlr.AbstractInternalAntlrParser.parse(AbstractInternalAntlrParser.java:586)
        at org.eclipse.xtext.parser.antlr.AbstractAntlrParser.doParse(AbstractAntlrParser.java:103)
        at org.eclipse.xtext.parser.antlr.AbstractAntlrParser.parse(AbstractAntlrParser.java:85)
        at org.eclipse.xtext.parser.antlr.AbstractAntlrParser.doParse(AbstractAntlrParser.java:63)
        at org.eclipse.xtext.parser.AbstractParser.parse(AbstractParser.java:34)
        at org.eclipse.xtext.resource.XtextResource.doLoad(XtextResource.java:178)
        at org.eclipse.xtext.linking.lazy.LazyLinkingResource.doLoad(LazyLinkingResource.java:115)
        at org.eclipse.emf.ecore.resource.impl.ResourceImpl.load(ResourceImpl.java:1563)
        at org.eclipse.emf.ecore.resource.impl.ResourceImpl.load(ResourceImpl.java:1342)
        at org.eclipse.xtext.resource.persistence.StorageAwareResource.load(StorageAwareResource.java:64)
        at org.eclipse.emf.ecore.resource.impl.ResourceSetImpl.demandLoad(ResourceSetImpl.java:259)
        at org.eclipse.emf.ecore.resource.impl.ResourceSetImpl.demandLoadHelper(ResourceSetImpl.java:274)
        at org.eclipse.xtext.resource.XtextResourceSet.getResource(XtextResourceSet.java:266)
        at org.eclipse.xtext.resource.SynchronizedXtextResourceSet.getResource(SynchronizedXtextResourceSet.java:33)
        at org.metafacture.metafix.validation.XtextValidator.getResource(XtextValidator.java:63)
        at org.metafacture.metafix.validation.XtextValidator.getValidatedResource(XtextValidator.java:67)
        at org.metafacture.metafix.FixStandaloneSetup.parseFix(FixStandaloneSetup.java:31)
        at org.metafacture.metafix.Metafix.lambda$getRecordTransformer$0(Metafix.java:162)
        at java.util.HashMap.computeIfAbsent(HashMap.java:1128)
        at org.metafacture.metafix.Metafix.getRecordTransformer(Metafix.java:162)
        at org.metafacture.metafix.Metafix.<init>(Metafix.java:111)
        ... 15 more

FAILURE: Build failed with an exception.

@blackwinter
Copy link
Member Author

Note that we've seen this error before in #90.

@dr0i
Copy link
Member

dr0i commented Oct 21, 2022

Seems similar to hbz/lobid-resources#1462 - a conflict between different xtext libraries.

@blackwinter
Copy link
Member Author

Seems similar to hbz/lobid-resources#1462 - a conflict between different xtext libraries.

How do you figure?

@dr0i
Copy link
Member

dr0i commented Oct 21, 2022

Because of your error message (how did you get this , btw - I only see that it fails). It's said there "NoSuchFieldError" and that reminded me. Also, locally build and consumed mf-master works, and metafacture-flux (which uses ANTLR) has the same bytes on github-packages as the locally one. But this is only a "smell", no really proof, so this may be misleading.

@blackwinter
Copy link
Member Author

It's said there "NoSuchFieldError" and that reminded me.

True, but it's a different field. EOF_TOKEN comes from ANTLR, not Xtext. As I said, it's the same error as in #90.

@blackwinter
Copy link
Member Author

how did you get this , btw - I only see that it fails

Ran it locally. Maybe we should change the integration tests to print errors to the console in CI.

@dr0i
Copy link
Member

dr0i commented Oct 21, 2022

Yeah but xtext uses ANTLR. I am unsure - is there no connection?

@blackwinter
Copy link
Member Author

Yeah but xtext uses ANTLR. I am unsure - is there no connection?

I don't know. Since we weren't able to identify the actual root cause in hbz/lobid-resources#1462 or any cause in #90, I really can't say. It's clearly reminiscent of the former (works in one environment, but not the other), so it might make sense to investigate in that direction.

@blackwinter
Copy link
Member Author

Maybe we should change the integration tests to print errors to the console in CI.

Done in 9ee2194.

dr0i added a commit that referenced this pull request Nov 11, 2022
This resolves "Caused by: java.lang.NoSuchFieldError: EOF_TOKEN
        at org.eclipse.xtext.parser.antlr.Lexer.nextToken(Lexer.java:60)"
so that integration tests passes again.
@dr0i
Copy link
Member

dr0i commented Nov 11, 2022

@blackwinter : updated to a "stable" 5.4.1-rc1 and force psuhed. Rebased. Fixed the integration test error. You may want take over?

@dr0i dr0i assigned blackwinter and unassigned dr0i Nov 11, 2022
@blackwinter blackwinter marked this pull request as ready for review November 14, 2022 10:04
@blackwinter
Copy link
Member Author

Thanks @dr0i. I can't say I particularly like the version pinning, but let's just move on I guess...

Ready for review.

@blackwinter blackwinter assigned dr0i and unassigned blackwinter and TobiasNx Nov 14, 2022
@blackwinter blackwinter removed the request for review from TobiasNx November 14, 2022 12:49
@dr0i
Copy link
Member

dr0i commented Nov 14, 2022

Thx for correcting the commit.

I can't say I particularly like the version pinning

Why?The underlying problem seems to be a conflicting dependency , i.e. the erratic choosing of an one of the two antrl libraries in the classpath. Pinning the correct version in metafix-runner makes this a stable solution, no? Resp. is there a better solution?

@dr0i dr0i assigned blackwinter and unassigned dr0i Nov 14, 2022
@blackwinter
Copy link
Member Author

Because it's non-obvious (to me, at least). No dependencies have changed between 5.4.0 and 5.4.1-rc1, only the publishing process. I don't see anything erratic about it either, it's consistently selecting the wrong version - but only in the runner (integration tests), not in the library (unit tests). I don't understand it, pure and simple.

@dr0i
Copy link
Member

dr0i commented Nov 15, 2022

From https://stackoverflow.com/questions/24962607/multiple-versions-of-the-same-dependency-in-maven :

Note that if two dependency versions are at the same depth in the dependency tree, the first declaration wins.

My understanding (may be wrong - but "explains" the phenomenon):
What has changed is the packaging (done as GitHub packages). Assumption: The build of these packages changes
the order of the dependency tree (why and how I also don't understand, admittedly). Thus, consuming the GitHub packages results in picking versionA while in local builds versionB is choosen.

@blackwinter
Copy link
Member Author

Thus, consuming the GitHub packages results in picking versionA while in local builds versionB is choosen.

Both metafix (unit tests) and metafix-runner (integration tests) consume the same package do they not? And integration tests fail(ed) both locally and in CI.

@dr0i
Copy link
Member

dr0i commented Nov 15, 2022

Oh, yes, you are correct.
So, let's compare the dependency-tree:

git reset --hard 4976755d3a2182d71059c38ea34fdab5347bc4ec # where everything was fine
./gradlew metafix-runner:dependencies > before266_allfine.txt
git reset --hard 6d82e08df665ffd5b90a8aba86dc5aa446267c63 # where things broke
./gradlew metafix-runner:dependencies > 255_broken.txt
diff 255_broken.txt before266_allfine.txt

in 255_broken.txt there is:

[...]
runtimeClasspath - Runtime classpath of source set 'main'.
+--- org.eclipse.xtext:xtext-dev-bom:2.26.0
[...]
| +--- org.antlr:antlr-runtime:3.2 -> 3.5.2 (c)
[...]

while in before266_allfine.txt there is:

| +--- org.antlr:antlr-runtime:3.2 (c)

So: why is there "3.5.2 (c)" using GithubPackages?

@dr0i
Copy link
Member

dr0i commented Nov 15, 2022

@blackwinter can we merge this and resolve #265 ?
We could further investigate the version problem here or open another issue, if you want.

@blackwinter
Copy link
Member Author

can we merge this and resolve #265 ?

@TobiasNx hasn't finished his functional review.

@blackwinter
Copy link
Member Author

blackwinter commented Nov 15, 2022

We could further investigate the version problem here or open another issue, if you want.

Yes. We should probably do it in a dedicated issue, but I'm not sure what the right angle would be (I so desperately want Metafacture's Gradle build cleaned up, and also updated to a current Gradle version). And seeing as we already started here, let me just post another observation:

The new publishing process (via maven-publish plugin, e.g. publishToMavenLocal; or to only generate the Maven POM files: generatePomFileForMavenArtifactsPublication) includes metafacture-flux's antlr dependency (via antlr plugin), while the old process (via maven plugin, e.g. uploadArchives) did not.

--- metafacture-flux-uploadArchives-pom.xml 2022-11-15 14:39:31.375148909 +0100
+++ metafacture-flux-publishToMavenLocal-pom.xml  2022-11-15 14:39:15.143648173 +0100
@@ -63,6 +68,12 @@
     <url>https://github.com/metafacture/metafacture-core/actions</url>
   </ciManagement>
   <dependencies>
+    <dependency>
+      <groupId>org.antlr</groupId>
+      <artifactId>antlr</artifactId>
+      <version>3.5.2</version>
+      <scope>compile</scope>
+    </dependency>
     <dependency>
       <groupId>org.metafacture</groupId>
       <artifactId>metafacture-framework</artifactId>
@@ -81,17 +92,5 @@
       <version>master-SNAPSHOT</version>
       <scope>runtime</scope>
     </dependency>
-    <dependency>
-      <groupId>junit</groupId>
-      <artifactId>junit</artifactId>
-      <version>4.12</version>
-      <scope>test</scope>
-    </dependency>
-    <dependency>
-      <groupId>org.metafacture</groupId>
-      <artifactId>metafacture-plumbing</artifactId>
-      <version>master-SNAPSHOT</version>
-      <scope>test</scope>
-    </dependency>
   </dependencies>
 </project>

NOTE: I added the Maven project description to the new POM in order to reduce noise; but we should probably do it anyway if we want to preserve this information. [Done in metafacture/metafacture-core@d9a47e4.]

@dr0i
Copy link
Member

dr0i commented Nov 15, 2022

Ah, that explains it!

@blackwinter
Copy link
Member Author

Ah, that explains it!

Well, not exactly. We still don't know why... (need to look into the antlr plugin next; maybe some dependency mapping issue: maven vs. maven-publish).

@TobiasNx
Copy link
Collaborator

There are options that are unknown to me. I could add a documentation, if somebody explains their function to me.

compression
decompress_concatenated
encoding
ignore_pattern

@blackwinter
Copy link
Member Author

All options are documented in the source (Javadoc; ignore_pattern hasn't been released yet, though).

@blackwinter
Copy link
Member Author

blackwinter commented Nov 18, 2022

Please note that I already added some lookup() documentation in #269.

@TobiasNx
Copy link
Collaborator

I added some documentation. Reused some of the existing documentation and ignored ignore_pattern.

@TobiasNx
Copy link
Collaborator

Otherwise +1 from me concerning functional review.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@blackwinter
Copy link
Member Author

I added some documentation. [...] Otherwise +1 from me concerning functional review.

Thanks! Can you approve then?

@blackwinter blackwinter merged commit d223a4e into master Nov 18, 2022
@blackwinter blackwinter deleted the 265-filemapOptions branch November 18, 2022 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add options for multi column lookup to put_filemap()
3 participants