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

Rework dev dependencies to be Pipfile generated, locked, and hashed. #4891

Merged
merged 3 commits into from Nov 2, 2017

Conversation

Projects
None yet
3 participants
@jmchilton
Copy link
Member

jmchilton commented Oct 27, 2017

Switch to Pipfile based management of dev-requirements and linting requirements (this PR builds on #4876) (merged). In addition to providing better native options for users of pipenv the generated requirements.txt files are superior to previous variants in some ways including:

  • Like #4876, this should prevent linting from failing randomly based on upstream project updates. It will now be simpler than ever to update linting dependencies as we need them. (Same is true for things like Sphinx generation though no that it will be pinned.)
  • We had no mechanism for keeping pinned and hashed requirements in sync - the hashed version had grown very out of date. Likewise the unpinned variant was out of date also - generating everything from a single source Pipefile will keep these different installation mechanisms together.
  • The generated requirement files are more complete - so the locking should be more reproducible and will not break due to our dependencies getting updated.
  • The generated requirement files include Python version requirements as needed - so this should aide in creating requirement files that will work with Python 2 and 3 together. (If indeed we abandon virtualenv and pip for pipenv - there is built in support for handling 2 vs 3 environments that is even better). xref. #1715
  • When we apply this to all of our dependencies in the longer term, this will allow us to separate what we actually depend on and what they depend on. Since we previously tracked this in one big list of pins - we lost this information and also had to track more than we should have needed to manually.

Updated: Rebased against dev to bring in @nsoranzo's bashism bashings in #4876 and fixed up to strip trailing whitespace from generated requirement files. Updated PR description a bit for the merging of #4876 as well.

@jmchilton jmchilton changed the title Rework dev and linting dependencies to use Pipfiles. Rework dev and linting dependencies to use locked dependencies. Oct 27, 2017

@galaxybot galaxybot added the triage label Oct 27, 2017

@galaxybot galaxybot added this to the 18.01 milestone Oct 27, 2017

@jmchilton jmchilton force-pushed the jmchilton:dev_dependencies branch 2 times, most recently from 0c9e1ab to 0df3428 Oct 30, 2017

@jmchilton jmchilton force-pushed the jmchilton:dev_dependencies branch from 0df3428 to 1e2af2c Nov 1, 2017

@jmchilton jmchilton changed the title Rework dev and linting dependencies to use locked dependencies. Rework dev depenedencies to be Pipfile generated, locked, and hashed. Nov 1, 2017

tox.ini Outdated
@@ -5,23 +5,23 @@ skipsdist = True
[testenv:py27-lint]
commands = bash .ci/flake8_wrapper.sh
whitelist_externals = bash
deps = -r.ci/tox_envs/flake8/requirements.txt
deps = -rlib/galaxy/dependencies/pipfiles/flake8/pinned-requirements.txt

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Nov 1, 2017

Member

Why use pinned-requirements.txt instead of pinned-hashed-requirements.txt (which is what .ci/tox_envs/flake8/requirements.txt previously was)?

This comment has been minimized.

Copy link
@jmchilton

jmchilton Nov 1, 2017

Author Member

I guess we might as well use the hashed requirements - I was worried it was maybe more brittle (in case the hashes are wrong somehow or the version of pip used is too old and doesn't support them).

I'll switch this.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Nov 1, 2017

Member

We can expect developers and Travis to have a recent pip, I suppose. And scripts/common_startup.sh by default installs pip>=8.1 .

# Strip out hashes and trailing whitespace for unhashed version
# of this requirements file.
sed 's/--hash[^[:space:]]*//g' pinned-hashed-requirements.txt | sed 's/[[:space:]]*$//' > pinned-requirements.txt
cd ".."

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Nov 1, 2017

Member

This is not needed any more, $THIS_DIRECTORY is absolute thanks to pwd above.

done

THIS_DIRECTORY="$(cd "$(dirname "$0")" > /dev/null && pwd)"
cd "$THIS_DIRECTORY"

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Nov 1, 2017

Member

This is not needed.


for env in $ENVS
do
cd "$env"

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Nov 1, 2017

Member

Well, actually the other 2 are not needed only if you use cd "$THIS_DIRECTORY/$env" here.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Nov 1, 2017

Author Member

Right that was my comment on the other PR about not needing the absolute path anymore. Do you have a preference between these?

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Nov 2, 2017

Member

Having both && pwd and the cd .. is redundant, so I'd prefer to use cd "$THIS_DIRECTORY/$env" here and get rid of line 33 and 46.

EOF
}

while getopts ":i" opt; do

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Nov 1, 2017

Member

I think that ":i" should be changed to "hc" here.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Nov 1, 2017

Author Member

Ahh - that makes sense - copy/paste mistake.

@jmchilton jmchilton force-pushed the jmchilton:dev_dependencies branch from 1e2af2c to 65b528e Nov 1, 2017

Use pipenv to generate development dependencies.
- Rework pattern for linting dependencies to work for dev-requirements.txt.
- Get rid of dev-requirements.txt and replace it with a symbolic link to the latest pinned versions of dependencies.

Rebased against dev to bring in @nsoranzo's bashism bashings in #4876 and fixed up to strip trailing whitespace from generated requirement files.

@jmchilton jmchilton force-pushed the jmchilton:dev_dependencies branch from 65b528e to 1b0ccf6 Nov 2, 2017

@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Nov 2, 2017

@nsoranzo I made the requested change. I also ran the update and commit script which brought in a fix for flake8-import-order and verified that script works.

@nsoranzo nsoranzo merged commit 7886af3 into galaxyproject:dev Nov 2, 2017

6 checks passed

api test Build finished. 306 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 162 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 57 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
@nsoranzo

This comment has been minimized.

Copy link
Member

nsoranzo commented Nov 2, 2017

Thanks @jmchilton!

@nsoranzo nsoranzo changed the title Rework dev depenedencies to be Pipfile generated, locked, and hashed. Rework dev dependencies to be Pipfile generated, locked, and hashed. Mar 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.