Add installer attributes for name, version, platform, and type to .constructor-build.info#1260
Add installer attributes for name, version, platform, and type to .constructor-build.info#1260stacynoland wants to merge 3 commits into
Conversation
…nstructor-build.info
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
| out["installer_name"] = info.get("installer_name", "Unknown") | ||
| out["installer_version"] = info.get("installer_version", "Unknown") | ||
| out["installer_platform"] = info.get("installer_platform", "Unknown") |
There was a problem hiding this comment.
| out["installer_name"] = info.get("installer_name", "Unknown") | |
| out["installer_version"] = info.get("installer_version", "Unknown") | |
| out["installer_platform"] = info.get("installer_platform", "Unknown") | |
| out["installer_name"] = info.get("name", "Unknown") | |
| out["installer_version"] = info.get("version", "Unknown") | |
| out["installer_platform"] = info.get("_platform", "Unknown") |
I looked at the keys again that I mentioned yesterday and I realized that was incorrect. I added this suggestion which should be accurate:
namefromconstruct.yamlversionfromconstruct.yaml_platformis set viamain.pyinstaller_typealso set viamain.py
I also wonder if we should simply have empty strings as default but no strong opinion, I guess the consumer of this data might have an opinion of this.
Perhaps also installer_name and installer_version should not even have a default since something must be fundamentally wrong if it not set.
There was a problem hiding this comment.
They're all guaranteed to be set. I think a simple get() without default values is good here - in the unlikely case that a value isn't set, we'd get null instead of a special string in the JSON file. Empty string is a good alternative, which I'd prefer over "Unknown".
marcoesters
left a comment
There was a problem hiding this comment.
I think we should use a separate file to not conflate installer and build environment information. I would also like to see the changes be covered by a test - I recommend just modifying an existing one.
|
|
||
| files = ( | ||
| "pkgs/.constructor-build.info", | ||
| ".constructor-build.info", |
There was a problem hiding this comment.
I'm not sure about whether to re-use that file. The file contains information about the build process and contains information on conda, python, etc. from the build environment. Adding information of the installer could imply that these are the version of conda, python, etc. that are packaged in the installer.
I think a separate file makes more sense.
| out["installer_name"] = info.get("installer_name", "Unknown") | ||
| out["installer_version"] = info.get("installer_version", "Unknown") | ||
| out["installer_platform"] = info.get("installer_platform", "Unknown") |
There was a problem hiding this comment.
They're all guaranteed to be set. I think a simple get() without default values is good here - in the unlikely case that a value isn't set, we'd get null instead of a special string in the JSON file. Empty string is a good alternative, which I'd prefer over "Unknown".
Description
This PR adds attributes to the
.constructor-build.infofile for persistent storage:installer_nameinstaller_versioninstaller_platforminstaller_typeThis information is being stored so other services, such as telemetry, can find details about the installer used.
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?Add / update necessary tests?Add / update outdated documentation?