-
Notifications
You must be signed in to change notification settings - Fork 392
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
Remove EP-Compare #10726
Remove EP-Compare #10726
Conversation
Fixes #7324? 😄 |
I suppose, if you really stretch the concept of a "fix" :-) |
|
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.
There are still a couple of locations that need to be adjusted.
scripts/dev/verify_signature.py
76: "PostProcess/EP-Compare/EP-Compare.app",
src/EP-Launch/epl-ui.frm
6078:utilProg(10).name = "EP-Compare"
6091:utilProg(10).applicationFile = "PostProcess\EP-Compare\EP-Compare.exe"
doc/getting-started/src/getting-started-with-energyplus.tex
37:| +-- EPCompare A graphical tool for comparing two EnergyPlus output sets
@@ -141,8 +140,6 @@ if [ ! "$link_directory" = "n" ]; then | |||
rm -f "${link_directory}/energyplus" | |||
rm -f "${link_directory}/Energy+.idd" | |||
rm -f "${link_directory}/Energy+.schema.epJSON" | |||
rm -f "${link_directory}/EP-Compare" | |||
rm -f "${link_directory}/EPMacro" |
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.
Did you remove that EPMacro line by mistake?
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.
Thanks @jmarrec, the EPMacro line was a mistake and I will make the changes to verify_signature.py and getting started.
The change to EP-Launch, I think I will skip for now. It fails gracefully, which I think is better than just making it disappear from the list of choices. But the next time I work on EP-Launch, I will pick it up for a future release. I added it to this issue for the future
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.
@jmarrec in verify_signature.py, shouldn't the list of the bundled apps include others like IDFVersionUpdater, ExpandObjects and EPMacro and other utilities that are available on MacOS? Or is it only for apps that had not included the source code in the repo?
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.
IDFVersionUpdater is in there. That BUNDLED_APPS is just so I don't recheck something I actually codesigned in the repo directly.
ExpandObjects is a built one (and a fortran exe, not a bundled app) so it'll be checked by default.
EnergyPlus/scripts/dev/verify_signature.py
Lines 73 to 77 in 22e9501
BUNDLED_APPS = [ | |
"PreProcess/EP-Launch-Lite.app", | |
"PreProcess/IDFVersionUpdater/IDFVersionUpdater.app", | |
"PostProcess/EP-Compare/EP-Compare.app", | |
] |
…mare found by Marrec during his review.
I just posted the source and zip of the binaries in an archive repo: |
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.
Love this. Great job cleaning it all out and putting it in a separate archive repo. I'll make sure to note this in the readme and deprecation files that go into the release packages.
GitHub is of course showing that you removed about 235 lines of code with this PR, it would be kinda nice if it actually said how many MB you were also removing with all the binary removals.
It's like 45 MB removed, nice!! OK, this is great. Merging this in. Thanks @JasonGlazer and @jmarrec. Obviously we'll make sure things are happy when I start making release candidates and deal with issues at that point. |
Pull request overview
Removes EP-Compare program
The EP-Compare program did not appear to have any users since it showed an issue when being used that was never reported by any user.
Removes the binaries and updates the documentation and leaves a readme and a brief note in the documentation only.
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Reviewer
This will not be exhaustively relevant to every PR.