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

Use official XSpec repo, not user fork #116

Merged
merged 4 commits into from
May 7, 2024

Conversation

galtm
Copy link
Collaborator

@galtm galtm commented Mar 26, 2024

Committer Notes

The reason for temporarily using a user fork of the XSpec repo is no longer relevant, so it makes sense to use the official XSpec repo.

#47 was from July 2023, while XSpec v2.3 supporting Saxon 12.x was released in October 2023.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you squashed any non-relevant commits and commit messages? [instructions]
  • Do all automated CI/CD checks pass?

Changes to Core Features:

N/A because this is not a change in a core feature.

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you included examples of how to use your new feature(s)?
  • Have you updated all website and readme documentation affected by the changes you made? Changes to the website can be made in the website/content directory of your branch.

The reason for temporarily using a user fork
of the XSpec repo is no longer relevant.

usnistgov#47 was from July 2023, while XSpec v2.3 was
released in October 2023.
@galtm galtm requested a review from a team as a code owner March 26, 2024 12:48
Copy link

github-actions bot commented Mar 26, 2024

XSpec Test Results

  2 files  ±0  40 suites  ±0   0s ⏱️ ±0s
105 tests ±0  90 ✅ ±0  15 💤 ±0  0 ❌ ±0 
114 runs  ±0  99 ✅ ±0  15 💤 ±0  0 ❌ ±0 

Results for commit 8e3d07e. ± Comparison against base commit 7637dd2.

♻️ This comment has been updated with latest results.

@galtm
Copy link
Collaborator Author

galtm commented Mar 26, 2024

In the "CI / Test and Collect Results (pull_request)" log, I see

  From https://github.com/xspec/xspec
   * branch              d7fa240395dbae3886c558d2ee5bd98a97fcd73d -> FETCH_HEAD
  Submodule path 'support/xspec': checked out 'd7fa240395dbae3886c558d2ee5bd98a97fcd73d'

instead of the following, which is from #115 from yesterday.

Submodule 'support/xspec' (https://github.com/AirQuick/xspec.git) registered for path 'support/xspec'
...
  From https://github.com/AirQuick/xspec
   * branch              d7fa240395dbae3886c558d2ee5bd98a97fcd73d -> FETCH_HEAD
  Submodule path 'support/xspec': checked out 'd7fa240395dbae3886c558d2ee5bd98a97fcd73d'

Having the same branch as before this change looks suspicious, as if the image being obtained from xspec/xspec is from last July. I'll try specifying branch = master to see if that comes out different.

@galtm
Copy link
Collaborator Author

galtm commented Mar 26, 2024

Even with branch = master the log still says

From https://github.com/xspec/xspec
   * branch              d7fa240395dbae3886c558d2ee5bd98a97fcd73d -> FETCH_HEAD
  Submodule path 'support/xspec': checked out 'd7fa240395dbae3886c558d2ee5bd98a97fcd73d'

and https://github.com/xspec/xspec/tree/d7fa240395dbae3886c558d2ee5bd98a97fcd73d says, "d7fa240 · 8 months ago".

So, I'm not sure if I should be doing something different.

@aj-stein-nist
Copy link
Contributor

aj-stein-nist commented Mar 26, 2024

Hi @galtm, did you do the following?

  1. Change the .gitmodules file?
  2. Run git submodule update --init --recursive after 1
  3. Then git add the .gitmodules and git add the xspec directory in question after 2.
  4. Then git commit ...

I sometimes forget about 3 and 4 so in short the remote has changed but not the particular commit to retrieve from the remote, does that help?

@galtm
Copy link
Collaborator Author

galtm commented Mar 26, 2024

Thanks a lot for the advice, @aj-stein-nist !

My local result of step 2 still referred to the old SHA.

 git submodule update --init --recursive
Submodule path 'support/xspec': checked out 'd7fa240395dbae3886c558d2ee5bd98a97fcd73d'

I found some useful instructions at https://devconnected.com/how-to-add-and-update-git-submodules/#Update_a_Git_Submodule and now the latest log for this PR says

 Submodule path 'support/xspec': checked out '5069d13709373bd2a545947a2d059e81f29bd828'

Now, I have a question for you: do you want XSpec v2.3.2 or the release candidate for v3.0? My 3rd commit is for the release candidate, but if you want v2.3.2, I'll redo my process to give you v2.3.2 instead. The breaking changes in v3.0 are that XSLT code coverage requires Saxon 12.4 and Schematron testing uses SchXslt under the hood instead of the skeleton implementation. Everything else should be backward compatible.

@wendellpiez
Copy link
Collaborator

wendellpiez commented Mar 26, 2024 via email

@wendellpiez
Copy link
Collaborator

wendellpiez commented Mar 27, 2024

@galtm wrt the question of XSpec version - we need to consider this, also/esp given work over at usnistgov/xslt3-functions#9 where same question comes up.

Everything else being equal, I guess I am inclined to peg it to the latest release, not the latest RC, unless there is a reason to prefer the RC. But almost any reason would do.

Thanks for the initiative here. It is spurring work. (If there isn't a more up to date metaphor to use 🚀 )

@galtm
Copy link
Collaborator Author

galtm commented Mar 28, 2024

I am inclined to peg it to the latest release, not the latest RC

OK. Locally, I have a commit that pegs it to v2.3.2, which at this moment is the latest release. Let me know if/when I should push that commit to this branch for this PR.

@wendellpiez
Copy link
Collaborator

Excellent, thanks! @galtm does it make sense to do this as a precautionary measure?

Now I'm actually thinking that with more refactoring soon to come (if/as xslt3-functions repo comes into play), having this in place and current will be verry niice, helping to align the dependencies and removing one potential source of instability. Thanks again for advancing -

@galtm
Copy link
Collaborator Author

galtm commented Apr 1, 2024

does it make sense to do this as a precautionary measure?

I guess. Should I go ahead?

Instead of v3.0
@galtm
Copy link
Collaborator Author

galtm commented Apr 1, 2024

The 4th commit in this PR, 8e3d07e, is for XSpec v2.3.2, and the test log includes Submodule path 'support/xspec': checked out '9cef3713f5a7aaedf38a4ec2190ba3262d51cd72'. That corresponds to the commit for https://github.com/xspec/xspec/releases/tag/v2.3.2 .

@wendellpiez
Copy link
Collaborator

@galtm after thrashing on my part this is good to go right? (I have also fixed up that other repository.)

@galtm
Copy link
Collaborator Author

galtm commented Apr 3, 2024

this is good to go right?

Yes, I believe so!

@wendellpiez wendellpiez merged commit df05440 into usnistgov:develop May 7, 2024
3 checks passed
@galtm galtm deleted the xspec-module-v2.3 branch May 7, 2024 21:10
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.

None yet

3 participants