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

Make compilation to single file compatible with include __DIR__ . '...' in files #943

Merged
merged 1 commit into from
Jul 3, 2020

Conversation

labbati
Copy link
Member

@labbati labbati commented Jul 2, 2020

Description

Problem

For performance reasons, we compile all our files into a single file that is then served from the bridge folder. The problem with this is that in 3 classes of files in our code base relies on include __DIR__ . '...';. During compilation __DIR__ is replaced with the actual absolute path of the machine when compilation happens, e.g. /home/circleci/datadog/.... This is clearly not compatible with our deployments that are installed in either /opt/... or any custom folder the user likes.

Which types of files and who is impacted?

Only users on PHP 5.4, because

  1. Legacy integrations because of the include of try_catch_finally.php include. Hence only users on PHP 5.4 are impacted as the new sandboxed integrations are not using this mechanism.
  2. Tracer::version() method: this impact no one in production, it only breaks one line in dd-doctor.php.

What is the short term proposal?

After we generate the dingle file, we change all absolute import from bridge to the bridge dir itself.

What is the long term proposal?

  1. Legacy integrations are disappearing as 5.4 will be soon migrated into the sandboxed api.
  2. Remove the Tracer::version() check which was only used to compare composer vs ext versions. This is now no longer requested after we are providing the noop classes in src/api.

What other options where tried

--skip_dir_file, --fix_dir and --fix_files from https://github.com/ClassPreloader/ClassPreloader but the problem is that even relative paths from bridge would be different in dev and in production.

Why we did not catch in tests and what we will do?

When executing tests locally and in CI, we build in the same machine where we execute tests, so files happen to be there accidentally. We were using those files without even realizing.

  • short term: the release will be tested manually on PHP 5.4 and released.
  • mid term: we will be testing the installed version in CI.

Readiness checklist

  • (only for Members) Changelog has been added to the appropriate release draft. Create one if necessary.
  • Tests added for this feature/bug.

Reviewer checklist

  • Appropriate labels assigned.
  • Milestone is set.
  • Changelog has been added to the appropriate release draft. For community contributors the reviewer is in charge of this task.

@morrisonlevi morrisonlevi changed the title Make compilation to single file compatibile with include __DIR__ . '...' in files Make compilation to single file compatible with include __DIR__ . '...' in files Jul 2, 2020
@SammyK SammyK added packaging 🐛 bug Something isn't working labels Jul 2, 2020
@SammyK SammyK added this to the 0.47.0 milestone Jul 2, 2020
Copy link
Contributor

@SammyK SammyK left a comment

Choose a reason for hiding this comment

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

Confirmed this change on PHP 5.4:

return include '/home/circleci/app/bridge/../src/DDTrace/Integrations/Eloquent' . '/../../try_catch_finally.php';

->

return include __DIR__ . '/../src/DDTrace/Integrations/Eloquent' . '/../../try_catch_finally.php';

Nice short-term solution @labbati!

@morrisonlevi morrisonlevi marked this pull request as ready for review July 2, 2020 22:06
@labbati labbati merged commit 1cfa542 into master Jul 3, 2020
@labbati labbati deleted the labbati/build/compiled-try-catch branch July 3, 2020 08:42
@morrisonlevi morrisonlevi mentioned this pull request Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants