ckeditor / ckeditor4-sdk

A set of software development tools for CKEditor 4 along with samples.
Other
18 stars 21 forks source link

Build update improvements #151

Closed Tade0 closed 9 years ago

Tade0 commented 9 years ago

This pull request contains a few adjustments to the build system, namely:

-Submodule update is now done through a grunt task that accepts a parameter which determines to which branch should ckeditor-dev be checked-out. Apart from that it modifies a line in samples/assets/simplesample.js so that it contains the URL to the current (as updated) version of CKEditor.

-Built files are placed in build/ (=online, offline) instead of dev/ckeditor. Similarly when using the --sdk-pack flag the zip file is placed in the path: build/.zip.

Tade0 commented 9 years ago

Fixed the errors found during review.

Additional changes: after the build, the sdk-get-version task supplies the version number which is then used by replace:simplesample to copy samples/assets/simplesample.js again, but this time with <CKEditorVersion> replaced by the proper value.

Reinmar commented 9 years ago

Are you sure it's the right thing to do to copy simplesample while replacing that placeholder? What if some other task would like to process that file after it was copied for the first time? E.g. what if some processing is done inside app.js?

IMO it should work this way:

  1. First copy all files.
  2. Then process these copied files.

Or:

  1. Copy all files.
  2. While copying, process them.
Tade0 commented 9 years ago

Here's some context to the solution I implemented:

Currently the build script has some quirks that require ironing out. One of them is that some of the files get copied twice during the build. Some things work only because they were added on top of all that copying. Bottom line is, the part that does the copying requires a major rewrite.

I wanted to avoid doing that in this ticket, so I settled for a temporary solution.

Reinmar commented 9 years ago

But is this temporary solution safe at this very moment? What if simplesample should be processed by some other function? In such case we'll override that processing by copying this file again. I'm pretty sure you'll be able to find a place at the end of the process chain to plug-in this replacement.

Tade0 commented 9 years ago

I just found that my solution doesn't even work properly in all cases(namely when creating a *.zip file), so I guess I should go with your idea.

Tade0 commented 9 years ago

Update: setting the version is now done in the builder app.js, instead through grunt.

Reinmar commented 9 years ago

I rebased the whole branch and pushed more into it. There were couple of issues still and some things must be still fixed – namely, the docs are not built in a right place. To reproduce, just generate and offline version and see that the docs are somewhere inside build/guides/ rather than inside a concrete build.

Please make sure to test all possible packages (offline, online, zips and dev versions).

Reinmar commented 9 years ago

Still the same issue with build/guides/. I sent you details on priv.

Tade0 commented 9 years ago

The temporary guides directory is now removed after the build.

Reinmar commented 9 years ago

Cool! Everything works fine now. Merged to master with 0e5612e.