Add eachdist.py to simplify build. #291

Oberon00 · 2019-11-15T14:51:06Z

Quoting from CONTRIBUTING.md:

To quickly get up and running, you can use the scripts/eachdist.py tool that
ships with this project. First create a virtualenv and activate it.
Then run python scripts/eachdist.py develop to install all required packages
as well as the project's packages themselves (in --editable mode).
You can then run scripts/eachdist.py test to test everything or
scripts/eachdist.py lint to lint everything (fixing anything that is auto-fixable).

Of course the script works on both Windows and Linux (py > sh 😄). I encourage you to try it out.

PS: Yes, I know that I created "an ad-hoc, informally-specified, bug-ridden, slow implementation of half of" ~~xargs~~ find -exec.

Excluding `build` for black isn't a great idea when travis is checking out under `/home/travis/build/open-telemetry`.

codecov-io · 2019-11-15T16:22:34Z

Codecov Report

Merging #291 into master will increase coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
+ Coverage   85.74%   85.88%   +0.14%     
==========================================
  Files          33       33              
  Lines        1613     1630      +17     
  Branches      181      182       +1     
==========================================
+ Hits         1383     1400      +17     
  Misses        182      182              
  Partials       48       48

Impacted Files	Coverage Δ
...app/src/opentelemetry_example_app/flask_example.py	`100% <0%> (ø)`	⬆️
...src/opentelemetry/ext/opentracing_shim/__init__.py	`95.86% <0%> (+0.17%)`	⬆️
...xt-jaeger/src/opentelemetry/ext/jaeger/__init__.py	`85.18% <0%> (+0.18%)`	⬆️
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py	`90.54% <0%> (+0.26%)`	⬆️
...ts/src/opentelemetry/ext/http_requests/__init__.py	`89.18% <0%> (+0.3%)`	⬆️
opentelemetry-sdk/src/opentelemetry/sdk/util.py	`85.88% <0%> (+0.34%)`	⬆️
...opentelemetry/sdk/context/propagation/b3_format.py	`84.61% <0%> (+0.61%)`	⬆️
...ry-sdk/src/opentelemetry/sdk/resources/__init__.py	`70.83% <0%> (+2.65%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d441b1...ac4174b. Read the comment docs.

First, black already parsers .gitignore, so no need for the exclude option in pyproject.toml (apart from checked in generated files). The problem with this is that .gitignore has `build/` in it already and black does not take into account the root of the git repository, but instead seems to apply the ignore patterns to the path that results from the command line arguments (be that relative or absolute).

Oberon00 · 2019-11-22T13:40:39Z

~~DO NOT MERGE yet: eachdist.py lint/test miss most the examples currently (because they have no setup.py/pyproject.toml)~~

Fixed in ea07bd6

Conflicts: tox.ini

lzchen · 2019-12-03T20:28:58Z

pyproject.toml

@@ -3,21 +3,7 @@ line-length = 79
 exclude = '''
 (
 /(


If I understand correctly, this file is for excluding certain folders from being checked with black. We are removing these because the tox will run the eachdist.py script (which will only run on folders specified in eachdist.ini), instead of running black directly correct? For cases when I want to fix the lint directly myself without running tox, will changing this file effect that?

No, running black directly vs eachdist.py makes no difference at all. This was removed because black already checks .gitignore and so the lines were just redundant. This is actually a change independent from the rest of this PR and could be merged separately.

lzchen · 2019-12-03T20:36:03Z

dev-requirements.txt

@@ -0,0 +1,10 @@
+pylint~=2.3


Should this file have the word constraints in it as well? dev-constraints.txt maybe? Due to use using it as constraints in tox.

Well, if you use the install --with-dev-deps or develop commands, it will be used as a requirements file.

lzchen · 2019-12-03T20:38:22Z

tox.ini

+ -c dev-requirements.txt
+ sphinx
+ sphinx-rtd-theme
+ sphinx-autodoc-typehints
 opentracing~=2.2.0
 Deprecated>=1.2.6


How come no -c dev-requirements.txt for testenv:py37-tracecontext?

It seems that environment doesn't use any requirement that is listed in dev-requirements. Also, because the dev-requirements file is used for installing too (#291 (comment)), I didn't want to include all that.

lzchen · 2019-12-03T20:43:18Z

CONTRIBUTING.md

-This project uses [`tox`](https://tox.readthedocs.io) to automate some aspects
+To quickly get up and running, you can use the `scripts/eachdist.py` tool that
+ships with this project. First create a virtualenv and activate it.
+Then run `python scripts/eachdist.py develop` to install all required packages


What's the difference between python scripts/eachdist.py develop and python scripts/eachdist.py install --editable?

It is an alias for install --editable --with-dev-deps --eager-upgrades (see

opentelemetry-python/scripts/eachdist.py

Line 82 in cf7e685

editable=True, with_dev_deps=True, eager_upgrades=True

)

scripts/eachdist.py

codeboten

This is awesome, it makes writing code so much easier!

Just a non-blocking suggestion and it would be nice to have an example of how to use exec, I ended up digging through the test file to figure out what format was expecting.

Oberon00 · 2019-12-06T16:55:44Z

@codeboten

Just a non-blocking suggestion and it would be nice to have an example of how to use exec, I ended up digging through the test file to figure out what format was expecting.

This was actually bugging me too, so I took the time and documented exec properly (including two examples) in 487a600.

$ scripts/eachdist.py exec --help
usage: eachdist.py exec [-h] [--all [ALLFORMAT]] [--allsep ALLSEP]
                        [--allowexitcode ALLOWEXITCODE] [--mode MODE]
                        format

Run a command according to the `format` argument for each or all targets.

This is an advanced command that is used internally by other commands.

For example, to install all distributions in this repository
editable, you could use:

    scripts/eachdist.py exec "python -m pip install -e {}"

This will run pip for all distributions which is quite slow. It gets
a bit faster if we only invoke pip once but with all the paths
gathered together, which can be achieved by using `--all`:

    scripts/eachdist.py exec "python -m pip install {}" --all "-e {}"

The sortfirst option in the DEFAULT section of eachdist.ini makes
sure that dependencies are installed before their dependents.

Search for usages of `parse_subargs` in the source code of this script
to see more examples.

This command first collects target paths and then executes
commands according to `format` and `--all`.

Target paths are initially all Python distribution root paths
(as determined by the existence of setup.py, etc. files).
They are then augmented according to the section of the
`PROJECT_ROOT/eachdist.ini` config file specified by the `--mode` option.

The following config options are available (and processed in that order):

- `extraroots`: List of project root-relative glob expressions.
  The resulting paths will be added.
- `sortfirst`: List of glob expressions.
  Any matching paths will be put to the front of the path list,
  in the same order they appear in this option. If more than one
  glob matches, ordering is according to the first.
- `subglob`: List of glob expressions. Each path added so far is removed
  and replaced with the result of all glob expressions relative to it (in
  order of the glob expressions).

After all this, any duplicate paths are removed (the first occurrence remains).

positional arguments:
  format                Format string for the command to execute.

                        The available replacements depend on whether `--all` is specified.
                        If `--all` was specified, there is only a single replacement,
                        `{}`, that is replaced with the string that is generated from
                        joining all targets formatted with `--all` to a single string
                        with the value of `--allsep` as separator.

                        If `--all` was not specified, the following replacements are available:

                        - `{}`: the absolute path to the current target in POSIX format
                          (with forward slashes)
                        - `{rel}`: like `{}` but relative to the project root.
                        - `{raw}`: the absolute path to the current target in native format
                          (thus exactly the same as `{}` on Unix but with backslashes on Windows).
                        - `{rawrel}`: like `{raw}` but relative to the project root.

                        The resulting string is then split according to POSIX shell rules
                        (so you can use quotation marks or backslashes to handle arguments
                        containing spaces).

                        The first token is the name of the executable to run, the remaining
                        tokens are the arguments.

                        Note that a shell is *not* involved by default.
                        You can add bash/sh/cmd/powershell yourself to the format if you want.

                        If `--all` was specified, the resulting command is simply executed once.
                        Otherwise, the command is executed for each found target. In both cases,
                        the project root is the working directory.

optional arguments:
  -h, --help            show this help message and exit
  --all [ALLFORMAT]     Instead of running the command for each target, join all target
                        paths together to run a single command.

                        This option optionally takes a format string to apply to each path. The
                        available replacements are the ones that would be available for `format`
                        if `--all` was not specified.

                        Default ALLFORMAT if this flag is specified: `{rel}`.
  --allsep ALLSEP       Separator string for the strings resulting from `--all`.
                        Only valid if `--all` is specified.
  --allowexitcode ALLOWEXITCODE
                        The given command exit code is treated as success and does not abort execution.
                        Can be specified multiple times.
  --mode MODE, -m MODE  Section of config file to use for target selection configuration.
                        See description of exec for available options.

You're welcome!

See #2 (comment).

c24t

This is a huge improvement, and will make developing with multiple packages in the same repo much easier. I'm taking some of the script on faith here, and didn't do an exhaustive review, but it did work as advertised when I tested it.

c24t · 2019-12-10T01:20:45Z

tox.ini

+ pylint
+ flake8
+ isort
+ black


These are listed in dev-requirements, why duplicate them here?

Because dev-requirements has more requirements than each tox environment on its own. That's why it's used with -c so that only the version requirements are taken, but the targets are not automatically installed. I only moved the versions out of the toxfile with this. If I used -r, it would install unnecessary packages for each target (e.g. we don't need pytest or sphinx for the lint toxenv).

c24t · 2019-12-10T01:21:17Z

tox.ini

+ sphinx
+ sphinx-rtd-theme
+ sphinx-autodoc-typehints


Same question

c24t · 2019-12-10T01:39:35Z

dev-requirements.txt

+sphinx-rtd-theme~=0.4
+sphinx-autodoc-typehints~=1.10.2
+pytest!=5.2.3
+pytest-cov>=2.8


I understand these were moved from tox.ini, but what do you think about using the latest version of build tools instead of pinning these? It does mean dealing with some build unpredictability, but prevents us having to make big changes to update these packages later.

I think we should at least have >= requirements (although the minimum version would be very low due to Python 3.4 support most of the time, at least for requirements we use on Python != 3.7). I'm fine with removing upper bounds though.

scripts/eachdist.py

c24t · 2019-12-10T01:50:49Z

scripts/eachdist.py

+ for subdir in rootpath.iterdir():
+ if not subdir.is_dir():
+ continue
+ if subdir.name.startswith(".") or subdir.name.startswith("venv"):


Using gitignore or similar seems like a better long term solution here.

True, actually since Python already supports globbing (and it's already used in that script), that should be a relatively simple change.

Add eachdist.py to simplify build.

afc7722

Oberon00 requested review from a-feld, c24t, carlosalberto, lzchen, reyang and toumorokoshi as code owners November 15, 2019 14:51

Oberon00 added 5 commits November 15, 2019 16:04

Fix coverage build.

56f2fb3

Mark eachdist.py executable.

4efd2af

Fix eachdist.py for Python 3.4 & 3.5

e5dcf63

Fix lint.

c38b2fb

Fix linting missing files.

e62f5bc

Excluding `build` for black isn't a great idea when travis is checking out under `/home/travis/build/open-telemetry`.

Oberon00 force-pushed the eachdist-script branch from bf28f53 to e62f5bc Compare November 15, 2019 16:15

Oberon00 force-pushed the eachdist-script branch from 4d93900 to 2524efe Compare November 15, 2019 16:38

Merge branch 'master' into eachdist-script

e3266e7

Oberon00 added the build & infra Issues related to build & infrastructure. label Nov 18, 2019

Merge branch 'master' into eachdist-script

7693a40

Oberon00 added 5 commits November 22, 2019 16:49

Add extraroots to cover missing py files.

ea07bd6

eachdist.py: Add --eager-upgrades, default w/ develop.

ef7dd33

Fix lint.

51f5d8d

Merge branch 'master' into eachdist-script

945e1e7

Conflicts: tox.ini

Make sure testutil & wsgi are installed before dependents.

83120d6

Oberon00 force-pushed the eachdist-script branch from f3900b3 to 83120d6 Compare November 25, 2019 08:23

Oberon00 added 3 commits November 25, 2019 11:05

Fix dependencies for ext-testutil.

66e9c8a

Merge branch 'master' into eachdist-script

f216836

Conflicts: tox.ini

Fix wrong dependency order for tracecontext in tox.ini.

cf7e685

lzchen reviewed Dec 3, 2019

View reviewed changes

Merge branch 'master' into eachdist-script

10cfb56

lzchen approved these changes Dec 5, 2019

View reviewed changes

Fix mypy version being ignored.

c5a2aa6

codeboten reviewed Dec 5, 2019

View reviewed changes

scripts/eachdist.py Show resolved Hide resolved

codeboten approved these changes Dec 5, 2019

View reviewed changes

Oberon00 added 2 commits December 6, 2019 16:12

Fix error message when calling without command.

4052f5f

eachdist.py: Document exec, improve doc in general.

487a600

Typo.

3c203be

Oberon00 added 2 commits December 9, 2019 10:36

Merge branch 'master' into eachdist-script

a8800b2

Lint jaeger examples.

ac4174b

See #2 (comment).

Oberon00 mentioned this pull request Dec 9, 2019

Version number normalization test #254

Closed

a-feld removed their request for review December 10, 2019 00:55

c24t approved these changes Dec 10, 2019

View reviewed changes

c24t merged commit b25af7f into open-telemetry:master Dec 10, 2019

Oberon00 deleted the eachdist-script branch December 10, 2019 08:51

c24t mentioned this pull request Dec 10, 2019

Adding Zipkin exporter #320

Merged

c24t removed the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Dec 11, 2019

Oberon00 mentioned this pull request Dec 11, 2019

Build fails: Various erros in py37-tracecontext #329

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add eachdist.py to simplify build. #291

Add eachdist.py to simplify build. #291

Oberon00 commented Nov 15, 2019 •

edited

Loading

codecov-io commented Nov 15, 2019 •

edited

Loading

Oberon00 commented Nov 22, 2019 •

edited

Loading

lzchen Dec 3, 2019 •

edited

Loading

Oberon00 Dec 5, 2019

lzchen Dec 3, 2019

Oberon00 Dec 5, 2019

lzchen Dec 3, 2019

Oberon00 Dec 5, 2019

lzchen Dec 3, 2019

Oberon00 Dec 5, 2019

codeboten left a comment

Oberon00 commented Dec 6, 2019 •

edited

Loading

c24t left a comment

c24t Dec 10, 2019

Oberon00 Dec 10, 2019

c24t Dec 10, 2019

c24t Dec 10, 2019

Oberon00 Dec 10, 2019

c24t Dec 10, 2019

Oberon00 Dec 10, 2019

Add eachdist.py to simplify build. #291

Add eachdist.py to simplify build. #291

Conversation

Oberon00 commented Nov 15, 2019 • edited Loading

codecov-io commented Nov 15, 2019 • edited Loading

Codecov Report

Oberon00 commented Nov 22, 2019 • edited Loading

lzchen Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeboten left a comment

Choose a reason for hiding this comment

Oberon00 commented Dec 6, 2019 • edited Loading

c24t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 commented Nov 15, 2019 •

edited

Loading

codecov-io commented Nov 15, 2019 •

edited

Loading

Oberon00 commented Nov 22, 2019 •

edited

Loading

lzchen Dec 3, 2019 •

edited

Loading

Oberon00 commented Dec 6, 2019 •

edited

Loading