christoph2 / pyA2L

ASAM ASAP2 Library for Python
GNU General Public License v2.0
138 stars 71 forks source link

Suggestion: Remove autogenerated code #13

Closed bessman closed 4 years ago

bessman commented 4 years ago

This project currently includes a lot of ANTLR4-generated code under the py3 package. This creates unnecessary code churn when upgrading ANTRL. It may also hide bugs if CI-servers regenerate these files as part of the build process, which AppVeyor does.

I propose that the generation of parsers and lexers be moved to build-time. This would create an build-time dependency on ANTLR. It would not affect existing unit tests, since AppVeyor already does this and Travis is broken anyway.

@christoph2 If you think this sounds like a way forward, I can prepare a pull request.

christoph2 commented 4 years ago

Hmm, I'm not sure... This complicates matters for users: You are required to have a working Java compiler toolchain, the right ANTLR jar in your CLASSPATH and a matching Antlr-Python runtime... P. S. : I've just disabled Appveyor regeneration.

bessman commented 4 years ago

This complicates matters for users: You are required to have a working Java compiler toolchain, the right ANTLR jar in your CLASSPATH and a matching Antlr-Python runtime...

Indeed. A possible solution would be to setup automatic building of wheels with every commit and add them to the pre-release page.

Unless I'm mistaken, the ANTRL Python run-time package is already required.

christoph2 commented 4 years ago

Well, the automatic package build and upload to GH releases (and in the near future to Python package index) actually works -- source builds only, I still have to figure out why there are no wheels, but in this case (no C/C++ extensions) wheels are not really required.

christoph2 commented 4 years ago

CLASSPATH seems to preferred: https://stackoverflow.com/questions/41021963/how-to-install-antlr4 https://www.antlr.org

bessman commented 4 years ago

I'm working on improving the way setup.py finds ANTLR. I'll include looking in CLASSPATH.

christoph2 commented 4 years ago

There still two issues:

bessman commented 4 years ago
* build_py doesn't copy the Antlr artifacts to the now empty py3/ directory.

This is intended. Since python2 is no longer supported, I saw no reason to keep the py2/py3 distinction. build_py copies the artifacts directly into pya2l/.

* build_py should also be triggered by the develop target,  which is important for people working on the project.

Fixed in https://github.com/christoph2/pyA2L/pull/15.

By the way, did you intend to remove a2l_listener.py in your most recent commit? It seems kind of important...

christoph2 commented 4 years ago

a2l_listener is back again... Now I'm trying to figure out what's wrong with appveyor's CLASSPATH.

bessman commented 4 years ago

You are going to need to add -L to the curl call when downloading ANTLR in appveyor.yml. https://www.antlr.org/download/antlr-4.8-complete.jar is a redirect, but without -L curl just downloads the html page instead of following it. Edit: Or perhaps that's just curl on unix. Appveyor did seem to be working earlier.

I'm just as confused as you with regards to the environment variable, though. Could it be that coverage is opening a new shell? Environment variables set via set are shell local.

christoph2 commented 4 years ago

This could be a problem with subprocces, yes. I will replace the coverage stuff with a plain setup.py call.

bessman commented 4 years ago

I tried some stuff in https://github.com/christoph2/pyA2L/pull/17 and the only thing that worked for me was cmd /C "set "CLASSPATH=%APPVEYOR_BUILD_FOLDER%\antlr-4.8-complete.jar" && %PYTHON%\python.exe setup.py test"

It still fails because setup.py test currently does not run ANTLR automatically. I'm thinking to fix that with a unit test which will both cover the AntlrAutogen class and also creates Lexers and Parsers as a side effect.

christoph2 commented 4 years ago

I've found a working makeshift solution, but now there are import errors: https://ci.appveyor.com/project/christoph2/pya2l/builds/31578524/job/7g7gmji3s3l4wps5

bessman commented 4 years ago

That's because the test isn't generating lexers and parsers. I've fixed it locally, but I'm currently out of office. I'll make a pull request in a few hours.

Can you merge https://github.com/christoph2/pyA2L/pull/16 in the meantime?

bessman commented 4 years ago

AppVeyor almost works now. Coveralls just can't find the repo token for some reason?

Edit: Probably because of this: https://help.appveyor.com/discussions/problems/5873-coveralls-environment-variable-not-always-working#comment_41725435

christoph2 commented 4 years ago

I would merge, but unfortunately there are conflicts, so rebase and merge functionality is disabled.

bessman commented 4 years ago

Are you sure? On my end github says "This branch has no conflicts with the base branch". I'll try to do a rebase.

Edit: Rebase done, try it now.