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

director.py: restore READTHEDOCS_VERSION_[TYPE|NAME] #9052

Merged
merged 1 commit into from
Mar 30, 2022

Conversation

jsquyres
Copy link
Contributor

These env vars were accidentally removed in #9002. This commit naively restores the 2 missing env variables in the new get_rtd_env_vars() location.

Signed-off-by: Jeff Squyres jeff@squyres.com

This commit is a potential naïve fix for #9051. I have no good way to test this, but it does restore the 2 env vars that were removed in #9002.

These env vars were accidentally removed in
readthedocs#9002.  This commit
naively restores the 2 missing env variables in the new
get_rtd_env_vars() location.

Signed-off-by: Jeff Squyres <jeff@squyres.com>
@jsquyres jsquyres requested a review from a team as a code owner March 29, 2022 19:48
Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

Thanks, looks like we have a test, but it's marked as skip

@mock.patch("readthedocs.doc_builder.director.load_yaml_config")
@pytest.mark.skip()
# NOTE: find a way to test we are passing all the environment variables to all the commands
def test_get_env_vars_default(self, load_yaml_config):
....

@jsquyres
Copy link
Contributor Author

jsquyres commented Mar 29, 2022

I see that some of the CI failed. It looks like this may be something I just ran across in an entirely different context the other day:

Traceback (most recent call last):
  File "/home/circleci/project/.tox/sphinx-35/bin/pytest", line 8, in <module>
    sys.exit(console_main())
  File "/home/circleci/project/.tox/sphinx-35/lib/python3.8/site-packages/_pytest/config/__init__.py", line 187, in console_main
    code = main()
  File "/home/circleci/project/.tox/sphinx-35/lib/python3.8/site-packages/_pytest/config/__init__.py", line 145, in main
    config = _prepareconfig(args, plugins)
...
  File "/home/circleci/project/.tox/sphinx-35/lib/python3.8/site-packages/sphinx/util/rst.py", line 21, in <module>
    from jinja2 import Environment, environmentfilter
ImportError: cannot import name 'environmentfilter' from 'jinja2' (/home/circleci/project/.tox/sphinx-35/lib/python3.8/site-packages/jinja2/__init__.py)
ERROR: InvocationError for command /home/circleci/project/.tox/sphinx-35/bin/pytest -m embed_api (exited with code 1)

The latest release of Jinja2 -- v3.1.0, released 24 May 2022 -- made a backwards-incompatible change. From https://jinja.palletsprojects.com/en/3.1.x/changes/#version-3-1-0:

  • Remove previously deprecated code. #1544
    • contextfilter and contextfunction are replaced by pass_context. evalcontextfilter and evalcontextfunction are replaced by pass_eval_context. environmentfilter and environmentfunction are replaced by pass_environment.

If your CI environment restricts itself to the previous release of Jinja 2 (v3.0.3), it should work fine.

That being said, I see several places in the code base where you already have jinja2<3.1.0, so I'm not sure where an additional fix would be needed.

@stsewd
Copy link
Member

stsewd commented Mar 29, 2022

@jsquyres the tests from tests-embedapi aren't related to this change and are failing on main, we can fix that later, don't worry about it.

@jsquyres
Copy link
Contributor Author

@jsquyres the tests from tests-embedapi aren't related to this change and are failing on main, we can fix that later, don't worry about it.

Great!

Do you think that this is a sufficient fix for #9051? (I admit that I did a very little bit of grep'ing and git spelunking around the code base to come up with this fix)

If so, is there a way to get this fix into production so that our RTD builds start working again? 😄

Copy link

@gpaulsen gpaulsen left a comment

Choose a reason for hiding this comment

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

I started digging in as well, and saw these were missing.

@stsewd stsewd merged commit 4029353 into readthedocs:main Mar 30, 2022
@jsquyres jsquyres deleted the pr/restore-new-env-vars branch March 30, 2022 15:35
@jsquyres
Copy link
Contributor Author

@stsewd Thank you! How long does it take for this change to get into production?

@stsewd
Copy link
Member

stsewd commented Mar 30, 2022

@jsquyres we will try to deploy this change today

ericholscher pushed a commit that referenced this pull request Mar 30, 2022
These env vars were accidentally removed in
#9002.  This commit
naively restores the 2 missing env variables in the new
get_rtd_env_vars() location.

Signed-off-by: Jeff Squyres <jeff@squyres.com>
ericholscher pushed a commit that referenced this pull request Mar 30, 2022
These env vars were accidentally removed in
#9002.  This commit
naively restores the 2 missing env variables in the new
get_rtd_env_vars() location.

Signed-off-by: Jeff Squyres <jeff@squyres.com>
@ericholscher ericholscher added the PR: hotfix Pull request applied as hotfix to release label Mar 30, 2022
@ericholscher
Copy link
Member

@jsquyres
This should now be fixed, please let me know if it isn't.

@jsquyres
Copy link
Contributor Author

@ericholscher Looks good -- many thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: hotfix Pull request applied as hotfix to release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants