Skip to content
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

Update Mac Installer #7323

Merged
merged 21 commits into from
Jul 15, 2019

Conversation

jmarrec
Copy link
Contributor

@jmarrec jmarrec commented Jun 3, 2019

Pull request overview

Fix #5410: Symlink to /usr/local/bin on Mac
Fix #7298: Mac EP-Launch-Lite and IDFVersionUpdater work on mac.

The Gist here is that darwinpostflight.sh is something only used by PackageMaker (the old packaging program used for Mac), and now we use QtIFW, so instead you should use some install operations (cf doc).

I also added actual components that you can choose to install or not (all enabled by default):

  • Create Symlinks: if enabled, requires admin rights (you have to type your password), used to symlink to /usr/local/bin and to symlink the man page too. If disabled, then you don't need admin right (it won't ask for your password)
  • DataSets, WeatherData and ExampleFiles: if disabled, you won't get these folders and their content.

Screen Shot 2019-06-03 at 4 37 18 PM

I have tested the installer and the various combinations of stuff you can install and diff'ed the directory structure compared to an installer of current develop, and everything does make sense. If you enable eveyrthing, the only difference is that now you don't have "Uninstall Energyplus.app" (since it's superseeded by the "Maintenance Tool" created by QtIFW that will correctly remove the symlinks etc))

Work Checklist

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • At least one of the following appropriate labels must be added to this PR to be consumed into the changelog:
    • Defect: This pull request repairs a github defect issue. The github issue should be referenced in the PR description
    • Refactoring: This pull request includes code changes that don't change the functionality of the program, just perform refactoring
    • NewFeature: This pull request includes code to add a new feature to EnergyPlus
    • Performance: This pull request includes code changes that are directed at improving the runtime performance of EnergyPlus
    • DoNoPublish: This pull request includes changes that shouldn't be included in the changelog

Review Checklist

This will not be exhaustively relevant to every PR.

  • Functional code review (it has to work!)
  • If defect, results of running current develop vs this branch should exhibit the fix
  • CI status: all green or justified
  • Performance: CI Linux results include performance check
  • Unit Test(s)
  • C++ checks:
    • Argument types
    • If any virtual classes, ensure virtual destructor included, other things
  • IDD changes:
    • Verify naming conventions and styles, memos and notes and defaults
    • Open windows IDF Editor with modified IDD to check for errors
    • If transition, add to input rules file for interfaces
    • If transition, add transition source
    • If transition, update idfs
  • If new idf included, locally check the err file and other outputs
  • Required documentation updates?
  • ExpandObjects changes?
  • If output changes, including tabular output structure, add to output rules file for interfaces

jmarrec added 18 commits May 29, 2019 15:09
…Installer

* commit '727be933c7f1f180dee1d86d73b2ec9cb81d2cfd':
  link to /usr/local/bin instead
…ghts aren't required. Properly deletes symlink when uninstall too. Still a problem with the man pages
…ix aren't referenced anywhere. Uninstall EnergyPlus.app is deprecated for the QtIFW "Maintenance Tool"
@jmarrec jmarrec changed the title Pr opened/fix 5410 7298 mac installer Update Mac Installer Jun 3, 2019
@jmarrec jmarrec added the Defect Includes code to repair a defect in EnergyPlus label Jun 5, 2019
@jmarrec jmarrec requested a review from Myoldmopar June 5, 2019 17:43
@nrel-bot-2b
Copy link

@jmarrec @lgentile it has been 28 days since this pull request was last updated.

Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a coding standpoint, this looks good. The IFW installer is so much nicer than PackageMaker. I built this locally and tested it out successfully. Thanks for cleaning out so much stuff. I'll push a small commit to change one string and then this can merge in.

@@ -59,8 +59,6 @@ if(NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES)
"MinSizeRel" "RelWithDebInfo")
endif()

set(CPACK_PACKAGE_CONTACT "Edwin Lee <[email protected]>")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take that, Edwin Lee!

@@ -1,4 +1,6 @@
set( CPACK_PACKAGE_VENDOR "US Department of Energy" )
set(CPACK_PACKAGE_VENDOR "US Department of Energy" )
set(CPACK_PACKAGE_CONTACT "Edwin Lee <[email protected]>")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nevermind, there he is.

install(FILES "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/workflows/transition.py" DESTINATION "workflows/")
install(FILES "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/Energy+.idd" DESTINATION "PreProcess/IDFVersionUpdater/" RENAME "V9-2-0-Energy+.idd" )

# Workflow stuff, takes about 40KB, so not worth it proposing to not install it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, and it would stink for a user to have to add it in later if they use EP-Launch-3


cpack_add_component(WeatherData
DISPLAY_NAME "Weather Data"
DESCRIPTION "A few EPW files"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to make a change here to "A set of EPW files" rather than "A few..."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "EPW Weather Files"? to mimic "IDF Example Files"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that expand to "EnergyPlus Weather Weather Files"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasondegraw EPW is actually a recursive acronym where the E stands for EnergyPlus Weather Data Files, so the full name is "EnergyPlus Weather Data Files Plus Weather Weather Files ... the 3rd Baron of Townsville"

As bad as having a PIN number or a VIN number or and IDF file is, I think "EPW Weather Files" will make perfect sense to the users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, yes. Using that same logic it would be "Input Data File Examples" and "EnergyPlus Weather Files".

@Myoldmopar
Copy link
Member

  • Create Symlinks: if enabled, requires admin rights (you have to type your password), used to symlink to /usr/local/bin and to symlink the man page too. If disabled, then you don't need admin right (it won't ask for your password)

On my work machine, it still asks for an admin password if I install it into /Applications even if I don't have symlinks installed. It probably wouldn't have asked if I had installed it into a user directory.

@Myoldmopar Myoldmopar merged commit d72b45d into NREL:develop Jul 15, 2019
@jmarrec jmarrec deleted the PR_opened/Fix_5410_7298_Mac_Installer branch July 16, 2019 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Defect Includes code to repair a defect in EnergyPlus
Projects
None yet
9 participants