-
Notifications
You must be signed in to change notification settings - Fork 425
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Windows installer always register ocx files #7979
Conversation
👍 👍 👍 👍 |
@Myoldmopar @jmarrec Tested this locally, which only proves it did not break anything. Tried to have a user test this fix on a system which exhibited the missing ocx problem, but there were security problems with downloading the package exe so the user was not able to test. Unless another user shows up with the problem, I'm not sure if we'll be able to really prove this works. |
Can you unregister an ocx file and then test. |
Tested locally by unregistering |
@rraustad Thanks. |
// Register it: Only for "OCX" | ||
// On some systems these files may be present but not properly registered, so always register here | ||
// If it's a .ocx (case insensitive), we save it to be registered | ||
if (systemArray[i].toLowerCase().indexOf(".ocx") !== -1) { | ||
dllsToReg.push(targetFile); |
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.
Moved this out of the if (!installer.fileExists(targetFile)) {
block so that the ocx files are always registered, even if they already were present.
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.
This is a great change.
// Register it: Only for "OCX" | ||
// On some systems these files may be present but not properly registered, so always register here | ||
// If it's a .ocx (case insensitive), we save it to be registered | ||
if (systemArray[i].toLowerCase().indexOf(".ocx") !== -1) { | ||
dllsToReg.push(targetFile); |
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.
This is a great change.
Your testing results are sufficient, changes look good, and CI won't show anything on this anyway. Merging. Thanks @mjwitte ! |
Pull request overview
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repairIf any diffs are expected, author must demonstrate they are justified using plots and descriptionsIf any defect files are updated to a more recent version, upload new versions here or on DevSupportIf IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange labelIf structural output changes, add to output rules file and add OutputChange labelIf adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependenciesReviewer
This will not be exhaustively relevant to every PR.