Skip to content

Conversation

@aru-d-at
Copy link
Contributor

  • ANTLR v4.9.2 -> v4.11.1

  • Go Dependencies

  • Parser README

### TESTING Steps:

  • Run go get -u ./... -> SUCCESS
  • Run go test ./... -> SUCCESS - Output

@jrgemignani
Copy link
Contributor

@aru-d-at @dehowef Do we need to have the generated files on github?

@aru-d-at
Copy link
Contributor Author

aru-d-at commented Feb 25, 2023

@jrgemignani

Did you mean the interface related files inside the parser folder? I had already removed the unneeded generated *.token and *.interp files prior to this commit.

@dehowef
Copy link
Member

dehowef commented Feb 27, 2023

@jrgemignani @aru-d-at On the ANTLR modification for the python driver, I asked Arun to remove the generated files from the commit-- I think he would apply similar logic to this one as well. As far as I can see, there are no generated files. Looks good to me.

@jrgemignani
Copy link
Contributor

@jrgemignani @aru-d-at On the ANTLR modification for the python driver, I asked Arun to remove the generated files from the commit-- I think he would apply similar logic to this one as well. As far as I can see, there are no generated files. Looks good to me.

I'm referring to the age_*.go files in golang/parser. All of them are generated by Age.g4 it looks like. We can probably remove them all.

@dehowef
Copy link
Member

dehowef commented Feb 28, 2023

@jrgemignani @aru-d-at On the ANTLR modification for the python driver, I asked Arun to remove the generated files from the commit-- I think he would apply similar logic to this one as well. As far as I can see, there are no generated files. Looks good to me.

I'm referring to the age_*.go files in golang/parser. All of them are generated by Age.g4 it looks like. We can probably remove them all.

@aru-d-at can you go and see if it is possible to remove these files from the repository? Thank you

@aru-d-at
Copy link
Contributor Author

aru-d-at commented Feb 28, 2023

@jrgemignani @aru-d-at On the ANTLR modification for the python driver, I asked Arun to remove the generated files from the commit-- I think he would apply similar logic to this one as well. As far as I can see, there are no generated files. Looks good to me.

I'm referring to the age_*.go files in golang/parser. All of them are generated by Age.g4 it looks like. We can probably remove them all.

@aru-d-at can you go and see if it is possible to remove these files from the repository? Thank you

@dehowef @jrgemignani Various functions and interfaces from the 'parser' package are used in the 'builder.go' and 'mapper.go'. These are just some of the ones in first few lines.

Seems like it is necessary to keep these functions and interfaces as well as it dependencies located inside the Parser package.

  • Parser
    image
    image

  • Age (Mapper & Builder)
    image
    image

@jrgemignani
Copy link
Contributor

jrgemignani commented Mar 1, 2023

@aru-d-at I just checked and they [.go files in parser] are generated by antlr4. However, we don't provide antlr4 in our package, just the runtime. It looks like if those files aren't there, it would be a pain for anyone to regenerate them manually. So, ignore my concerns about the .go files in parser.

@aru-d-at
Copy link
Contributor Author

aru-d-at commented Mar 1, 2023

@jrgemignani @dehowef So in that case all good for the merge?

@jrgemignani
Copy link
Contributor

Hmmmm,... did you regenerate these files or just edit them?

image

@aru-d-at
Copy link
Contributor Author

aru-d-at commented Mar 2, 2023

@jrgemignani All parser files were generated then licenses were edited in. I needed to edit the antlr import statement to reflect newer import path for v4.11.1 for mapper and builder inside age folder.

@jrgemignani
Copy link
Contributor

@jrgemignani All parser files were generated then licenses were edited in. I needed to edit the antlr import statement to reflect newer import path for v4.11.1 for mapper and builder inside age folder.

Is it possible to add something to the go driver to automatically build these files or something in a README explaining how to build them so that we don't need to include the generated files in the distribution?

@aru-d-at
Copy link
Contributor Author

aru-d-at commented Mar 21, 2023

@jrgemignani I have this Batch script ready for Windows and have tested it.

I also have the Bash script ready and tested on Linux but, haven't found a way to test it on Mac yet.

Should I go ahead and commit them?

Batch script

@echo off

rem Install Java Development Kit (JDK)
echo Installing JDK...
if not exist "%ProgramFiles%\Java\jdk-19\" (
  mkdir C:\temp
  cd C:\temp
  powershell Invoke-WebRequest -Uri "https://download.oracle.com/java/19/archive/jdk-19.0.2_windows-x64_bin.exe" -OutFile jdk-19.0.2_windows-x64_bin.exe
  start /wait jdk-19.0.2_windows-x64_bin.exe /s ADDLOCAL="ToolsFeature" /s
  del /f jdk-19.0.2_windows-x64_bin.exe
  setx /M JAVA_HOME "C:\Program Files\Java\jdk-19.0.2"
  setx /M PATH "%PATH%;%JAVA_HOME%\bin"
) else (
  echo JDK already installed.
)

rem Download and install ANTLR
echo Downloading ANTLR...
if not exist "%ProgramFiles%\ANTLR" (
  mkdir "%ProgramFiles%\ANTLR"
  cd "%ProgramFiles%\ANTLR"
  powershell Invoke-WebRequest -Uri "https://www.antlr.org/download/antlr-4.11.1-complete.jar" -OutFile "antlr-4.11.1-complete.jar"
  setx /M PATH "%PATH%;%ProgramFiles%\ANTLR\"
  setx /M CLASSPATH ".;%ProgramFiles%\ANTLR\antlr-4.11.1-complete.jar;%CLASSPATH%"
)

echo ANTLR installation complete.

rem Checking Compatablity for Golang
echo Checking if current version of Golang >= Go 1.18.....
set "minimum_version=1.18"
set "installed_version="
set "download_url=https://go.dev/dl/go1.19.7.windows-amd64.msi"

:: Check if Go is installed and get its version
set "go_path="
for %%i in (go.exe) do set "go_path=%%~$PATH:i"
if defined go_path (
	for /f "tokens=3" %%v in ('go version 2^>^&1') do set "installed_version=%%v"
)

:: If Go is not installed or the version is less than 1.18, prompt the user to install a new version
if not defined installed_version (
	echo installing Go
	:: Download and install the latest version of Go
	powershell -Command "& {Invoke-WebRequest -Uri "%download_url%" -OutFile '%TEMP%\go-minimum-version.msi'}"
	start /wait msiexec /i "%TEMP%\go-minimum-version.msi"
	for /f "tokens=3" %%v in ('go version 2^>^&1') do set "installed_version=%%v"
) else if "%installed_version%" lss "%minimum_version%" (
    set /p "install_new_version=Go version %minimum_version% or higher is required. Would you like to install the latest version? (y/n)"
    if /i "%install_new_version%"=="y" (
        :: Download and install the latest version of Go
        powershell -Command "& {Invoke-WebRequest -Uri "%download_url%" -OutFile '%TEMP%\go-minimum-version.msi'}"
        start /wait msiexec /i "%TEMP%\go-minimum_version.msi"
        for /f "tokens=3" %%v in ('go version 2^>^&1') do set "installed_version=%%v"
    ) else (
        echo Please update Go version before installing driver.
        goto skip
    )
)

rem Installing Driver
echo --^> Generating ANTLR parser ^& lexer ^for Golang%
mkdir parser
copy Age.g4 parser/Age.g4
java org.antlr.v4.Tool -Dlanguage=Go -visitor parser/Age.g4 -o parser/
echo --^> Installing Driver
go get -u ./..
del /f /q /s parser
goto end
:skip
echo Aborted
:end
pause
endlocal

@jrgemignani
Copy link
Contributor

@aru-d-at I believe that a decision on this is up to @dehowef

That being said, I do think that if we can have these files autogenerated on the users machine, then we won't need to track them in the repo. Being that they are autogenerated, we shouldn't track them in the repo.

@dehowef
Copy link
Member

dehowef commented Mar 23, 2023

@aru-d-at go ahead and commit it

@dehowef
Copy link
Member

dehowef commented Mar 23, 2023

@aru-d-at but yeah we should remove as many auto generated files as possible-- there is no need to track those unless absolutely necessary

@aru-d-at aru-d-at force-pushed the golang-antlr-4_11_1 branch from 02c6f72 to 1de90fa Compare March 29, 2023 07:27
@aru-d-at
Copy link
Contributor Author

@dehowef I have pushed the scripts after testing on all 3 OS's

@aru-d-at aru-d-at force-pushed the golang-antlr-4_11_1 branch from 1de90fa to b1e2595 Compare April 15, 2023 08:50
@aru-d-at
Copy link
Contributor Author

@dehowef Seems the Go Workflow needed to be updated as well as a Go Target Generation file.

@aru-d-at aru-d-at force-pushed the golang-antlr-4_11_1 branch from b1e2595 to d5f9d9a Compare April 17, 2023 17:19
@aru-d-at
Copy link
Contributor Author

@dehowef Updated the workflow again since generation was failing as ANTLR jar is non-existent. Hopefully workflow works now. (If it doesn't may need to inspect Java dependency in yml)

@dehowef
Copy link
Member

dehowef commented Apr 18, 2023

@aru-d-at Yeah, it still seems to be failing 2 tests-- this issue has been plaguing us for awhile

@aru-d-at aru-d-at force-pushed the golang-antlr-4_11_1 branch from d5f9d9a to cbca575 Compare April 19, 2023 12:04
@aru-d-at
Copy link
Contributor Author

@dehowef Sorry, my bad, I should have run workflow on a separate repo first.
Should work now.
image

@dehowef
Copy link
Member

dehowef commented Apr 20, 2023

@aru-d-at All checks passed. Great work! I'll merge it now

@dehowef dehowef merged commit f35f343 into apache:master Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants