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

Install Python Dev Dependencies to Virtual Environment #15602

Merged
merged 33 commits into from Jul 21, 2022

Conversation

U8NWXD
Copy link
Member

@U8NWXD U8NWXD commented Jun 19, 2022

Overview

  1. This PR fixes or fixes part of #[n/a].
  2. This PR does the following: Instead of installing Python dev dependencies to oppia_tools, this PR changes our installation code to install them to the user's virtual environment. This lets us pin our transitive dev dependency versions using a requirements_dev.txt file much like we do with a requirements.txt file for our production python dependencies.

Essential Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The linter/Karma presubmit checks have passed locally on your machine.
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
    • This lets reviewers restart your CircleCI tests for you.
  • The PR is made from a branch that's not called "develop".

Proof that changes are correct

I have verified that this PR works correctly by confirming that installing Oppia from scratch works macOS, and I have worked with vojtechjelinek and kevintab95 to test on Linux.

PR Pointers

  • Make sure to follow the instructions for making a code change.
  • If you need a review or an answer to a question, and don't have permissions to assign people, leave a comment like the following: "{{Question/comment}} @{{reviewer_username}} PTAL". Oppiabot will help assign that person for you.
  • For what code owners will expect, see the Code Owner's wiki page.
  • Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays.
  • Never force push. If you do, your PR will be closed.
  • Some of the e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.

Rename it to `install_python_prod_dependencies.py` to make clear that
this script installs production dependencies.
@oppiabot
Copy link

oppiabot bot commented Jun 19, 2022

Hi @U8NWXD, can you complete the following:

  1. The karma and linter checklist has not been checked, please make sure to run the frontend tests and lint tests before pushing.
    Thanks!

@oppiabot
Copy link

oppiabot bot commented Jun 19, 2022

Hi, @U8NWXD, this pull request does not have a "CHANGELOG: ..." label as mentioned in the PR pointers. Assigning @U8NWXD to add the required label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks!

@oppiabot
Copy link

oppiabot bot commented Jun 20, 2022

Hi @U8NWXD, there is a new change in develop which needs to be in your PR. Please update your branch with the latest changes in develop. For instructions, refer to this link. Thanks!

@oppiabot oppiabot bot added the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Jun 21, 2022
@oppiabot
Copy link

oppiabot bot commented Jun 21, 2022

Hi @U8NWXD, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

@oppiabot oppiabot bot removed the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Jun 21, 2022
@oppiabot oppiabot bot added the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Jun 24, 2022
@oppiabot
Copy link

oppiabot bot commented Jun 24, 2022

Hi @U8NWXD, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

@oppiabot
Copy link

oppiabot bot commented Jul 1, 2022

Hi @U8NWXD, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale label Jul 1, 2022
@oppiabot oppiabot bot closed this Jul 5, 2022
@U8NWXD U8NWXD reopened this Jul 11, 2022
@oppiabot
Copy link

oppiabot bot commented Jul 11, 2022

Hi @U8NWXD, can you complete the following:

  1. The karma and linter checklist has not been checked, please make sure to run the frontend tests and lint tests before pushing.
    Thanks!

@oppiabot oppiabot bot removed stale PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. labels Jul 11, 2022
@oppiabot
Copy link

oppiabot bot commented Jul 11, 2022

Hi, @U8NWXD, this pull request does not have a "CHANGELOG: ..." label as mentioned in the PR pointers. Assigning @U8NWXD to add the required label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks!

@U8NWXD
Copy link
Member Author

U8NWXD commented Jul 20, 2022

@seanlip I've addressed your comments. PTAL

@U8NWXD U8NWXD assigned seanlip and unassigned U8NWXD Jul 20, 2022
scripts/common_test.py Outdated Show resolved Hide resolved
scripts/install_python_dev_dependencies.py Outdated Show resolved Hide resolved
@seanlip seanlip assigned U8NWXD and unassigned seanlip Jul 20, 2022
Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Choose a reason for hiding this comment

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

LGTM!

@oppiabot
Copy link

oppiabot bot commented Jul 20, 2022

Unassigning @vojtechjelinek since they have already approved the PR.

@U8NWXD
Copy link
Member Author

U8NWXD commented Jul 20, 2022

@seanlip PTAL

Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

Thanks @U8NWXD! LGTM, just found one typo. Thanks for removing all the import path messiness!

scripts/install_python_dev_dependencies.py Outdated Show resolved Hide resolved
@seanlip seanlip assigned U8NWXD and unassigned seanlip Jul 20, 2022
@oppiabot oppiabot bot added the PR: LGTM label Jul 20, 2022
@oppiabot
Copy link

oppiabot bot commented Jul 20, 2022

Hi @U8NWXD, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

@U8NWXD U8NWXD merged commit 388c5fb into oppia:develop Jul 21, 2022
@U8NWXD U8NWXD deleted the python-dev-deps-venv branch July 21, 2022 02:16
U8NWXD added a commit to U8NWXD/oppia-web-developer-docs that referenced this pull request Jan 11, 2023
gp201 pushed a commit to oppia/oppia-web-developer-docs that referenced this pull request Jan 12, 2023
* Update third party lib docs for oppia/oppia#15602

* Remove lingering reference to regenerate_requirements.py
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.

None yet

6 participants