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

PR: Add pysyntect recipe #11315

Merged
merged 35 commits into from
Jun 3, 2020
Merged

PR: Add pysyntect recipe #11315

merged 35 commits into from
Jun 3, 2020

Conversation

andfoy
Copy link
Contributor

@andfoy andfoy commented Apr 17, 2020

This PR adds pysyntect to conda-forge. Currently, this package depends on Rust nightly, which is not available on conda-forge, however, we don't need it for execution, as we produce a static library based on C.

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml"
  • License file is packaged (see here for an example)
  • Source is from official source
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged)
  • If static libraries are linked in, the license of the static library is packaged.
  • Build number is 0
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details)
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there

@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/pysyntect) and found it was in an excellent condition.

@andfoy
Copy link
Contributor Author

andfoy commented Apr 17, 2020

Pinging @ccordoba12 to ask for his permission to participate on this package

@ccordoba12
Copy link
Contributor

https://community.plotly.com/t/plotly-express-in-spyder-offline-instead-of-iplot/21743

I agree to be part of the maintainers team for this feedstock.

@andfoy
Copy link
Contributor Author

andfoy commented Apr 24, 2020

@ccordoba12, this one is ready at last!

@ccordoba12
Copy link
Contributor

Great!! Thanks a lot for hard work on this one @andfoy!

@conda-forge/staged-recipes, could you help us with this one? This library provides Python bindings for sytenct Rust library for syntax highlighting. That's why the installation process is kind of convoluted.

@mariusvniekerk
Copy link
Member

Wow that's quite involved

@isuruf
Copy link
Member

isuruf commented Apr 24, 2020

How about adding a nightly branch to rust-feedstock and use it from there?

Copy link
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

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

Since this is building a static library using dependencies with licenses like Apache-2.0, you need to distribute the licenses and copyright notices for all of the dependencies.

@andfoy
Copy link
Contributor Author

andfoy commented Apr 24, 2020

How about adding a nightly branch to rust-feedstock and use it from there?

Could you please help us opening a dev (CFEP-05) branch there, it seems that it does not exist right now

@andfoy
Copy link
Contributor Author

andfoy commented Apr 24, 2020

Since this is building a static library using dependencies with licenses like Apache-2.0, you need to distribute the licenses and copyright notices for all of the dependencies.

Syntect is MIT license, should I include it also?

@isuruf
Copy link
Member

isuruf commented Apr 24, 2020

Syntect is MIT license, should I include it also?

That doesn't mean that the static binary is MIT licensed. For eg: pyo3-derive-backend is Apache-2.0. Therefore you need to include the license and copyright of pyo3-derive-backend in this package. Same issue with the 91 other dependencies used. Maybe rust has a way of giving you the licenses and copyrights of all the packages.

@ccordoba12
Copy link
Contributor

@isuruf, could you create a dev branch in the rust feedstock so that @andfoy can create a PR for nightly against it?

@isuruf
Copy link
Member

isuruf commented Apr 24, 2020

@andfoy, can you create a PR against master of rust-feedstock and ask a maintainer to merge it into a new branch?

@isuruf
Copy link
Member

isuruf commented Apr 24, 2020

https://github.com/maghoff/cargo-license-hound might help you with the license issue.

@andfoy
Copy link
Contributor Author

andfoy commented Apr 24, 2020

@andfoy, can you create a PR against master of rust-feedstock and ask a maintainer to merge it into a new branch?

Sure! I'll do it

@conda-forge-linter
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/pysyntect) and found some lint.

Here's what I've got...

For recipes/pysyntect:

  • requirements: build: rust > 0.43.0 should not contain a space between relational operator and the version, i.e. rust >0.43.0

@conda-forge-linter
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/pysyntect) and found some lint.

Here's what I've got...

For recipes/pysyntect:

  • requirements: build: rust>0.43.0 must contain a space between the name and the pin, i.e. rust >0.43.0

@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/pysyntect) and found it was in an excellent condition.

@conda-forge-linter
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/pysyntect) and found some lint.

Here's what I've got...

For recipes/pysyntect:

  • requirements: build: rust>0.43.0 must contain a space between the name and the pin, i.e. rust >0.43.0

@andfoy
Copy link
Contributor Author

andfoy commented May 1, 2020

Can windows dependencies be different?

Is there any conflicting combination?

Co-authored-by: Isuru Fernando <isuruf@gmail.com>
@andfoy
Copy link
Contributor Author

andfoy commented May 4, 2020

@isuruf, any other comment on this one?

@isuruf
Copy link
Member

isuruf commented May 4, 2020

Can windows dependencies be different?

What I meant from this is that we only check that the licenses exist in unix. If the rust dependencies change for windows, then running this check only on unix is not enough.

@andfoy
Copy link
Contributor Author

andfoy commented May 4, 2020

What I meant from this is that we only check that the licenses exist in unix. If the rust dependencies change for windows, then running this check only on unix is not enough.

Sure! Let me check this one as well

@andfoy
Copy link
Contributor Author

andfoy commented May 5, 2020

@isuruf There are no Windows-only packages

@andfoy
Copy link
Contributor Author

andfoy commented May 6, 2020

@isuruf Any update on this one?

@andfoy
Copy link
Contributor Author

andfoy commented May 7, 2020

@conda-forge/staged-recipes All the licenses are up-to-date, does this package is ready to merge?

@ccordoba12
Copy link
Contributor

@andfoy, please squash all commits and leave a single one here.

@isuruf, we need this to be able to integrate pysyntect with Spyder and @andfoy has worked very hard to meet your review. I think this is ready.

@isuruf
Copy link
Member

isuruf commented May 10, 2020

@ccordoba12, I'll need to carefully review the licenses of this PR before I can sign off on this. (For eg: there are Apache licenses without copyright notices. If the package does have those, we need to copy them here. I currently don't have the time to do it and please note that my time here is entirely voluntary.

cc @conda-forge/staged-recipes

@ccordoba12
Copy link
Contributor

I'll need to carefully review the licenses of this PR before I can sign off on this.

Ok, I didn't know that was the main issue here. Are you referring to the licenses of all crates the Rust syntect library depends on?

If that's the case, we could give you a hand with that and list here all those licenses.

For eg: there are Apache licenses without copyright notices

I think @andfoy mentioned all packages licensed as Apache are dual-licensed so that's no longer required. However, I'll double check with him if that's the case or not.

I currently don't have the time to do it and please note that my time here is entirely voluntary.

I understand, and thanks for your time.

@isuruf
Copy link
Member

isuruf commented May 19, 2020

@andfoy, can you confirm that all licenses are dual licensed MIT and Apache? I still see some Apache only licenses.

@andfoy
Copy link
Contributor Author

andfoy commented May 19, 2020

@andfoy, can you confirm that all licenses are dual licensed MIT and Apache? I still see some Apache only licenses.

There are some Apache-only licenses indeed, however, they have their respective copyright notices, as far as I know

@isuruf
Copy link
Member

isuruf commented May 20, 2020

For eg: recipes/pysyntect/syntax_licenses/NSIS.txt doesn't have a copyright notice.

@andfoy
Copy link
Contributor Author

andfoy commented May 20, 2020

For eg: recipes/pysyntect/syntax_licenses/NSIS.txt doesn't have a copyright notice.

The copyright notice for that one in particular is found at the end of the full license: https://github.com/github/linguist/blob/master/vendor/licenses/grammar/NSIS.txt

@andfoy
Copy link
Contributor Author

andfoy commented Jun 3, 2020

@isuruf, any update on this one?

@isuruf isuruf merged commit cc11e23 into conda-forge:master Jun 3, 2020
@isuruf
Copy link
Member

isuruf commented Jun 3, 2020

Thanks

@ccordoba12
Copy link
Contributor

Thanks a lot @isuruf!

@andfoy
Copy link
Contributor Author

andfoy commented Jun 3, 2020

Thanks @isuruf for your help on this one!

@andfoy andfoy deleted the add_pysyntect branch June 3, 2020 22:37
@charliermarsh charliermarsh mentioned this pull request Nov 25, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants