Opened 14 years ago

Closed 14 years ago

#4939 closed Bug (fixed)

CKReleaser limits source and target directories too aggressively

Reported by: Clayton Coleman Owned by: Wiktor Walc
Priority: Normal Milestone:
Component: Project : CKReleaser Version: SVN (CKEditor) - OLD
Keywords: Confirmed Review+ Cc:

Description

CKReleaser prevents the source and target directories from being different because io.isChildPath has an incorrect logical test. The default build (as specified in release.bat) works because target is a child path of source and because the _dev directory is ignored during copy.

Default command (from latest version of release.bat):

java -jar ckreleaser/ckreleaser.jar ckreleaser.release ../.. release "3.1 SVN" ckeditor_3.1_svn --run-before-release=langtool.bat

release.bat is in the <root>/_dev/releaser directory of the source tree, which means the source dir is <root> and the target directory is <root>/_dev/releaser/release

For 3rd parties using different build systems and different directory structures (for instance, where the build output must live in a directory outside of the source tree) it would be preferable to specify a command like:

java -jar ckreleaser/ckreleaser.jar customckreleaser.release path/to/ckeditor build/ckeditor <versionid> <filename>

Executing this command throws the exception:

Target directory must be located outside source directory

which is thrown by this line of releaser.js

468 if ( CKRELEASER.io.isChildPath( targetDir.getCanonicalPath(), sourceDir.getCanonicalPath() ) )
469 throw "Target directory must be located outside source directory";

This is incorrect - the target directory and source directory are not located underneath each other, which examination of isChildPath shows is because the return statement is too aggressive.

403 isChildPath : function( childPath, parentPath )
404 {
405 if ( childPath.length > parentPath.length )
406 return false;
407
408 var i = 0, max = Math.min( childPath.length(), parentPath.length() );
409
410 while ( i < max && childPath.charAt( i ) == parentPath.charAt( i ) )
411 i++;
412
413 return i != max;
414 }

If the working directory is c:\test, the source dir is c:\test\path\to\ckeditor, and the target dir is c:\test\build\ckeditor, then isChildPath should return false (the two are not child paths of each other). However, "return i != max" returns true for this scenario because the two paths match only up until character 7 (i == 7, max == 22). It should be "return i==max".

If isChildPath is adjusted to correctly check path parentage then ckreleaser can be used in different directory structures

Attachments (2)

4939.patch (3.1 KB) - added by Wiktor Walc 14 years ago.
ckreleaser.jar (1.1 MB) - added by Wiktor Walc 14 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 14 years ago by Wiktor Walc

Component: GeneralProject : CKReleaser
Keywords: Confirmed added
Owner: set to Wiktor Walc

Changed 14 years ago by Wiktor Walc

Attachment: 4939.patch added

Changed 14 years ago by Wiktor Walc

Attachment: ckreleaser.jar added

comment:2 Changed 14 years ago by Wiktor Walc

Keywords: Review? added

The isChildPath() function was invalid.

Apart from that, theoretically, there should be something like

if (CKRELEASER.io.isChildPath(target, source) && !CKRELEASER.release.isIgnoredPath(target))
     throw "Target directory must be located outside the source directory";

to check whether target dir is ignored. If yes, then we should allow using target directory in the source directory (otherwise we'll not be able to create a release inside of the _dev folder).

In reality, the "isIgnoredPath" function works in a special way and cannot be called like that. Instead, we're looping through all ignored directories and checking whether the target directory is a child folder of one of the ignored folders.

comment:3 in reply to:  2 Changed 14 years ago by Clayton Coleman

The provided JAR worked in the scenarios I needed - running in a working directory outside of source where target was a child of the working directory.

Thanks

comment:4 Changed 14 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:5 Changed 14 years ago by Wiktor Walc

Resolution: fixed
Status: newclosed

Fixed with [4873].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy