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

Support alternate rendering directory with internal property #2670

Merged
merged 1 commit into from May 9, 2017

Conversation

Projects
None yet
5 participants
@robander
Member

robander commented May 2, 2017

I have several custom transform types that override the html5, xhtml, and eclipsehelp transform types. In each case, my custom transform type is almost the same as the default, but I need to return a zip of the result to the output directory.

Currently, this is very difficult (particularly with eclipsehelp). The default output.dir property is used in many targets that either copy or render files. My custom transform type - which again is almost exactly the same as html5 - has to override all of preprocess so that I can redirect targets that copy images. It has to override the main HTML5 steps for the same reason.

This pull request allows custom transform types to use an init style parameter that specifies a relative path within the temporary directory. When specified, all steps that render or copy files (apart from the log) will be placed in that directory within temp. This allows a final custom target to zip the files or do any other custom post-processing before they are returned. For example, an ebook transform type could use this to render all HTML5 / CSS / images in a temporary directory, add a step to generate the TOC and manifest, and return a complete zipped EPUB.

The new internal property is (for now) named ditaot.render.dir, and defaults to the output directory. This is fully backwards compatible; any custom transform type that uses output.dir will still work, and any value specified for output.dir will still work. This will only change processing for transform types that choose to override the render directory before running build-init.

For myself - this property is most important for html5, xhtml, and eclipsehelp (I override all of these to create a zip). It's less urgent for other transform types, but potentially useful for the same reasons, so this pull request supports the new property for all default transform types. For that same reason, I've added a new parameter dita.map.render.dir that could deprecate the existing dita.map.output.dir. This wasn't 100% necessary, but particularly with Eclipse Help, it felt wrong to have all the topics go to a render directory while map based files all went to a output directory.

The property used to do this is currently temp.render.dir.name. (I tried a lot of different property names and wasn't 100% happy with any of them.) It must be a relative path that will be created within the temporary directory. For example, if the value is zip_dir, all output files will be placed inside temp/zip_dir.

Whether or not that is the final parameter name, I'd like to create some initial doc for the parameter, but would like input from @infotexture on where that should go. (It's not really something that should be set on the command line, rather it's there for anybody doing custom transform types. If you set it on the command line with a default transform type, you won't get any output.)

@robander robander added the feature label May 2, 2017

@robander

This comment has been minimized.

Member

robander commented May 2, 2017

I added a test case for this to IntegrationTest.java but it doesn't seem to work.

In the real world - this new property will only be set by custom transform types during initialization. I tried creating a test case that forces the property to be set, which should result in zero files in the output directory. This is how I tested every default transform type locally, and that test passed locally. But, on Travis it gives a Null pointer error when comparing an empty output directory to an empty exp directory.

The test case I created, which I should probably remove with a new commit, was:

    @Test
    public void testrenderdir() throws Throwable {
        builder().name("renderdir")
                .transtype(xhtml)
                .input(Paths.get("simplemap.ditamap"))
                .put("temp.render.dir.name", "hidden")
                .put("clean.temp", "no")
                .test();
    }

I'll leave it for now in case anybody has an idea on how tweak this to provide a meaningful test.

@robander

This comment has been minimized.

Member

robander commented May 2, 2017

FWIW -- here is the batch file I used to test this on Windows with every default transform type. For each transform type, I ran a normal build which places everything in out/transtypenormal. Then I ran a transform that set temp.render.dir.name=testInTemp on the command line; the output directory was set to out/transtypeempty (which should be empty), and temp directory was saved in out/transtypetemp (which should have a valid copy of all output in out/transtypetemp/testInTemp).

BATCH FILE UPDATED based on code review to change test parameter name

call bin\dita -install

set TRANSTYPE=tocjs
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%normal -f %TRANSTYPE% -temp temp -Dclean.temp=no
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%empty -f %TRANSTYPE% -temp out\%TRANSTYPE%temp -Dclean.temp=no -Dtemp.output.dir.name=testInTemp

set TRANSTYPE=eclipsehelp
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%normal -f %TRANSTYPE% -temp temp -Dclean.temp=no
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%empty -f %TRANSTYPE% -temp out\%TRANSTYPE%temp -Dclean.temp=no -Dtemp.output.dir.name=testInTemp

set TRANSTYPE=html5
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%normal -f %TRANSTYPE% -temp temp -Dclean.temp=no
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%empty -f %TRANSTYPE% -temp out\%TRANSTYPE%temp -Dclean.temp=no -Dtemp.output.dir.name=testInTemp

set TRANSTYPE=htmlhelp
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%normal -f %TRANSTYPE% -temp temp -Dclean.temp=no
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%empty -f %TRANSTYPE% -temp out\%TRANSTYPE%temp -Dclean.temp=no -Dtemp.output.dir.name=testInTemp

set TRANSTYPE=javahelp
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%normal -f %TRANSTYPE% -temp temp -Dclean.temp=no
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%empty -f %TRANSTYPE% -temp out\%TRANSTYPE%temp -Dclean.temp=no -Dtemp.output.dir.name=testInTemp

set TRANSTYPE=pdf2
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%normal -f %TRANSTYPE% -temp temp -Dclean.temp=no
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%empty -f %TRANSTYPE% -temp out\%TRANSTYPE%temp -Dclean.temp=no -Dtemp.output.dir.name=testInTemp

set TRANSTYPE=troff
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%normal -f %TRANSTYPE% -temp temp -Dclean.temp=no
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%empty -f %TRANSTYPE% -temp out\%TRANSTYPE%temp -Dclean.temp=no -Dtemp.output.dir.name=testInTemp

set TRANSTYPE=xhtml
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%normal -f %TRANSTYPE% -temp temp -Dclean.temp=no
call bin\dita -i docsrc\samples\hierarchy.ditamap -o out\%TRANSTYPE%empty -f %TRANSTYPE% -temp out\%TRANSTYPE%temp -Dclean.temp=no -Dtemp.output.dir.name=testInTemp
@xephon2

This comment has been minimized.

Contributor

xephon2 commented May 2, 2017

Could you please explain, why "a second temp dir" is necessary? I developed something similar, which works a little different. I set the out.dir to a processing dir and then add a copy step to copy everything from that dir to a calculated output dir. I calculated the dir by reading metadata after the preprocessing and then save it to properties. The copy step is, of course, called after the publication. This was a little tricky to implement, because there is no after-publication extension point for any reason...

@jelovirt jelovirt added the ant label May 2, 2017

<target name="dita2tocjs" depends="test-init, build-init, preprocess, tocjsInit, map2tocjs, topic2tocjs, tocjsDefaultOutput">
</target>
<target name="test-init">
<property name="render.path.within.temp" value="flerp"/>

This comment has been minimized.

@jelovirt

jelovirt May 2, 2017

Member

Debug code?

This comment has been minimized.

@robander

robander May 2, 2017

Member

Sigh. Yes.

@@ -29,7 +32,7 @@ See the accompanying LICENSE file for applicable license.
<isset property="output.file" />
</not>
</condition>
<property name="output.file" value="${output.dir}${file.separator}toctree.js"/>
<property name="output.file" value="${ditaot.render.dir}${file.separator}toctree.js"/>

This comment has been minimized.

@jelovirt

jelovirt May 2, 2017

Member

The ditaot. prefix for the property doesn't follow any of the assorted conventions we currently have. The only real consistent naming scheme we have is prefixing internal properties with dita.. It's not really good, but I don't want to introduce ditaot. since with would be yet another convention. My preference is probably dita.output.dir, because that would at least be consistent with dita.temp.dir.

This comment has been minimized.

@robander

robander May 2, 2017

Member

I liked render just because it's not the actual "output" as we've used the term -- it's not the file(s) returned to the user, rather it's the place we put everything we're rendering. But I really don't care too much so I'm happy to change.

This comment has been minimized.

@robander

robander May 2, 2017

Member

Along with this - I think it's best to change the override property from temp.render.dir.name to temp.output.dir.name.

<condition property="ditaot.render.dir" value="${dita.temp.dir}${file.separator}${temp.render.dir.name}" else="${output.dir}">
<isset property="temp.render.dir.name"/>
</condition>
<property name="ditaot.render.dir" location="${output.dir}"/>

This comment has been minimized.

@jelovirt

jelovirt May 2, 2017

Member

This is not needed because the else-branch of the preceding condition will set the property value.

This comment has been minimized.

@robander

robander May 2, 2017

Member

Yep - leftover from when I first set it up as 2 conditionals. Thanks.

@jelovirt

Few comments

@infotexture

As for where to document this, perhaps the Internal Ant properties topic would be appropriate?

Though not complete, that topic is intended to describe any such properties that are used internally (and may be overridden by customizations), but that are not exposed for general use through the command line.

@robander

This comment has been minimized.

Member

robander commented May 2, 2017

Could you please explain, why "a second temp dir" is necessary?

@xephon2 Of course your method works. TL;DR version: I can't really control output.dir itself in my environment - and many targets put results there directly. I'm currently duplicating each of those targets just to avoid this. I'm looking for something easier that works across various transform types.

My user community is already setting output.dir today as part of their command (they've been doing this for a long time now, and the value cannot be automated). I think this rules out my doing anything new with the output.dir parameter itself -- I'd either add a whole lot of wrapper code to call a new transform with a new output.dir, or break build files by forcing a change to actual.output.dir.

For my HTML5 output, I basically need the default HTML5 output, plus a navigation file unique to our platform, and I need to return all of that as a zip. In other words, the zip is the output file, rather than the HTML5. To make this work in 2.3 or 2.4, I had to override all of preprocess, so that I could insert my own version of copy-image, copy-flag, and copy-html. I also had to duplicate all of the major HTML5 targets (html5.topics, html5.map, and html5.css) so that the rendered HTML and CSS did not go straight to output.dir. All 6 of these steps are exactly the same as the core OT steps, except that they substitute internal.zip.dir for output.dir.

With this new feature, I'm able to use a single property in my ibmhtml5.init to redirect all of the standard HTML5 output to a temporary location. I no longer need to duplicate any core targets. I can run preprocess, the core HTML5 targets, and then add my own two targets to generate a navigation file + place a zip in output.dir.

I did this once last summer for HTML5. In the fall, I had to extend eclipsehelp so that it produced the same thing, but returned everything as doc.zip. Eclipse has 11 targets that explicitly use output.dir, so I had to duplicate each of those + targets that call them.

Now I need to create another variant of eclipsehelp that tweaks a couple of the 11 steps. I was looking with horror on the idea of duplicating those 11 again, when I realized others might have this problem. I discussed it at the March and April DITA-OT contributor calls and several others found this idea very useful. And that's the long version of 'why the pull request'...

@robander

This comment has been minimized.

Member

robander commented May 2, 2017

@infotexture thanks - that makes sense.

@xephon2

This comment has been minimized.

Contributor

xephon2 commented May 2, 2017

@robander thank you so much for your time and explanation 👍 😄

Support alternate rendering directory with internal property
Signed-off-by: Robert D Anderson <robander@us.ibm.com>

robander added a commit to dita-ot/docs that referenced this pull request May 2, 2017

Describe internal property temp.output.dir.name
Draft description to go with new parameter created with dita-ot/dita-ot#2670
@robander

This comment has been minimized.

Member

robander commented May 9, 2017

@jelovirt Any issues with this one? I think I've addressed all of the code review comments - once integrated, I'm ready to add a couple more fixes that would otherwise create conflicts.

@jelovirt jelovirt merged commit 9d044dc into dita-ot:develop May 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@robander robander deleted the robander:feature/renderdir branch May 9, 2017

infotexture added a commit to dita-ot/docs that referenced this pull request May 10, 2017

Describe internal property temp.output.dir.name (#142)
Draft description to go with new parameter created with dita-ot/dita-ot#2670
@mironovalexey

This comment has been minimized.

Contributor

mironovalexey commented May 12, 2017

Robert, I'm sorry, I still do not understand the need for a second parameter that works just like the output.folder.
Plugin developer can override output.folder and any other DITA variable in this way:

<project name="ibmhtml5">
...
<target name="ibmhtml5">
    <antcall target="ibmhtml5-makehtml">
        <param name="output.dir" value="out/transtypetemp"/>
    </antcall>
    <antcall target="ibmhtml5-makejar"/>
</target>

<target name="ibmhtml5-makehtml"
        depends="ibmhtml5.init,
                 html5.init,
                 build-init,
                 preprocess,
                 html5.topic,
                 html5.map,
                 ...">

    <echo>Temp output folder: ${output.folder}</echo>
</target>

<target name="ibmhtml5-makejar">
    <echo>Final output folder: ${output.folder}</echo>
    <jar ...>...</jar>
</target>

</project>

It seems to me that there's no need to introduce new variable, as it complicates the code and increases the number of parameters.
Your approach implies introducing new "core" properties for each similar task.

@robander

This comment has been minimized.

Member

robander commented May 12, 2017

@mironovalexey I don't think this implies new core properties for similar tasks. (I'd also argue that this one parameter doesn't significantly complicate the code; it also does not introduce a build parameter, as this is something that should only be seen or set by a plugin developer.)

Initially I didn't consider this as something to go back into the core; I implemented workarounds for one transform type last summer, then a new one last fall, and only started thinking about this as a useful core property when I was called on to do it for a third. At that point I saw others expressing needs that would also be greatly simplified by this property. For example, creating a JAR file in #2261, which is one of the things I had to do with my workaround.

Before starting a pull request, I also brought up the idea in our monthly contributor call to see if others thought it useful, and got quite a bit of positive feedback. Others have also run into the need for post-processing and zipping, mostly for variations of HTML5 or EPUB. Allowing all of that to happen within the temp directory simplifies this (apparently) common task, as opposed to creating + deleting a second temp directory outside of the normal build process.

@mironovalexey

This comment has been minimized.

Contributor

mironovalexey commented May 12, 2017

@robander Robert, I just suggest an approach to getting the same result without modifying the base code, like in the above example. We can simply divide the full sequence of Ant targets into number of parts (targets as well) and call them sequentially. Each part can use its own overrode (or not) Ant properties like output.folder.

@robander robander added this to the 2.5 milestone Jun 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment