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

[omnibus] Fix MacOS build conflicting with system Python 3 #5010

Merged
merged 2 commits into from
Feb 27, 2020

Conversation

KSerrania
Copy link
Contributor

@KSerrania KSerrania commented Feb 27, 2020

What does this PR do?

Work around an implementation detail of Python 3 on MacOS making the build fail (because the embedded pip3 tries to use the system python3 instead of the embedded one).

Motivation

Make the MacOS build work with Python 3 installed on the system.

Full explanation

On MacOS, python3 does an ugly hack to circumvent the limits imposed on processes by Apple on framework builds (which is the case when installing python3 with brew).

This hack consists in setting __PYVENV_LAUNCHER__ to the location of the system python3 executable, and using it instead of argv[0] (which is set by the OS). The problem is that this variable is not unset after it's read, so it's still set in all commands launched by omnibus.

In particular, it's set during the pip installation (in pip3.rb), which makes the /opt/datadog-agent/embedded/bin/python3 setup.py install --prefix=/opt/datadog-agent/embedded command link pip3 to the system python instead of the embedded python (since that's what the __PYVENV_LAUNCHER__ var is pointing at), bypassing the --prefix option and making the build fail.

More info here: https://stackoverflow.com/questions/26323852/whats-the-meaning-of-pyvenv-launcher-environment-variable

This might be fixed by: pypa/virtualenv#1648, so we should check again if this hack is still needed in future releases.

@KSerrania KSerrania added this to the 7.18.0 milestone Feb 27, 2020
@KSerrania KSerrania requested a review from a team as a code owner February 27, 2020 13:28
@KSerrania KSerrania added the [deprecated] team/agent-core Deprecated. Use metrics-logs / shared-components labels instead.. label Feb 27, 2020
olivielpeau
olivielpeau previously approved these changes Feb 27, 2020
Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for debugging this!

Made a small suggestion

tasks/agent.py Outdated
# is pointing at), bypassing the "--prefix" option and making the build fail.
# More info here: https://stackoverflow.com/questions/26323852/whats-the-meaning-of-pyvenv-launcher-environment-variable
# This might be fixed by: https://github.com/pypa/virtualenv/pull/1648, so we should check again if this hack
# is still needed in future releases.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation! Since it's quite complex and long it might make sense to put the full explanation in the PR description, and link to the PR from here?

@codecov
Copy link

codecov bot commented Feb 27, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5010      +/-   ##
==========================================
+ Coverage   52.14%   52.16%   +0.01%     
==========================================
  Files         719      719              
  Lines       52208    52225      +17     
==========================================
+ Hits        27223    27242      +19     
+ Misses      23365    23364       -1     
+ Partials     1620     1619       -1
Flag Coverage Δ
#linux 53.63% <ø> (ø) ⬆️
#windows 52.33% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
pkg/trace/writer/testserver.go 93% <0%> (-3%) ⬇️
pkg/util/log/strip.go 94.53% <0%> (+5.34%) ⬆️

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 4e795db...7c78b57. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Feb 27, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5010      +/-   ##
==========================================
+ Coverage   52.14%   52.16%   +0.01%     
==========================================
  Files         719      719              
  Lines       52208    52225      +17     
==========================================
+ Hits        27223    27242      +19     
+ Misses      23365    23364       -1     
+ Partials     1620     1619       -1
Flag Coverage Δ
#linux 53.63% <ø> (ø) ⬆️
#windows 52.33% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
pkg/trace/writer/testserver.go 93% <0%> (-3%) ⬇️
pkg/util/log/strip.go 94.53% <0%> (+5.34%) ⬆️

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 4e795db...7c78b57. Read the comment docs.

@KSerrania KSerrania merged commit d16a8a4 into master Feb 27, 2020
@KSerrania KSerrania deleted the kserrania/fix-python3-build-mac branch February 27, 2020 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog [deprecated] team/agent-core Deprecated. Use metrics-logs / shared-components labels instead.. [deprecated] team/agent-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants