fix(ci): fix Windows and macOS ngspice installation in GitHub Actions#242
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
xarthurx
left a comment
There was a problem hiding this comment.
Windows test still failed. Also make sure linting is correct
Two separate failures were fixed in commit
Linting already passed in the previous run; all ruff/mypy/CodeQL checks continue to pass with these changes. |
xarthurx
left a comment
There was a problem hiding this comment.
One check: "pre-commit / conventional-commits (pull_request)"
still failed. fix it.
|
@g-braeunlich Could you take over this PR? |
The two bad commits ( The actual code changes are correct (all ruff/mypy/CodeQL checks pass). To get the conventional-commits check to pass, the simplest fix is to squash-merge this PR on the GitHub side — GitHub's squash-merge feature lets you set a single clean commit message that satisfies the linter. Alternatively, a force-push of a clean 3-commit history would also work. |
|
Why all that empty "initial plan" commits? |
Should be ready to merge based on this explanation. |
|
The
|
See. |
33261b7 to
ba3ebd4
Compare
|
I did a force push with a suggestion on how I would organize the changes into commits. |
65a0445 to
9bae90e
Compare
|
I replaced all commits relating to the windows setup with a single commit just changing the version to 26.2 again.
|
@g-braeunlich No, I dont think so. To the best of my knowledge, NGSPICE is only used in Power Electronics. This was failing before that commit. |
|
@mkeeler43 The test in this case is about thermoelastic2d and not for ngspice / powerelectronic |
Yes I know @g-braeunlich, but ngspice is what is causing problems and has nothing to do with thermoelastic2d. Zhao and Gabe mentioned this issue before this commit was made. (Edit) Oh sorry I see you mean the "volume_fraction" issue not the other issue. Yes I think Gabe must have renamed this in the new dataset. @gapaza |
|
@mkeeler43 What do we do about the macos error? |
|
Sorry, regarding the macos error, I actually ment @xarthurx |
@g-braeunlich I think the problem with the thermoelastic data was not caught because the dataset was changed after the commit on huggingface. This commit/merge was made and then @gapaza changed the name of some dataset columns on a dataset reupload to huggingface. It will be resolved. |
|
For macOS we should probably pin Homebrew to Right now Homebrew installs a newer version, and that seems to be the source of the remaining macOS failure. Since Linux and Windows are already being handled with the working version, the matching fix here would be to replace the macOS step in - name: Install ngspice
run: |
brew tap-new local/oldies
brew extract --version=44.2 ngspice local/oldies
brew install local/oldies/ngspice@44.2
brew pin ngspice@44.2
ngspice --versionThat keeps CI off the moving Homebrew |
9bae90e to
9da07d0
Compare
|
Thanks @SoheylM |
9da07d0 to
c8e812f
Compare
|
Update: I could fix the URL in the brew script. But now I get the next error: |
…ice executable
- ngspice.py: add shutil.which("ngspice") to Windows path search so ngspice
installed via Chocolatey (in PATH) is found automatically; also simplify the
path resolution logic and update the error message
- Make Windows version detection in ngspice.py more robust by falling back to
--version subprocess if docs folder PDF is not found
c8e812f to
0c32575
Compare
|
@g-braeunlich I'm fine with this for the time being. We should make a note in a issue to come back to this just so we don't forget. |
|
Just did it now. Feel free to complain! From my side, I will now approve everything I did not do mysef. |
|
@g-braeunlich I dug into this locally, and I think disabling the macOS ngspice path in CI for now is the right intermediate move. What I verified on April 17, 2026:
So I agree with disabling the macOS ngspice check in this PR to unblock things. If we want to re-enable it later, I think there are two real paths:
So from my side: +1 to the temporary disable, and then we can track one of the above as a follow-up issue. |
|
There is no clean way to pin the ngspice version via homebrew. For that, we need to either pin to macos-15 (which is pretty old and is a shaky fix), or - best long term solution - host our own image of ngspice 44.2 and use that. |
FileNotFoundError– fixed with MSYS2 pacman (commit88073ef)timeout-minuteslimit.github/workflows/test.yml: increasetimeout-minutesforwindows-testfrom 30 → 60Original prompt