Add Ant build support for Schematron #160

cirulls · 2018-01-24T17:26:38Z

Summary

This pull request integrates #138 submitted by @markdunnoup and allows to run XSpec tests for Schematron using the ant build script. For further detail, please see conversation in #138.

This pull request also adds two bats tests checking that the ant build script works correctly for XSLT and Schematron.

As this was approved by @markdunnoup and me in #138 (comment), I'm merging into master.

Further work

I will update the documentation in the wiki.

@AirQuick: would you be able to add similar tests for the batch script? Note that ant is installed by default in Travis but I had to install additional ant libraries in .travis.yaml. Ant may not be installed by default on AppVeyor.

AirQuick

I have some questions.
I didn't track the original PR in details. Sorry if they're already answered.

AirQuick · 2018-01-25T07:49:00Z

build.xml

-			</antcall>
-		</sequential>
-	</macrodef>
+  <!-- OXYGEN PATCH needed to create a custom Saxon, for example  -warnings:recover -strip:none -opt:10 -dtd:off -l:off -versionmsg:off -expand:on -outval:fatal -val:lax -->


These OXYGEN PATCH phrases are confusing. As the changes are being committed here, they aren't patches anymore.

I removed the OXYGEN PATCH title but kept the comment for documentation purposes in 3b1a6f4

AirQuick · 2018-01-25T08:23:32Z

build.xml

+          name="http://saxon.sf.net/feature/allow-external-functions" 
+          value="true"/>
+      </factory>
+    </xslt>


saxon.custom.options is defined before, but not used here. Why? (Is it possible in the first place?)

I guess saxon.custom.options is used by oXygen. I can see it is defined with this comment:

 <property name="saxon.custom.options" value=""/> 

I wonder if it is is worth removing oXygen customizations that are currently not needed for this feature and work with oXygen for a smooth integration of XSpec. This should also address #138 (comment) raised by @markdunnoup.

Shall we just remove the saxon.custom.options if not used? Do you see any side effect?

We may just remove it, but...
This PR changes <java> to <xslt>. Is it easy to implement the feature of saxon.custom.options in <xslt>? I'm afraid not.

I reintroduced the use of <java> instead of <xslt> in 3bcb2cd#diff-2cccd7bf48b7a9cc113ff564acd802a8R227. I have kept saxon.custom.options as a result.

AirQuick · 2018-01-25T08:25:54Z

build.xml

+      force="true"
+      >     
+      <factory name="net.sf.saxon.TransformerFactoryImpl" />     
+    </xslt>


oXygen's build.xml enables XML catalog here and in other places. Whey are they not incorporated in this PR?

I reintroduced XML catalog in various places in 3bcb2cd following the current version of build.xml packaged with oXygen 19. I had to add xml-resolver.jar as additional jar library in .travis.yaml and appveyor.yaml otherwise tests failed on my local machine with the following error message:

[java] Transformation failed: Failed to load Apache catalog resolver library [java]

Tests are passing on Travis but failing on AppVeyor. The culprit is the ant test for Schematron which fails with this error:

[java] Error on line 12 column 87 of demo-03.xsl: [java] Malformed URL C:/projects/xspec/tutorial/schematron/demo-03-sch-compiled.xsl(base [java] file:/C:/projects/xspec/tutorial/schematron/xspec/demo-03.xsl): unknown protocol: c [java] Malformed URL C:/projects/xspec/tutorial/schematron/demo-03-sch-compiled.xsl(base file:/C:/projects/xspec/tutorial/schematron/xspec/demo-03.xsl)

The issue is the path file:/C, I suspect the XML resolver or the java command may have caused this. I also wonder if this is a similar problem I described in #138 (review) (see where I talk about file:/ in Unix and Windows) but this time it's problematic for Windows.

I won't have more time today to debug this and unfortunately Windows is not my forte. Could you have a look at this failing test in case you spot any glaring error I made?

Thanks for the improvements!
I looked at the intermediate file (demo-03.xsl) and found a fake URI:

<xsl:import href="c:/xspec/tutorial/schematron/demo-03-sch-compiled.xsl"/> ... <xsl:variable name="x:stylesheet-uri" as="xs:string" select="'c:/xspec/tutorial/schematron/demo-03-sch-compiled.xsl'"/>

Probably 4e2ed51#diff-2cccd7bf48b7a9cc113ff564acd802a8R129 makes it.
I'm fixing it.

AirQuick · 2018-01-25T08:29:12Z

build.xml

+    xspec.phase                Required if XSpec describes a Schematron file, with value "#ALL" 
+    xspec.compiled.xsl.dir     Folder in which to place the compiled XSLT file generated from a Schematron file
+                               [Optional; the default is a subfolder "xspec" of the folder containing the XSpec file]
+


Not all parameters are explained. Are they missed? Or, will be explained only in Wiki?

I added more documentation in e9861f8. This will be used to update the wiki. I left the documentation on how to run it in oXygen as it will be useful for the oXygen integration and for all those who want to use it before this feature is fully integrated in oXygen.

AirQuick · 2018-01-25T09:40:43Z

build.xml

+
+  <!-- Copy of original XSpec file
+  will be overwritten if XSpec applies to Schematron -->
+  <property name="xspec.compiled.xml" location="${xspec.base.dir}/${xspec.base}-compiled-oxygen.xspec"/>


Why does this file name have -oxygen?
It was puzzling to find such a file when I ran build.xml outside oXygen.

I removed the -oxygen from the file name in 3b1a6f4#diff-2cccd7bf48b7a9cc113ff564acd802a8R113. This should be unnecessary once we fully integrate with the oXygen ant build file.

AirQuick · 2018-01-25T09:50:12Z

test/xspec.bats

+
+
+@test "running XSpec via ant for XSLT support" {
+    run ant -buildfile ${PWD}/../build.xml -Dxspec.xml=${PWD}/../tutorial/escape-for-regex.xspec -lib /tmp/xspec/saxon/saxon9he.jar -lib /tmp/ant/lib/xmltask.jar -lib /tmp/ant/lib/ant-contrib.jar 


-lib /tmp/xspec/saxon/saxon9he.jar should be -lib ${SAXON_CP} to coordinate with other tests and help changing Saxon versions/editions.

-lib /tmp/ant/lib/xmltask.jar -lib /tmp/ant/lib/ant-contrib.jar may be shortened to -lib /tmp/ant/lib

-Dclean.output.dir=true should be added, otherwise git status finds a temp file after testing locally.

I implemented this feedback in this branch in cd32771

AirQuick · 2018-01-25T10:07:04Z

test/xspec.bats

+
+@test "running XSpec via ant for Schematron support" {
+    echo ${PWD}
+    run ant -buildfile ${PWD}/../build.xml -Dxspec.xml=${PWD}/../tutorial/schematron/demo-03.xspec -lib /tmp/xspec/saxon/saxon9he.jar -lib /tmp/ant/lib/xmltask.jar -lib /tmp/ant/lib/ant-contrib.jar -Dtest.type=s -Dxspec.project.dir=${PWD}/../ -Dxspec.compiled.xsl.dir=${PWD}/../tutorial/schematron -Dxspec.phase=#ALL


After running this command line locally, git status finds tutorial/schematron/demo-03-sch-compiled.xsl even with -Dclean.output.dir=true. Is it a bug of clean.output.dir?

I remember @markdunnoup mentioning this in #138 (comment):

The "cleanup" target is not fully compatible with XSpec for Schematron yet. When ${clean.output.dir} is set to true, it removes SVRL files which the HTML report links to if the XSPec fails.

The problem here is that a file is not removed. The known problem in the cited comment is that a file is removed. Are they the same thing?
Anyway, if cleanup is incomplete, it would be nice if known issues or remaining tasks are commented in the file.

I added the removal of the temporary file in 3b1a6f4#diff-2cccd7bf48b7a9cc113ff564acd802a8R252, the XSL compiled file was missing. I tested it locally and it removes the temporary files when the option clean.output.dir is set to true via the command line.

AirQuick · 2018-01-25T16:01:10Z

@AirQuick: would you be able to add similar tests for the batch script?

I pushed the tests to this branch.

appveyor.yml on AppVeyor defines an environment variable XSPEC_ANT_LIB and installs external Jar files there.
xspec-bat.cmd runs Ant tests only when the XSPEC_ANT_LIB is defined. (XSLT and Schematron)

I made so, because I usually run xspec-bat.cmd on a local computer where Ant and libraries are not readily available.

- See comment in #160 (review) - Use SAXON_CP instead of path to Saxon - Shorten path to ant libraries - Add clean option to remove temp file

AirQuick · 2018-01-25T17:52:10Z

test/xspec.bats

+
+
+@test "running XSpec via ant for XSLT support" {
+    run ant -buildfile ${PWD}/../build.xml -Dxspec.xml=${PWD}/../tutorial/escape-for-regex.xspec -lib ${SAXON_CP} -lib /tmp/ant/lib 


If I recall correctly, -Dclean.output.dir=true is necessary here too.

Good catch! I added it in ab86e9f

- Remove comments OXYGEN_PATCH - Remove misleading -oxygen from compiled file name - Add removal of temporary compiled file - Add clean option in test for XSLT support

- Add clean option in test for XSLT support

- Use <java> instead of <xslt> - Enable XML catalog - Add xml-resolver.jar to CI configuration - Format and indent code

because the original comment is no longer true. Initial template can be specified: https://github.com/xspec/xspec/blob/4e2ed51f98b7f530b21bbbc3742eb97fe9712294/build.xml#L223-L225

This build.xml expects Saxon Jar to be specified by the -lib command line option

@Validate

because makeurl/@Validate=true by default. Also rename the property to better represent its nature.

AirQuick · 2018-01-29T19:57:36Z

The issue encountered on Travis was not reproduced locally even with the same version (1.9.3) of Ant. Something may be special on Travis's Ant or environment, I don't know. So I had to install a cleaner Ant on Travis.

While doing so, I noticed Bats was readily available on Travis. So I removed its installation process from .travis.yml.

As we've put some installation processes, .travis.yml and appveyor.yml are getting crowded. So I tidied them up a bit.

<fail> is reinstated in 52c123c.
With <fail>, I added a few more tests in xspec.bats and xspec-bat.cmd. In the course of adding the tests, I had to inject a test failure into tutorial/schematron/demo-04.xspec.

I hope I'm done. Please review the changes which have been accumulated unexpectedly. Beware; I'm not familiar with Ant :)

this would be the last pull request to be merged before the new release. Is this fine with you?

No problem.

It will fail.

xspec.bat

AirQuick · 2018-01-30T08:25:27Z

Fixed a bug that xspec.dir is not always honored in Schematron.
Removed yet another unnecessary parameter xspec.compiled.xsl.
I believe filePrefix comments in Add Ant build support for XSpec for Schematron; fix Schematron compile bug #138 are obsolete.

I hope I'm really done now :(

Various changes have been made since the initial commit. For writing the usage in Wiki, you might want to take a look at xspec.bats and xspec-bat.cmd in test/ and an AppVeyor build log (actual command lines are printed there) as well as the usage comment in build.xml.

cirulls · 2018-01-30T16:47:49Z

Thanks for your work and the many improvements. I'll review your changes this week and let you know. If everything is fine, I'll approve it and merge this branch into master.

AirQuick · 2018-01-31T13:43:49Z

Sorry for adding commits. They are supposed to just augment tests and simplify build.xml.

cirulls · 2018-02-01T23:14:21Z

@AirQuick: Thanks a lot for your work, it's amazing how you improved this original pull request! Thanks for adding additional tests (always appreciated!), for removing unncessary dependencies, and simplifying the build script. Thanks also for making the Travis and AppVeyor configurations closer and for standardizing on tools like curl.

I checked your commits and they look good to me. I just have few questions:

I saw you installed Ant 1.9.8 as it is the Ant version installed in oXygen. I read your comment about the trouble you had with Ant installed by default on Travis. For documentation purposes, I'd like to know if we should state that XSpec has been tested with v1.9.8 or if XSpec actually requires Ant v1.9.8. I think the former but it would be good if you can confirm.
I'll update the documentation on Ant and will ask you to review it.
I noticed you introduced some JavaScript in make-path.js which is called in the build file. Was it problematic to do this with <makeurl>? I'm just a bit concerned about introducing another programming language in the code base for maintainability reasons and wonder if the same result could be achieved differently. This shouldn't stop this pull request being merged (which is my priority) but I just wanted to mention it.

Let me know what you think. I'm aiming to merge this pull request during the weekend and then working on #139 for releasing XSpec v1.0.0.

AirQuick · 2018-02-02T08:06:07Z

Based on the Ant manual, the new build.xml requires Ant 1.9.1. I tested with Ant 1.9.1 and it worked. You can confirm it here. So only 1.9.1 or later should be required.
I still do not know why the preinstalled Ant (1.9.3) on Travis fails. The error message is weird. If I install the same version of Ant separately on Travis, it works.

<makeurl> converts file path to URL. What we need is a reverse function (converts URL to file path). I couldn't find a smarter way of achieving it without external language.

cirulls · 2018-02-02T10:42:02Z

Thanks for your replies. I'll mention in the documentation that Ant 1.9.1 is the minimum requirement (I didn't know about if and unless in Ant, I'm glad they have been introduced so we don't have to use the ant-contrib library).

I can see the rationale for using a scripting programming language and I can see it is also one of the suggested solutions for a similar question on StackOverflow. It makes sense.

I'll merge this pull request during the weekend and review the documentation on the wiki.

cirulls · 2018-02-04T21:34:10Z

@AirQuick: I merged this pull request into master and deleted branch review/138 (if you think of other changes, please use a new branch and open a new pull request). I updated the documentation on the wiki, in particular the section Running XSpec's Ant build file from the command line. Could you please have a look at some point? Feel free to add changes in the documentation as we can't open pull request there.

AirQuick · 2018-02-05T01:02:04Z

Updated the wiki:

Removed the section Using the xspec macro, because the macro has been removed.
Moved lib from the section Useful Properties to its own section, because lib is not a property.
Removed the phrase and are passed to the Ant build script as option using the syntax -Dproperty-value unless otherwise specified, because it is included in the list of You can override the defaults by predefining the property:.
Other minor fixes.

markdunnoup · 2018-02-05T11:44:12Z

Thanks very much to both @AirQuick and @cirulls for your efforts in improving and locking down this request!

cirulls · 2018-02-05T14:04:52Z

@AirQuick: thanks for updating the wiki, it's much clearer now.
@markdunnoup: I'll close #138.

* 'master' of git://github.com/xspec/xspec: Add a pom file to publish XSpec as a Maven artifact (xspec#156) Add Ant build support for Schematron (xspec#160) Add tests for CSS inline (xspec#159) Improvements to HTML report (xspec#135) Run tests with Saxon 9.8.0.7, 9.7.0.21 and 9.7.0.19 (xspec#157)

Sandro Cirulli added 7 commits January 23, 2018 23:45

Add ant build file without filePrefix (review #138)

4e2ed51

Update travis configuration to latest master

f34c574

Merge branch 'master' into review/138

4676c4a

Add tests for running XSpec via ant

e7c05f3

debug

c86f6bf

make file paths explicit

7e28610

fix typo in env variable

1295cc4

cirulls self-assigned this Jan 24, 2018

cirulls added enhancement test labels Jan 24, 2018

Merge branch 'master' into review/138

68e4bc6

AirQuick reviewed Jan 25, 2018

View reviewed changes

AirQuick added 2 commits January 26, 2018 00:30

Install Ant and libraries on AppVeyor

7243d0c

Run tests for Ant on Windows

45a005f

Implement changes requested by AirQuick

cd32771

- See comment in #160 (review) - Use SAXON_CP instead of path to Saxon - Shorten path to ant libraries - Add clean option to remove temp file

AirQuick reviewed Jan 25, 2018

View reviewed changes

Sandro Cirulli and others added 10 commits January 25, 2018 17:56

Implement other changes requested by AirQuick

3b1a6f4

- Remove comments OXYGEN_PATCH - Remove misleading -oxygen from compiled file name - Add removal of temporary compiled file - Add clean option in test for XSLT support

Implement changes request by AirQuick

ab86e9f

- Add clean option in test for XSLT support

Add documentation

e9861f8

Implement changes requested by AirQuick

3bcb2cd

- Use <java> instead of <xslt> - Enable XML catalog - Add xml-resolver.jar to CI configuration - Format and indent code

Add clean.output.dir=true to Ant tests on Windows

30584be

Update comment for why java task is used

1d977c1

because the original comment is no longer true. Initial template can be specified: https://github.com/xspec/xspec/blob/4e2ed51f98b7f530b21bbbc3742eb97fe9712294/build.xml#L223-L225

Do not specify another Saxon Jar in classpath

1b62355

This build.xml expects Saxon Jar to be specified by the -lib command line option

Replace propertyregex with makeurl

34ca473

Make URL after creating file

c72b6c0

because makeurl/@Validate=true by default. Also rename the property to better represent its nature.

Replace tab with space

8633f96

AirQuick added 2 commits January 30, 2018 02:54

Tidy up temp dir

8317272

Reinstate fail in build.xml

52c123c

AirQuick added 6 commits January 30, 2018 13:11

Add a test for xspec.dir

310ae0a

It will fail.

Fix xspec.dir by providing missing test_dir

5061429

xspec.compiled.xml should be the same as xspec.sh and

0b1481b

xspec.bat

Remove unnecessary compiled.xsl.dir

e15d941

Tested on oXygen 19.1 and made minor tweaks

ad72889

Retry once if cleanup failed

1dd9357

AirQuick added 2 commits January 31, 2018 22:12

Test xspec.project.dir

a691952

Externalize helpers

d578eba

cirulls added this to the v1.0.0 milestone Feb 1, 2018

cirulls merged commit 63cf95d into master Feb 4, 2018

cirulls deleted the review/138 branch February 4, 2018 21:30

cirulls mentioned this pull request Feb 4, 2018

Add Ant build support for XSpec for Schematron; fix Schematron compile bug #138

Closed

This was referenced Feb 15, 2018

Schematron tests run using the ANT build.xml always fail #168

Closed

Schematron Ant: Do not set bogus phase parameter when phase is undefined #169

Merged

AirQuick mentioned this pull request Mar 13, 2019

Fix issue 542 (Fails to resolve URI on Windows when imported via catalog) #544

Merged

cirulls mentioned this pull request Sep 5, 2019

Automatically generate release notes #601

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Ant build support for Schematron #160

Add Ant build support for Schematron #160

cirulls commented Jan 24, 2018

AirQuick left a comment

AirQuick Jan 25, 2018

cirulls Jan 25, 2018

AirQuick Jan 25, 2018

cirulls Jan 25, 2018

AirQuick Jan 25, 2018

cirulls Jan 25, 2018

AirQuick Jan 25, 2018

cirulls Jan 25, 2018

AirQuick Jan 26, 2018

AirQuick Jan 25, 2018

cirulls Jan 25, 2018

AirQuick Jan 25, 2018

cirulls Jan 25, 2018

AirQuick Jan 25, 2018

cirulls Jan 25, 2018 •

edited

AirQuick Jan 25, 2018

cirulls Jan 25, 2018

AirQuick Jan 25, 2018

cirulls Jan 25, 2018 •

edited

AirQuick commented Jan 25, 2018

AirQuick Jan 25, 2018

cirulls Jan 25, 2018

AirQuick commented Jan 29, 2018

AirQuick commented Jan 30, 2018 •

edited

cirulls commented Jan 30, 2018

AirQuick commented Jan 31, 2018

cirulls commented Feb 1, 2018

AirQuick commented Feb 2, 2018

cirulls commented Feb 2, 2018

cirulls commented Feb 4, 2018

AirQuick commented Feb 5, 2018

markdunnoup commented Feb 5, 2018

cirulls commented Feb 5, 2018



		@test "running XSpec via ant for XSLT support" {
		run ant -buildfile ${PWD}/../build.xml -Dxspec.xml=${PWD}/../tutorial/escape-for-regex.xspec -lib /tmp/xspec/saxon/saxon9he.jar -lib /tmp/ant/lib/xmltask.jar -lib /tmp/ant/lib/ant-contrib.jar

Add Ant build support for Schematron #160

Add Ant build support for Schematron #160

Conversation

cirulls commented Jan 24, 2018

Summary

Further work

AirQuick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cirulls Jan 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cirulls Jan 25, 2018 • edited

Choose a reason for hiding this comment

AirQuick commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AirQuick commented Jan 29, 2018

AirQuick commented Jan 30, 2018 • edited

cirulls commented Jan 30, 2018

AirQuick commented Jan 31, 2018

cirulls commented Feb 1, 2018

AirQuick commented Feb 2, 2018

cirulls commented Feb 2, 2018

cirulls commented Feb 4, 2018

AirQuick commented Feb 5, 2018

markdunnoup commented Feb 5, 2018

cirulls commented Feb 5, 2018

cirulls Jan 25, 2018 •

edited

cirulls Jan 25, 2018 •

edited

AirQuick commented Jan 30, 2018 •

edited