Ticket #4939 (closed Bug: fixed)

Opened 4 years ago

Last modified 4 years ago

CKReleaser limits source and target directories too aggressively

Reported by: claycole Owned by: wwalc
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

4939.patch (3.1 KB) - added by wwalc 4 years ago.
ckreleaser.jar (1.1 MB) - added by wwalc 4 years ago.

Change History

comment:1 Changed 4 years ago by wwalc

  • Owner set to wwalc
  • Keywords Confirmed added
  • Component changed from General to Project : CKReleaser

Changed 4 years ago by wwalc

Changed 4 years ago by wwalc

comment:2 follow-up: ↓ 3 Changed 4 years ago by wwalc

  • 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 4 years ago by claycole

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 4 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:5 Changed 4 years ago by wwalc

  • Status changed from new to closed
  • Resolution set to fixed

Fixed with [4873].

Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy