Skip to content

Fix setup for ext packages.#122

Merged
Oberon00 merged 3 commits intoopen-telemetry:masterfrom
dynatrace-oss-contrib:bugfix/ext-setup
Sep 6, 2019
Merged

Fix setup for ext packages.#122
Oberon00 merged 3 commits intoopen-telemetry:masterfrom
dynatrace-oss-contrib:bugfix/ext-setup

Conversation

@Oberon00
Copy link
Copy Markdown
Member

@Oberon00 Oberon00 commented Sep 5, 2019

Previously the wheels would contain the metadata but not the actual
code.

Previously the wheels would contain the metadata but not the actual
code.
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
wsgi: pip install {toxinidir}/ext/opentelemetry-ext-wsgi
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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).

Copy link
Copy Markdown
Member

@a-feld a-feld left a comment

Choose a reason for hiding this comment

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

🚀 Nice catch!

Copy link
Copy Markdown
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

nice!

@Oberon00
Copy link
Copy Markdown
Member Author

Oberon00 commented Sep 6, 2019

I'm going ahead and merging this, I hope I addressed your comments properly, @reyang.

@Oberon00 Oberon00 merged commit 15039de into open-telemetry:master Sep 6, 2019
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.

4 participants