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

setup: speed up and clean improvements #699

Merged
merged 1 commit into from
Jan 29, 2020

Conversation

blankoworld
Copy link
Contributor

@blankoworld blankoworld commented Jan 15, 2020

Why are you opening this PR?

  • Because of the setup load 54 times the virtual environment (pipenv run), we need to reduce it to 1 load.
  • To reduce logs from setup (delete all useless warnings)

For that I use this tip from Pipenv: https://pipenv.readthedocs.io/en/latest/advanced/#custom-script-shortcuts .

For Python I also use a "ignore DeprecationWarning" state.

How to test?

# First prepare the environment
docker-compose down && pipenv --rm && ./scripts/bootstrap && docker-compose up -d && ./docker/wait-for-services.sh
# Then launch the setup like this:
PIPENV_QUIET=1 pipenv run setup

The setup should only display info messages, things about the setup.

Python DeprecationWarnings should have disappeared.

Code review check list

  • Commit message template compliance.
  • Commit message without typos.
  • File names.
  • Functions names.
  • Functions docstrings.
  • Unnecessary commited files?

Copy link
Contributor

@jma jma left a comment

Choose a reason for hiding this comment

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

You can also try a pyenv shell at the begining of ./scripts/setup.

@@ -73,3 +73,4 @@ python_version = "3.6"
test = "python setup.py test"
build_sphinx = "python setup.py build_sphinx"
dev = "pytest --no-cov -vs tests"
setup = "./scripts/setup"
Copy link
Contributor

Choose a reason for hiding this comment

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

PIPENV_QUIET=1 ./script/setup instead or better include PIPENV_QUIET=1 in the ./scripts/setup?

Copy link
Contributor Author

@blankoworld blankoworld Jan 17, 2020

Choose a reason for hiding this comment

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

PIPENV_QUIET=1 have an impact on the pipenv command only if used outside the pipenv itself. So before the pipenv run setup or before the script IF it includes pyenv shell inside.

It is to avoid the Courtesy note (Cf. pypa/pipenv#3068). But our pipenv version have a bug on it.

Edit: I will test with pyenv shell inside the scripts/setup file.

Copy link
Contributor Author

@blankoworld blankoworld Jan 21, 2020

Choose a reason for hiding this comment

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

@jma: pyenv shell seems to need more preparation before being usable for this.

❯ ./scripts/setup
pyenv: shell integration not enabled. Run `pyenv init' for instructions.
❯ pyenv init
# Load pyenv automatically by appending
# the following to ~/.zshrc:

eval "$(pyenv init -)"

It remains more comfortable to use the custom shortcut functionnality of pipenv:

  • no more preparation for the developer
  • a message explains to developer which command to use (if it forget to use pipenv command)

@blankoworld blankoworld force-pushed the improve-setup-variable-loading branch 3 times, most recently from f6175cc to 2b73e22 Compare January 21, 2020 13:22
@blankoworld blankoworld force-pushed the improve-setup-variable-loading branch 2 times, most recently from aa518ea to 3d29ec9 Compare January 23, 2020 14:34
@Garfield-fr Garfield-fr removed their request for review January 24, 2020 14:06
@blankoworld blankoworld force-pushed the improve-setup-variable-loading branch from 3d29ec9 to 036a573 Compare January 27, 2020 09:43
@blankoworld blankoworld removed the WIP label Jan 27, 2020
@blankoworld blankoworld requested a review from rerowep January 27, 2020 10:04
* Setup:
  * Cleans up DeprecationWarning from Python
  * Cleans up "There are .env of .flaskenv" warning messages
* Adds new pipenv command: `setup`. Launch `pipenv run setup`
* Deletes all `pipenv run` in scripts/setup file to avoid loading
multiple times the same environment
* Checks at setup if we are in a virtualenv (via pipenv)

Co-Authored-by: Olivier DOSSMANN <[email protected]>
@blankoworld blankoworld force-pushed the improve-setup-variable-loading branch from 036a573 to bb0a2f5 Compare January 28, 2020 15:18
@blankoworld blankoworld removed the WIP label Jan 28, 2020
@blankoworld blankoworld merged commit af60c35 into rero:dev Jan 29, 2020
@blankoworld blankoworld deleted the improve-setup-variable-loading branch March 30, 2020 15:06
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.

5 participants