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

fix: Install pip and pip-tools in upgrade script #70

Closed
wants to merge 11 commits into from

Conversation

Jawayria
Copy link

Updated the upgrade target script to check the compatibility of upgraded pip and pip-tools versions. For reference, look at this openedx/edx-repo-health#271 for new check added in edx-repo-health.

@mariajgrimaldi
Copy link
Member

Hello! Can you check the failing tests? they might be related to the libraries update for docs.in

Comment on lines +6 to +9
doc8==0.10.1 # reStructuredText style checker
edx_sphinx_theme==3.0.0 # edX theme for Sphinx output
readme_renderer==30.0 # Validates README.rst for usage on PyPI
Sphinx==4.5.0 # Documentation builder
Copy link
Member

Choose a reason for hiding this comment

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

why don't you add these constraints into the constraints.txt? & justify them please

Copy link
Member

@mariajgrimaldi mariajgrimaldi Aug 2, 2022

Choose a reason for hiding this comment

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

This worked for me in the constraints.txt, then I just get some doc errors but it doesn't crash

doc8==0.10.1
edx_sphinx_theme==3.0.0
Sphinx==4.5.0

This is how I tested after installing tox:
TOXENV=docs tox
But remember to justify the pinning. Do we need it?

Copy link
Author

Choose a reason for hiding this comment

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

I did it to test if new package versions were a problem

Sphinx # Documentation builder
doc8==0.10.1 # reStructuredText style checker
edx_sphinx_theme==3.0.0 # edX theme for Sphinx output
readme_renderer==30.0 # Validates README.rst for usage on PyPI
Copy link
Member

Choose a reason for hiding this comment

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

Try with readme-renderer

@Jawayria
Copy link
Author

Usama Sadiq will carry this forward

Comment on lines 13 to 14
# edX opaque keys Django constraint
django < 3.0, >= 2.2
Copy link
Member

Choose a reason for hiding this comment

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

You should remove this constraint since it's a bit old

@mariajgrimaldi
Copy link
Member

I think I made this work by:

[testenv:docs]
setenv =
    PYTHONPATH = {toxinidir}
    SPHINXOPTS = -W
whitelist_externals =
    make
    rm
    twine
deps =
    -r{toxinidir}/requirements/doc.txt
commands =
    doc8 --ignore-path docs/_build README.rst docs
    rm -f docs/openedx_events.rst
    rm -f docs/modules.rst
    # -e allows for overriding setting from the environment so that SPHINXOPTS gets picked up.
    make -e -C docs clean
    make -e -C docs html
    python setup.py sdist bdist_wheel
    twine check --strict dist/*

Also adding twine to requirements/docs.in
I used this repo as reference

@mariajgrimaldi
Copy link
Member

Hello there @UsamaSadiq! We're eager to push this PR forward with the changes suggested. Is there something we can do? Let me know!

@UsamaSadiq
Copy link
Member

Sure @mariajgrimaldi , I'll finalise the PR with suggested changes within next week and will let you know.

@mariajgrimaldi
Copy link
Member

Hello there! I merged some changes that may fix your CI issue, so you might not need more changes.

@UsamaSadiq
Copy link
Member

@mariajgrimaldi I don't have access to update this PR and Jawayria has changed her team so I've created a new PR with all the suggested changes.
Please close this PR and review the above mentioned PR.

@mariajgrimaldi
Copy link
Member

Got it! @UsamaSadiq, thanks for continuing this!

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

Successfully merging this pull request may close these issues.

3 participants