Skip to content

Comments

silo: add patch for constant namescheme issue#3252

Merged
alecbcs merged 8 commits intospack:developfrom
markcmiller86:patch-mcm86-04feb26-const-namescheme
Feb 5, 2026
Merged

silo: add patch for constant namescheme issue#3252
alecbcs merged 8 commits intospack:developfrom
markcmiller86:patch-mcm86-04feb26-const-namescheme

Conversation

@markcmiller86
Copy link
Contributor

We discovered Silo 4.12.0 has an issue with handling of delimiter characters in constant nameschemes.

This PR adds a patch for that.

@markcmiller86
Copy link
Contributor Author

@cyrush or @JustinPrivitera when you have a chance, can you pls take a look at this.

…kcmiller86/spack-packages into patch-mcm86-04feb26-const-namescheme
Copy link
Member

@alecbcs alecbcs left a comment

Choose a reason for hiding this comment

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

Quick suggestion to reference the upstream merge commit instead of copying the patch into the spack-packages repository. Otherwise looks good to me. Thanks @markcmiller86!

@alecbcs alecbcs changed the title Add patch for constant namescheme issue silo: add patch for constant namescheme issue Feb 5, 2026
@alecbcs alecbcs self-assigned this Feb 5, 2026
Copy link
Member

@alecbcs alecbcs left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @markcmiller86!

@alecbcs alecbcs enabled auto-merge (squash) February 5, 2026 17:37
@alecbcs alecbcs merged commit 9c7249e into spack:develop Feb 5, 2026
17 checks passed
@markcmiller86
Copy link
Contributor Author

@alecbcs thanks for quick response and merge.

@haampie
Copy link
Member

haampie commented Feb 9, 2026

@alecbcs / @markcmiller86 did you try this? The patch does not apply.

@markcmiller86
Copy link
Contributor Author

Well...now that I think about it ..I may have tried only the file based one before switching to GitHub ref. What are you seeing? Can you post details.

@cyrush
Copy link
Member

cyrush commented Feb 10, 2026 via email

@alecbcs
Copy link
Member

alecbcs commented Feb 10, 2026

That sounds likely that'd be the issue. Apologies for not noticing that before I suggested the merge commit.

@haampie stepped in and fixed the issue (saved the day?) by using a commit from within the PR. Not usually my preference, but it does work here fairly well.

@markcmiller86
Copy link
Contributor Author

That sounds likely that'd be the issue. Apologies for not noticing that before I suggested the merge commit.

@haampie stepped in and fixed the issue (saved the day?) by using a commit from within the PR. Not usually my preference, but it does work here fairly well.

Ah, ok, now I understand. That PR had work related to fixing Silo testing on macOS as well and only one specific commit from it was the patch to the source code itself to fix the constant namescheme bug. Apologies for not paying attention to that and just assuming it was the same commit hash for that one change in the PR and not the whole PR. The correct commit hash for that one change is 43a52d788a3c15bee3b9391906e8ed276c5a456c.

Thanks @haampie so much for unraveling this to the correct commit hash.

@markcmiller86
Copy link
Contributor Author

The correct commit hash for that one change is 43a52d788a3c15bee3b9391906e8ed276c5a456c.

which I confirm is now on develop.

@markcmiller86 markcmiller86 deleted the patch-mcm86-04feb26-const-namescheme branch February 10, 2026 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants