[Refactor] First-run and re-configuration directories #54
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
g5t
commented
Sep 30, 2022
g5t
commented
- The (re)configuration process uses default directories which are only appropriate for one version of McStas and McXtrace on a macOS system. For all other systems, as a first step a user must create an appropriate configuration file to use McStasScript.
- This commit adds the 'default' paths specified in the McStasScript documentation for Windows, Linux, and MacOS systems and choosed between them based on the current system platform. It then attempts to find the respective binary locations for McStas and McXtrace and uses their locations, if found, in place of the default paths.
- If McStasScript is installed into a non-user-writable directory, this method will fail as there is no way to create the first configuration file. Instead the configuration.yaml should be written into the home directory, possibly via the confuse library.
- The (re)configuration process uses default directories which are only appropriate for one version of McStas and McXtrace on a macOS system. For all other systems, as a first step a user *must* create an appropriate configuration file to use McStasScript. - This commit adds the 'default' paths specified in the McStasScript documentation for Windows, Linux, and MacOS systems and choosed between them based on the current system platform. It then attempts to find the respective binary locations for McStas and McXtrace and uses their locations, if found, in place of the default paths. - If McStasScript is installed into a non-user-writable directory, this method will fail as there is no way to create the first configuration file.
- The simple and complex instrument tests rely on having version 2.x of McStas configured for use via `mcrun`. They make use of a component, PSDlin_moonitor, which has a property `nx` under McStas v2 that was renamed to `nbin` under McStas v3. Before this commit there was no way to know which version of `mcrun` would be called by the `Instr` object and therefore no way to overcome this limitation. - This commit adds a property attribute to the Instr class which checks the output of the `mcrun --version` call and returns a `packaging.version.Version` or `packaging.version.LegacyVersion` object, as appropriate. The two instrument test are also modified to use this new property to modify the appropriate component attribute based on the version of `mcrun` being used. - The `executable_version` property of the Instr object is a public property, which may not be desirable.
- The process.run call to mcrun does not check whether the return code has been set to a non-zero value, indicating that an error has occurred. This can lead to situations in testing where an error occurs but is not detected until later, adding to debugging difficulty. - The process.run().returncode is now checked and an error is raised in cases where it is non-zero. - The call to mcrun should never produce a non-zero value if it has succeeded but there may be unintended cases where it returns zero despite an error condition being detected. In such cases the new check will not detect the error and the function will continue as before. A possibly more troublesome change is in raising an error at all; if users expected only a warning when an error has occurred they may now have anhandled exception in their code which would cause a halt to execution.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.