Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

Making GAE_PYTHON optional in tox config. #415

Merged
merged 1 commit into from
Feb 19, 2016

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Feb 17, 2016

Fallback is ${GIT_ROOT}/google_appengine.

The goal is to also make these pull the latest version of the SDK (right now it is downloaded once and frozen in time).

@theacodes
Copy link
Contributor

What was the issue with these commands being in tox, why did we have to move them to their own scripts?

@dhermes
Copy link
Contributor Author

dhermes commented Feb 17, 2016

The issue is that GAE_PYTHONPATH is a requirement, rather than an optional flag that can be used to over-ride. Just running tox is not possible in a "clean" environment.

@theacodes
Copy link
Contributor

You can make it optional by changing the substitution at line 40 from {env:GAE_PYTHONPATH} to {env:GAE_PYTHONPATH:/default/path}.

@theacodes
Copy link
Contributor

(as an aside, I love tox but hate using an ini file for configuration)

@dhermes dhermes mentioned this pull request Feb 17, 2016
@dhermes
Copy link
Contributor Author

dhermes commented Feb 17, 2016

@jonparrott I am aware of the default via {env:VARNAME:DEFAULT_VAL} but

  • was unclear on how to make this path relative to the root of the git repo
  • am "future-proofing" by giving us a place to manually update the SDK when needed

@theacodes
Copy link
Contributor

Fair enough, just wanted to make you aware of it. I'm fine with scripts as long as @nathanielmanistaatgoogle is fine with it.

Also, in terms of the GAE SDK - see https://github.com/GoogleCloudPlatform/python-repo-tools/blob/master/gcp_python_repo_tools/appengine_sdk.py - we're creating this in tandem with re-organizing the tests over at python-docs-samples. Eventually we should use that here to keep the SDK up-to-date (once I add updating functionality to it).

@dhermes
Copy link
Contributor Author

dhermes commented Feb 17, 2016

Nice! I'm very happy to let you handle it.

@nathanielmanistaatgoogle
Copy link
Contributor

As for my own being fine with scripts: uh... I really don't want to have to be. Look at the way this change adds so much more than it removes. How backed-into-a-corner are we?

@dhermes dhermes changed the title Removing tox reliance on GAE_PYTHON. Making GAE_PYTHON optional in tox config. Feb 17, 2016
@dhermes
Copy link
Contributor Author

dhermes commented Feb 17, 2016

OK I nuked the scripts. There should still be some unification done in build_docs.sh and GAE_PYTHONPATH should be used with fetch_gae_sdk / update_gae_sdk in a consistent way.

Also using --gae-lib-root instead of setting the PYTHONPATH.
@@ -111,11 +111,10 @@ deps = {[testenv]basedeps}
commands =
nosetests \
--with-gae \
--gae-lib-root={env:GAE_PYTHONPATH:google_appengine} \

This comment was marked as spam.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 19, 2016

@jonparrott PTAL. It turns out a { inside of a { is a no-no. I tried {env:GAE_PYTHONPATH:{toxinidir}/google_appengine} and it failed in strange ways.

UPDATE: FWIW tox -e gae works perfectly fine from ${GIT_ROOT} or from ${GIT_ROOT}/tests or any other subdirector, meaning that the relative path is relative to {toxinidir} anyhow, so no-harm no-foul.

@theacodes
Copy link
Contributor

Yeah toxinidir is almost always cwd, unless you explicitly change it.

@theacodes
Copy link
Contributor

This LGTM.

dhermes added a commit that referenced this pull request Feb 19, 2016
Making GAE_PYTHON optional in tox config.
@dhermes dhermes merged commit a16a478 into googleapis:master Feb 19, 2016
@dhermes dhermes deleted the tox-remove-gae-python branch February 19, 2016 17:42
@dhermes dhermes mentioned this pull request Feb 19, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants