Skip to content
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

Some XSpec for testing CI/CD #9

Merged
merged 26 commits into from
Apr 5, 2024

Conversation

wendellpiez
Copy link
Collaborator

@wendellpiez wendellpiez commented Jan 2, 2024

PR revised

This PR now holds an entire XSpec support infrastructure, not just some XSpec testing.

The intent is that repositories that include this one as a submodule should get XSpec support along with it.

PR original description

The XSpec is a very basic standalone, small but real-worldish.

PR #8 is related but the files here are all in the directory-manifest (application) directory, as a typical example.

In the short to medium term we cannot hope to have XSpecs for everything (can we?), but we can at least show how it can be done.

@wendellpiez
Copy link
Collaborator Author

Still working on this - next step is to get some iXML into the XSpec and see how we can run that.

Ant configuration of XSLT extension support for iXML requires the following, which might be pertinent for XSpec running in Saxon under Maven:

@wendellpiez
Copy link
Collaborator Author

As expected, the check fails.

But the traceback doesn't really show the cause -- presumably because the XSpec added to the commit won't compile and run due to its iXML dependency.

Solutions? - find a way to configure Saxon inside the Maven/Xproc configuration - or, run the CI/CD using Ant instead of XProc? which could potentially be configured with the necessary jar libraries and runtime switch to bind the extension functionality (as documented in the branch behind the PR).

…g NineML extension library support in XSLT under XProc
@wendellpiez
Copy link
Collaborator Author

wendellpiez commented Jan 19, 2024

See when we exclude the XSpec from the test run as in the last commit, everything passes.

Reproduce the error by commenting out line 96 of the POM.xml (the exclusion rule).

540ae10#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8R96

@wendellpiez
Copy link
Collaborator Author

wendellpiez commented Mar 26, 2024

Next on this work item:

  • migrate xspec-dev and xspec submodule from metaschema-xslt to this repo
    • putting it under support will make the Make migration easier
    • including support/lib (Morgana) with gitignore and script pre-check
  • port and test supporting/demo pipelines and scripts
  • port Make logic
  • test run interactively
  • rework GHA
  • PR/test

Rationale for all this? See wendellpiez/oscal-xslt#2

@wendellpiez
Copy link
Collaborator Author

This PR has grown considerably since its start as a small test.

@aj-stein-nist, @nikitawootten-nist, @galtm, and friendly readers, I would very much appreciate any tire kicking of the xspec support now included herein (hopefully familiar from the repo it is migrated from).

This repo's CI/CD configuration remains the Maven XSpec set up in the top-level pom.xml. But now there is also an xspec submodule (inside the xspec-dev folder). Is there any chance the CI/CD can use this instead of downloading from Maven?

There is also a pom.xml now in common subdirectory used by scripts.

These could be consolidated; indeed the new scripts could be used under CI/CD as they are in metaschema-xslt.

But this might be good to go for now. Advice, concerns, observations?

Copy link
Collaborator

@galtm galtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some minor comments based on looking at diffs. I'll try to kick the tires a bit during the next few days, and that's why I'm calling this review merely "Comment".

Makefile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
common/pom.xml Outdated
<modelVersion>4.0.0</modelVersion>

<groupId>gov.nist.secauto.oscal.tools.core</groupId>
<artifactId>metaschema-processing-support</artifactId>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it OK that this refers to metaschema even as it moves to this repo?

common/pom.xml Show resolved Hide resolved
xproc3/refresh-lib.sh Outdated Show resolved Hide resolved
xspec-dev/testing/xspec-basic.xspec Show resolved Hide resolved
ixml/readme.md Outdated Show resolved Hide resolved
@wendellpiez
Copy link
Collaborator Author

@galtm I think I've addressed all the comments above making many improvements thanks!

README.md Outdated

- `.github` - for Github CI/CD
- `common` - holds common `bash` scripting - utilities for reuse
- `support` - contains submodules, dynamic resources and libraries (or scripts for downloading them), also [XSpec support in development](support/xspec-dev/),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

support should now say lib, unless support/ will still exist for other things. And "XSpec support in development" is at the top level now, isn't it?

@wendellpiez
Copy link
Collaborator Author

To wrap this up:

  • After considering various aspects, I think the current CI/CD setup is Good Enough for Now in this repository. It has its virtues and is widely used, so if we improved it we could give back.
  • This does have to be documented for developers, not only how to use but also what's going on with xspec-dev. (It can still be used for CI/CD in other repositories.)
  • Check out 80fd48a

Assuming this sounds okay for now, only some docs/enhancement remains for this PR.

Finally, going forward xspec-dev should get some Windows-capable support, shouldn't it? (Since it runs fine when properly invoked.) Do we want an Issue for this?

@galtm
Copy link
Collaborator

galtm commented Apr 4, 2024

Finally, going forward xspec-dev should get some Windows-capable support, shouldn't it? (Since it runs fine when properly invoked.) Do we want an Issue for this?

Yes, yes, please.

"recurse=yes" \
"theme=toybox"

mvn --quiet \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this change. I was able to run the tests via this makefile from a "Git Bash for Windows" window, without having Linux or WSL installed. Before this change, that didn't work for me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I am really curious as to how this change made it work. I guess I have to test on Windows.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually not the change shown, but this change: 5175b3b

Copy link
Collaborator

@galtm galtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thanks for thinking about cross-platform usage, and I think it's fine to improve that incrementally instead of holding up this PR for it.

@wendellpiez wendellpiez merged commit 776430d into usnistgov:main Apr 5, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants