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

Do not chdir() to phakefile directory #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

clue
Copy link
Collaborator

@clue clue commented Dec 18, 2013

This is done in order to preserve relative path passed as arguments, as
they're relative to the current working directory.

Consider the directory structure:

project/
|- demo/
|   \- example.txt
\- Phakefile.php

Consider I have a task named test which accepts a single argument file:

  • If I'm in the directory project/, I expect to be able to run phake test file=demo/example.txt
  • If I'm in the directory project/demo, I expect to be able to run phake test file=example.txt

Open for discussion:

  • What was the original rational for changing dirs? (What about rake / make?)
  • Is this considered a BC break, and if so, how should this be handled?

This is done in order to preserve relative path passed as arguments, as
they're relative to the current working directory.
@jaz303
Copy link
Owner

jaz303 commented Dec 18, 2013

Build systems in general must cd to the directory containing the build file so that any file references within the file itself are resolved predictably and consistently; rake and make certainly both perform a chdir.

If implemented, this feature should either be a) behind a command-line flag or b) set within the Phakefile itself (e.g. a leading comment like // phake-nochdir.

@clue
Copy link
Collaborator Author

clue commented Dec 21, 2013

rake and make certainly both perform a chdir

Alright, admittedly I have little experience with relative paths in either make or rake, and a quick search didn't turn up many results either. So you can confirm this is actually the case?

My assumption was that all paths are relative to the current working directory (as with normal (php) scripts), whereas you're suggesting that they should be relative to the file they're defined in, right? This shouldn't show any obvious effects if you're running phake from the folder where your Phakefile is located, but it differs if you're running if from a sub-folder (say "demo/" in the above example).

Personally, I'm okay with it either way, it's just that latter case was unexpected. So if we keep it that way, we should probably address a few things instead:

  • Add documentation to the readme (unexpected behavior with regards to normal php)
  • Make sure the paths are always relative to the file they're defined in.
  • Store the original working directory somewhere (Application::get_original_working_directory()?) in case you need to resolve a file relative to the current working directory (see above example for passing in relative paths as arguments).

@jaz303
Copy link
Owner

jaz303 commented Dec 21, 2013

I've tested the behaviour across make and rake:

  • make in fact only searches the cwd for a makefile, and does follow up with a recursive search of parent directories. If a Makefile path is provided explicitly via the -f option, the working directory is unchanged.
  • rake will chdir to the Rakefile's directory when it is found implicitly. When the -f option is used, the working directory is again unchanged.

@clue
Copy link
Collaborator Author

clue commented Dec 24, 2013

Thanks for taking the time to confirm this!

When the -f option is used, the working directory is again unchanged.

So in other words, using relative paths is unpredictable as it depends on several factors not in control of the Phakefile author? If that is actually the case, I'm even more tempted to say that using chdir() is a bad idea and should be avoided.

In PHP, one usually works around this issue by using paths like require __DIR__ . '/myfile.php' instead. This makes it explicit on when to use relative and when to use absolute paths. Some quick searches seem to indicated this is applicable to rake/ruby as well?

If so, it's probably easier (and safer) to refrain from trying to work around relative paths as per the original patch.

Also, this would open up a few possibilities like

  • Using URLs like phake -f http://mycorporation.local/Phakefile.php
  • Embedded phake and a Phakefile in a single Phar (where __DIR__ looks like phar:///my/path/to/phake.phar/example and there's no way to fake a chdir() within a phar archive)

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.

2 participants