File path handling fixes #2280

solussd · 2018-04-24T12:33:17Z

Ensure file paths passed to STGroup[File] are absolute

Fixes AWS Lambda deployments
gracefully handle grammars without source fileNames
Depends on ST4 PR here: loadGroupURL to STGroup and URL constructor to STGroupFile stringtemplate4#199

- gracefully handle grammars without source fileNames

solussd · 2018-04-24T17:57:17Z

I don't understand the CI failure, any insight would be appreciated.

- Changes necessary to support classpath resources in a variety of different deployment scenarios See Antlr4 PR: antlr/antlr4#2280

- absolute file paths not reliable way to find resources on classpath - requires changes to ST4 in this PR: antlr/stringtemplate4#199

parrt · 2018-10-23T15:43:48Z

@solussd looks like we have some build errors

parrt · 2018-11-06T21:43:20Z

@solussd looks like build errors in the Target.java file you changed:

[ERROR] /C:/projects/antlr4/tool/src/org/antlr/v4/codegen/Target.java:[512,50] incompatible types: java.net.URL cannot be converted to java.lang.String
[ERROR] /C:/projects/antlr4/tool/src/org/antlr/v4/codegen/Target.java:[527,50] incompatible types: java.net.URL cannot be converted to java.lang.String

solussd · 2018-11-06T21:53:38Z

@parrt This PR depends on changes here: antlr/stringtemplate4#199

That PR was closed as "fixed" in another PR, but the other PR didn't implement a constructor for STGroupFile that takes a URL.

parrt · 2018-11-06T21:58:36Z

Ah. Can i just add public STGroupFile(URL url) { this(url, "UTF-8",'<', '>'); }?

solussd · 2018-11-06T23:48:50Z

That could be sufficient. I can test tomorrow morning w/ my AWS Lambda use case if you add that constructor.

solussd · 2018-11-06T23:50:12Z

I had rewritten the constructors so they all bottom out in loadGroupURL instead of loadGroupFile.

parrt · 2018-11-07T00:32:37Z

While moving the constructors would mean it uses a different style so we should move those back. I'm trying to build but am being unsuccessful now with later versions of Java. tools.jar apparently no longer exists. damn them! any idea what happened to the Java compiler objects? are they in a public API somewhere now?

solussd · 2018-11-07T21:37:14Z

@parrt I'm not sure, sorry. W.r.t. the constructors, I didn't change the interface to the existing ones, I just consolidated the implementation under the URL one.

ewanmellor · 2018-11-08T22:07:13Z

@parrt I've got a change to fix the tools.jar issue, I'll post that in a little while.

This is in different locations on JDK 9 and up. maven-jdk-tools-wrapper does the right thing on all JDK releases. This was mentioned in a side comment on PR antlr#2280.

parrt · 2018-11-09T00:30:12Z

Actually, it's no problem. I forgot we require java 1.7 so it's gotta be there.

parrt · 2018-11-09T00:40:08Z

Hi @solussd i have added the ctor to ST4: antlr/stringtemplate4@b5a82e0

I hope to release ST4 this weekend so please let me know :) thanks!

This is in different locations on JDK 9 and up. maven-jdk-tools-wrapper does the right thing on all JDK releases. This was mentioned in a side comment on PR antlr#2280.

solussd · 2018-11-13T16:56:58Z

@parrt works with updated ST4. Thanks!

parrt · 2018-11-13T17:06:02Z

@solussd cool. ok, so you still want this merged though right? Can you tell me more about the issue you're solving? Also, shouldn't that ST4 maven ref be 4.1 to get the release version?

solussd · 2018-11-13T17:18:10Z

@parrt yes, this PR. I’ve updated it. Yes, it looks like 4.1 has what I need. Will update in a moment.

The issue was executing on AWS lambda- file paths were being constructed such that some resources were not locateable when the deployment jar was unpacked in the directory AWS Lambda unpacks it.

parrt · 2018-11-13T18:52:14Z

Ok. also i'm not sure you can add others to contrib list. they need to add commits for their own name.

solussd · 2018-11-13T19:01:37Z

@parrt I did that to fix a merge conflict, technically, they were already there (though git might show it as being added in the diff of this PR).

parrt · 2018-11-13T19:02:12Z

ah. thanks

parrt · 2018-11-13T19:08:31Z

tool/src/org/antlr/v4/codegen/Target.java

 		STGroup result = null;
 		try {
-			result = new STGroupFile(groupFileName);
+			result = new STGroupFile(url);


It looks to me as though STGroupFile's ctor calls getURL() which does exactly that getResource thing. What additional functionality are you providing? i.e., where does ST4 go wrong? Maybe we shoulda fixed that in ST4 :)

I believe is related to when it is called. I’d have to dig into it again.

It's also possible the PR you merged into ST4 instead of mine (199) makes this unnecessary.

ah. cool. if you have time, could you find the minimal changeset that works? I'm terrified of changing this stuff in the tool!

Tested without the classloader in ANTLR (instead letting ST4 + getURL do the context class loader lookup) and it works. PR updated to what I believe is a minimal changeset.

- URL constructor unnecessary

parrt · 2018-11-13T21:01:55Z

tool/src/org/antlr/v4/Tool.java

@@ -397,7 +397,7 @@ public void processNonCombinedGrammar(Grammar g, boolean gencode) {

 		if ( generate_ATN_dot ) generateATNs(g);

-		if ( g.tool.getNumErrors()==0 ) generateInterpreterData(g);
+		if (gencode && g.tool.getNumErrors()==0 ) generateInterpreterData(g);


ok, looking good but do we need this tweak for gencode? I'm worried someone might rely on it.

This seems like a safe change to me– code is still generated when executed from the cli, but won't generate code when executed programmatically. I think there was an issue with it writing files in a readonly environment (e.g., deployed as a Lambda).

ok, sounds good. thanks for your perseverance and hard work on this!

Thanks for looking at it!

This is in different locations on JDK 9 and up. maven-jdk-tools-wrapper does the right thing on all JDK releases. This was mentioned in a side comment on PR antlr#2280.

Fixes file/path open problem on AWS lambda, see: antlr/antlr4#2280 antlr/antlr4@11d6013 Needed by an open Lacinia issue: walmartlabs/lacinia#175 (comment)

solussd added 3 commits April 23, 2018 18:55

Ensure file paths passed to STGroup[File] are absolute

8ce79ee

- gracefully handle grammars without source fileNames

revert groupId change

6469934

sign contributors agreement

7e93bf3

solussd mentioned this pull request Apr 24, 2018

Fails to load when used in AWS Lambda function walmartlabs/lacinia#175

Closed

solussd mentioned this pull request Apr 24, 2018

loadGroupURL to STGroup and URL constructor to STGroupFile antlr/stringtemplate4#199

Closed

solussd and others added 3 commits April 24, 2018 17:43

Initialize STGroupFile with url, not file path

7e89f26

- absolute file paths not reliable way to find resources on classpath - requires changes to ST4 in this PR: antlr/stringtemplate4#199

Don't generate interpreter data files if gencode is false

2daa261

Merge branch 'master' into master

696f9b9

parrt added comp:tool type:improvement labels Oct 23, 2018

Merge branch 'master' into master

25f178b

update st4 dep

6899cc5

Merge branch 'master' into master

61d8aea

Use release version of st4

7fcadaa

parrt reviewed Nov 13, 2018

View reviewed changes

Use STGroupFile filename constructor

6f057db

- URL constructor unnecessary

parrt reviewed Nov 13, 2018

View reviewed changes

parrt added this to the 4.7.2 milestone Nov 13, 2018

parrt merged commit 11d6013 into antlr:master Nov 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File path handling fixes #2280

File path handling fixes #2280

solussd commented Apr 24, 2018 •

edited

Loading

solussd commented Apr 24, 2018

parrt commented Oct 23, 2018

parrt commented Nov 6, 2018

solussd commented Nov 6, 2018

parrt commented Nov 6, 2018

solussd commented Nov 6, 2018

solussd commented Nov 6, 2018

parrt commented Nov 7, 2018

solussd commented Nov 7, 2018

ewanmellor commented Nov 8, 2018

parrt commented Nov 9, 2018

parrt commented Nov 9, 2018

solussd commented Nov 13, 2018

parrt commented Nov 13, 2018

solussd commented Nov 13, 2018 •

edited

Loading

parrt commented Nov 13, 2018

solussd commented Nov 13, 2018

parrt commented Nov 13, 2018

parrt Nov 13, 2018

solussd Nov 13, 2018

solussd Nov 13, 2018

parrt Nov 13, 2018

solussd Nov 13, 2018

parrt Nov 13, 2018

solussd Nov 13, 2018 •

edited

Loading

parrt Nov 13, 2018

solussd Nov 13, 2018

File path handling fixes #2280

File path handling fixes #2280

Conversation

solussd commented Apr 24, 2018 • edited Loading

solussd commented Apr 24, 2018

parrt commented Oct 23, 2018

parrt commented Nov 6, 2018

solussd commented Nov 6, 2018

parrt commented Nov 6, 2018

solussd commented Nov 6, 2018

solussd commented Nov 6, 2018

parrt commented Nov 7, 2018

solussd commented Nov 7, 2018

ewanmellor commented Nov 8, 2018

parrt commented Nov 9, 2018

parrt commented Nov 9, 2018

solussd commented Nov 13, 2018

parrt commented Nov 13, 2018

solussd commented Nov 13, 2018 • edited Loading

parrt commented Nov 13, 2018

solussd commented Nov 13, 2018

parrt commented Nov 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solussd Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solussd commented Apr 24, 2018 •

edited

Loading

solussd commented Nov 13, 2018 •

edited

Loading

solussd Nov 13, 2018 •

edited

Loading