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.

Sign up for GitHub

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

Jump to bottom

Add antlr-python-runtime #4559

Merged
merged 13 commits into from
Dec 10, 2017
Merged

Add antlr-python-runtime #4559

merged 13 commits into from
Dec 10, 2017

Conversation

bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Dec 9, 2017 •
edited
Loading

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/antlr4, recipes/antlr4-python2-runtime, recipes/antlr4-python3-runtime) and found it was in an excellent condition.

@bollwyvl bollwyvl mentioned this pull request Dec 9, 2017
Merged
@moorepants
Copy link
Contributor

moorepants commented Dec 9, 2017 •
edited
Loading

Is antlr4 versus previous antrl something akin to Python 2 vs Python 3? i.e. non-backward compatiable progression and people still need to use both new and old antlrs?

In conda, I would expect to type:

conda install antlr=4

and

conda install antlr=3

Having this separate package doesn't follow that convention. We can do that with Python, even though Py 2 and 3 have maybe a similar difference.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Dec 9, 2017 •
edited
Loading

My reasoning was that the two:

  • install different commands: antlr vs antlr4
  • have entirely different build infrastructure and architecture: (c++/make vs java/maven)
  • different home pages
  • official, downstreams reference it, a la anltr4-python3-runtime, rather than antlr-python-runtime

Oh, and there's another, incompatible epoch in between... but I'm not itching to even look into that.

So unlike py2/3, there's very little overlap other than use cases between the two... though yes, presumably they are monotonically-increasing versions!

Thoughts, antlr maintainers, @ocefpaf @kwilcox?

moorepants
moorepants reviewed Dec 9, 2017
View reviewed changes
recipes/antlr4-python2-runtime/meta.yaml Outdated
@@ -0,0 +1,42 @@
{% set name = "antlr4-python2-runtime" %}
Copy link
Contributor

@moorepants moorepants Dec 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea why these are two different pypi packages? In conda, we may want to combine them and use the dependency infrastructure to build the correct one. Then you can simply conda install python-antlr to install.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, to grammar people, they are as different as go and javascript! Indeed, we could even build them directly from the upstream repo. I guess since they install the same thing (antlr) to site-packages, that would not be unreasonable.

Also, humorously, antlr 4.7.1 just dropped within the last few minutes:
https://github.com/antlr/antlr4/releases/tag/4.7.1

However, the jar hadn't been updated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be best to ask an antlr developer their recommendations on the packaging. I assume they have dealt with this with some other package managers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears the antlrX (after antlr2) is the most common nomenclature:

This is consistent with what is proposed in this PR.

For the runtimes, at least the debian packages appear to favor python-antlr4 and python3-antlr4. It might indeed make sense to combine them into one name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was thinking the same thing.

Debian doesn't really give a good example because in Debian all packages are split to python and python3.

My preference would be to make a single package. Otherwise any package that depends on antlr will have to have

- antlr4-python2-runtime # [py2k]
- antlr4-python3-runtime # [py3k]

in the dependencies.

On the other hand, the usual way to do things is to match the pypi names, so ¯\_(ツ)_/¯ ‏

Any thoughts @conda-forge/core?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the python runtime, there's no need to have 2 packages.
For antlr, we have 2.7.7 in the feedstock. @conda-forge/antlr might be willing to maintain 2.x and 4.x in 2 separate branches. (Only nco will be affected at build time. antlr is a build time dependency of nco. cc @conda-forge/nco)
@conda-forge/antlr, what do you think?

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/antlr-python-runtime) and found it was in an excellent condition.

@bollwyvl
Copy link
Contributor Author

Ok, merged the two into antlr-python-runtime and removed antlr4 to be taken up over on the antlr-feedstock...

@bollwyvl bollwyvl mentioned this pull request Dec 10, 2017
Merged
@bollwyvl bollwyvl changed the title Add antlr4 and antlr4-python*-runtimes Add antlr-python-runtime Dec 10, 2017
@bollwyvl bollwyvl mentioned this pull request Dec 10, 2017
Merged
@bollwyvl
Copy link
Contributor Author

Huzzah, the 4.7.1 line was officially announced, which moves to setuptools, so had to deal with that. Added some more metadata as well.

@bollwyvl
Copy link
Contributor Author

Oooh, so 💚!

@ocefpaf
Copy link
Member

ocefpaf commented Dec 10, 2017

Merge?

@asmeurer
Copy link
Member

It looks good to me. You can also add me as a maintainer (now or after merging).

@isuruf isuruf merged commit a9a1826 into conda-forge:master Dec 10, 2017
moorepants
moorepants reviewed Dec 10, 2017
View reviewed changes
recipes/antlr-python-runtime/meta.yaml
about:
home: http://www.antlr.org
license: BSD-3-Clause
license_family: BSD
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the license file on a new PR? We missed that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skimmed through the sdists and did not find one. Can you please raise an issue upstream to get the license file included in future sdists?

@jakirkham jakirkham mentioned this pull request Dec 11, 2017
Merged
@jakirkham
Copy link
Member

Looks like conversion ran into a tricky situation with conda-smithy. Have raised as issue ( conda-forge/conda-smithy#626 ). Not sure what the long term fixes is yet.

In the interim, this recipe needs a workaround. Have put together a relatively simplistic one in PR ( #4585 ). It's not ideal (though not too bad IMHO). It seems to re-render fine locally with that change. Expect it should still build fine, but will wait and see.

@jakirkham
Copy link
Member

With that tweak, the recipe has successfully converted to a feedstock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isuruf isuruf isuruf left review comments

@jakirkham jakirkham jakirkham left review comments

@moorepants moorepants moorepants left review comments

@asmeurer asmeurer asmeurer left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bollwyvl @conda-forge-linter @moorepants @ocefpaf @asmeurer @jakirkham @isuruf