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

THRIFT-5564: add GitHub action for python 2.x and 3.x #2596

Closed
wants to merge 51 commits into from

Conversation

jimexist
Copy link
Member

@jimexist jimexist commented Apr 29, 2022

  • Did you create an Apache Jira ticket? (not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

@jimexist jimexist changed the title add GitHub action for python 2.x and 3.x THRIFT-5564: add GitHub action for python 2.x and 3.x Apr 29, 2022
@jimexist
Copy link
Member Author

jimexist commented May 6, 2022

before merging

@fishy do you want to have a look at this? this might help my work on #2588 as well

@@ -128,3 +128,171 @@ jobs:

- name: Run make kotlin
run: make -C lib/kotlin

lib-rust:
Copy link
Member

Choose a reason for hiding this comment

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

this seems unrelated to what the pr title/description says it does? but since the jira ticket itself covers all languages I don't really see this as a problem :)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes it's actually based on:

but i think we can merge in one pass as well


lib-python:
needs: compiler
runs-on: ubuntu-20.04
Copy link
Member

Choose a reason for hiding this comment

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

should we also make this part of the matrix? my understanding is that this is not the only version of ubuntu we want to support.

Copy link
Member Author

Choose a reason for hiding this comment

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

i'm adding matrix to the compiler part but for python i think it's okay to have just one - given how libraries and compilers are in many cases compiled, installed, and used separately. the matrix should be focusing on multiple python versions for this part.

compiler/cpp/thrift -version

- name: Run make python
run: make -C lib/py
Copy link
Member

Choose a reason for hiding this comment

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

does this run the unit tests or does it only make sure that the code builds?

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks - i added the check below

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants