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.

Sign up for GitHub

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

Jump to bottom

Fix setup for ext packages. #122

Merged
merged 3 commits into from
Sep 6, 2019
Merged

Fix setup for ext packages. #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa673c3
Fix setup for ext packages.
Oberon00 Sep 5, 2019
5ede830
Check that all packages install properly in test.
Oberon00 Sep 6, 2019
0375f25
Document why we don't use -e.
Oberon00 Sep 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ext/opentelemetry-ext-http-requests/setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ classifiers =
python_requires = >=3.4
package_dir=
=src
packages=find:
packages=find_namespace:
install_requires =
opentelemetry-api >= 0.1.dev0
requests ~= 2.0
Expand Down
2 changes: 1 addition & 1 deletion ext/opentelemetry-ext-wsgi/setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ classifiers =
python_requires = >=3.4
package_dir=
=src
packages=find:
packages=find_namespace:
install_requires =
opentelemetry-api

Expand Down
11 changes: 6 additions & 5 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,13 @@ changedir =
test-ext-http-requests: ext/opentelemetry-ext-http-requests/tests

commands_pre =
; Install without -e to test the actual installation
python -m pip install -U pip setuptools wheel
test: pip install -e {toxinidir}/opentelemetry-api
test-sdk: pip install -e {toxinidir}/opentelemetry-sdk
ext: pip install -e {toxinidir}/opentelemetry-api
wsgi: pip install -e {toxinidir}/ext/opentelemetry-ext-wsgi
http-requests: pip install -e {toxinidir}/ext/opentelemetry-ext-http-requests
test: pip install {toxinidir}/opentelemetry-api
test-sdk: pip install {toxinidir}/opentelemetry-sdk
ext: pip install {toxinidir}/opentelemetry-api
wsgi: pip install {toxinidir}/ext/opentelemetry-ext-wsgi
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to remove -e?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is done to expose the packaging problem. With the -e flag enabled, the tests will pass regardless of what appears in the package since all tests are just pointing at the existing directory structure.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense. would be great to verify and comment as such, this type of configuration can get really hard to understand without some comments.

Copy link
Member

Choose a reason for hiding this comment

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

Ln32 is using -e, what's the difference here?
ext: pip install -e {toxinidir}/opentelemetry-api

Copy link
Member Author

Choose a reason for hiding this comment

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

One build without -e should be enough to detect the problem, was my thinking.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see this is a different package. You are right, we could remove the -e there too to increase coverage.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment and removed the -e also from the other pip calls (but only in the test environments, lint keeps it).

http-requests: pip install {toxinidir}/ext/opentelemetry-ext-http-requests

commands =
mypy: mypy --namespace-packages opentelemetry-api/src/opentelemetry/
Expand Down