Fix directory detection heuristic - #1802
Open
cromedome wants to merge 2 commits into
Open
Conversation
The upward walk in _build_location was effectively stalling before 2.1.0,
so it rarely left the script's own directory. Now that it walks properly,
the heuristic it walks with turns out to be far too weak: "holds both lib/
and bin/" matches every Perl distribution root, most home directories,
/usr and /usr/local. Applications were resolving their config, views and
public directories against a directory far above themselves.
Recognise a directory as an application root by what it actually holds,
checked closest-first as the walk climbs:
1. a .dancer file, the explicit marker
2. environments/, views/ or public/
3. config.<ext> for any extension Config::Any handles, so anything
ConfigReader would load counts
4. lib/ + bin/, kept for apps carrying no Dancer2 artifacts at all, but
checked last so it only wins when found closer than real evidence
Also canonicalise the walk with realpath up front and stop on parent()
reaching its own fixed point. A relative $subdir became a chain of '..'
segments, which made the blib/ regex meaningless and the rootdir string
comparison non-portable.
t/issues/memleak/die_in_hooks.t asserts on stderr and had been relying on
detection failing: it previously found no app root, fell back to its own
directory, and so never loaded t/issues/config.yml with its logger: Note.
Give it a config of its own that states the requirement explicitly.
Fixes GH #1781
A directory holding .git, .hg, .svn, Makefile.PL, Build.PL, dist.ini or cpanfile is the root of a checkout or a distribution, and so a ceiling: whatever the application root is, it sits at or below that line, never in the home directory or build directory above it. Halt the upward walk there rather than escaping past it, falling back to the script's own directory as before. This closes the case GH #1781 leaves open, where an application carrying no Dancer2 artifacts anywhere above the script is run from somewhere under a home directory that happens to hold lib/ and bin/. The check runs only for directories _is_app_root has already rejected, so a project root that is also an application root - the usual case, since 'dancer2 gen' writes both a cpanfile and a Makefile.PL - has been accepted well before this is reached.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1781. Supersedes #1782.
Problem
_build_locationwalks up from the caller's directory looking for the applicationroot. Its test for "is this an app root?" was "does it hold both
lib/andbin/?"— which also describes every Perl distribution root, most home directories,
/usr,and
/usr/local.This was harmless while the upward walk was effectively stalling, since it rarely
left the script's own directory. 2.1.0 made the walk work, and the weak test
immediately started selecting directories far above the application. Apps then
resolved
config.yml,views/,public/andlogs/against the wrong root, whichis the template and public-file failures in #1781.
Reproducing
The trigger is any directory holding both
lib/andbin/above the app. ADancer2 checkout already has
lib/:That's why a plain
cpanm Dancer2fails for some people and not others:~/bin+~/libis enough, and the walk climbs out of the extracted tarball into the homedirectory. Reconstructing that:
11 failing test files on
main, 0 on this branch.Fix
Recognise an app root by what it actually holds, closest-first as the walk climbs:
.dancerfile — the explicit markerenvironments/,views/orpublic/config.<ext>for any extensionConfig::Anyhandleslib/+bin/— kept for apps with no Dancer2 artifacts, but checked last so itonly wins when found closer than real evidence. Still skips
blib/.Because
t/holdsconfig.yml,views/andpublic/, it now matches immediatelyand the walk never approaches the distribution root or
~.A second commit stops the walk at a checkout or distribution boundary (
.git,.hg,.svn,Makefile.PL,Build.PL,dist.ini,cpanfile) rather thanescaping above it. That only applies to directories already rejected as app roots,
so a
dancer2 genapp — which ships acpanfileand aMakefile.PLalongside.dancerandconfig.yml— is accepted several tiers earlier.The walk is also canonicalised with
realpathup front, and now stops whenparent()reaches its own fixed point instead of comparing againstrootdir. Arelative
$subdirbecame a chain of..segments, which made theblib/regexmeaningless and the root comparison non-portable. That's the real issue behind
#1782, whose stated premise —
path('.')->parentbeing a fixed point — doesn't holdon current Path::Tiny.
Note on the test change
t/issues/memleak/die_in_hooks.tasserts on stderr and was relying on detectionfailing: it previously found no app root, fell back to its own directory, and so
never loaded
t/issues/config.ymlwith itslogger: "Note". It now has a config ofits own stating that requirement.