-
Notifications
You must be signed in to change notification settings - Fork 14
run: find versions of deps, useful for granular regression testing #20
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
run: find versions of deps, useful for granular regression testing #20
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new Versions class to the Run.py module that detects and retrieves version information for ROOT, Geant4, and CLHEP by executing their respective config commands.
Key Changes:
- Introduced
Versionsclass with methods to query version information from external tools (ROOT, Geant4, CLHEP) - Added error handling for CLHEP version detection when the executable is not available
Comments suppressed due to low confidence (1)
src/pybdsim/Run.py:568
- Except block directly handles BaseException.
except :
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/pybdsim/Run.py
Outdated
| result = _subprocess.run(["root-config","--version"], capture_output=True, text=True) | ||
| return result.stdout.strip() | ||
| except (FileNotFoundError, _subprocess.CalledProcessError): | ||
| return "Not found" | ||
|
|
||
| def _find_geant4_version(self) : | ||
| try : | ||
| result = _subprocess.run(["geant4-config","--version"], capture_output=True, text=True) | ||
| return result.stdout.strip() | ||
| except (FileNotFoundError, _subprocess.CalledProcessError): | ||
| return "Not found" | ||
|
|
||
| def _find_clhep_version(self) : | ||
| try : | ||
| result = _subprocess.run(["clhep-config","--version"], capture_output=True, text=True) |
Copilot
AI
Nov 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception handling catches CalledProcessError but doesn't check the return code before accessing stdout. If the command fails (non-zero exit code), stdout may still contain data but the exception will be caught. Add check=True parameter to subprocess.run() to properly raise CalledProcessError on non-zero exit codes.
| result = _subprocess.run(["root-config","--version"], capture_output=True, text=True) | |
| return result.stdout.strip() | |
| except (FileNotFoundError, _subprocess.CalledProcessError): | |
| return "Not found" | |
| def _find_geant4_version(self) : | |
| try : | |
| result = _subprocess.run(["geant4-config","--version"], capture_output=True, text=True) | |
| return result.stdout.strip() | |
| except (FileNotFoundError, _subprocess.CalledProcessError): | |
| return "Not found" | |
| def _find_clhep_version(self) : | |
| try : | |
| result = _subprocess.run(["clhep-config","--version"], capture_output=True, text=True) | |
| result = _subprocess.run(["root-config","--version"], capture_output=True, text=True, check=True) | |
| return result.stdout.strip() | |
| except (FileNotFoundError, _subprocess.CalledProcessError): | |
| return "Not found" | |
| def _find_geant4_version(self) : | |
| try : | |
| result = _subprocess.run(["geant4-config","--version"], capture_output=True, text=True, check=True) | |
| return result.stdout.strip() | |
| except (FileNotFoundError, _subprocess.CalledProcessError): | |
| return "Not found" | |
| def _find_clhep_version(self) : | |
| try : | |
| result = _subprocess.run(["clhep-config","--version"], capture_output=True, text=True, check=True) |
src/pybdsim/Run.py
Outdated
| result = _subprocess.run(["root-config","--version"], capture_output=True, text=True) | ||
| return result.stdout.strip() | ||
| except (FileNotFoundError, _subprocess.CalledProcessError): | ||
| return "Not found" | ||
|
|
||
| def _find_geant4_version(self) : | ||
| try : | ||
| result = _subprocess.run(["geant4-config","--version"], capture_output=True, text=True) | ||
| return result.stdout.strip() | ||
| except (FileNotFoundError, _subprocess.CalledProcessError): | ||
| return "Not found" | ||
|
|
||
| def _find_clhep_version(self) : | ||
| try : | ||
| result = _subprocess.run(["clhep-config","--version"], capture_output=True, text=True) |
Copilot
AI
Nov 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception handling catches CalledProcessError but doesn't check the return code before accessing stdout. If the command fails (non-zero exit code), stdout may still contain data but the exception will be caught. Add check=True parameter to subprocess.run() to properly raise CalledProcessError on non-zero exit codes.
| result = _subprocess.run(["root-config","--version"], capture_output=True, text=True) | |
| return result.stdout.strip() | |
| except (FileNotFoundError, _subprocess.CalledProcessError): | |
| return "Not found" | |
| def _find_geant4_version(self) : | |
| try : | |
| result = _subprocess.run(["geant4-config","--version"], capture_output=True, text=True) | |
| return result.stdout.strip() | |
| except (FileNotFoundError, _subprocess.CalledProcessError): | |
| return "Not found" | |
| def _find_clhep_version(self) : | |
| try : | |
| result = _subprocess.run(["clhep-config","--version"], capture_output=True, text=True) | |
| result = _subprocess.run(["root-config","--version"], capture_output=True, text=True, check=True) | |
| return result.stdout.strip() | |
| except (FileNotFoundError, _subprocess.CalledProcessError): | |
| return "Not found" | |
| def _find_geant4_version(self) : | |
| try : | |
| result = _subprocess.run(["geant4-config","--version"], capture_output=True, text=True, check=True) | |
| return result.stdout.strip() | |
| except (FileNotFoundError, _subprocess.CalledProcessError): | |
| return "Not found" | |
| def _find_clhep_version(self) : | |
| try : | |
| result = _subprocess.run(["clhep-config","--version"], capture_output=True, text=True, check=True) |
src/pybdsim/Run.py
Outdated
| result = _subprocess.run(["clhep-config","--version"], capture_output=True, text=True) | ||
| parts = result.stdout.split() | ||
| if len(parts) == 2 : | ||
| return parts[1].strip() | ||
| else : | ||
| return "Not found" | ||
| except (FileNotFoundError, _subprocess.CalledProcessError): |
Copilot
AI
Nov 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception handling catches CalledProcessError but doesn't check the return code before accessing stdout. If the command fails (non-zero exit code), the code will attempt to parse stdout before the exception is caught. Add check=True parameter to subprocess.run() to properly raise CalledProcessError on non-zero exit codes.
|
Looks good. Only thing is that technically, your environment can be different from what BDSIM was compiled w.r.t. The other solution would be to add the functionality to BDSIM to report back what it was compiled with. If this technicality is ok for you then fine. |
I think here creating a bdsim-config script or executable (very similar to geant4-config, clhep-config and root-config) would make a lot of sense in this case. Not a lot of work, but not worth it right now I feel. |
471b3da to
12184d3
Compare
Need an easy way to determine what clhep, root and geant4 are installed