Fix #7256: Linux sh installer is broken in 9.1.0 #7313

jmarrec · 2019-05-27T15:52:21Z

Pull request overview

Fix #7256: Linux sh installer is broken in 9.1.0

The linux installer was broken in 9.1.0 and until now since the tar.gz (which is also embedded in the .sh script) has one level less, that top level EnergyPlus-X.Y.Z one (cf https://github.com/NREL/EnergyPlus/pull/7148/files#diff-41362486261ba172ec8a6127c1785ccf).

I modified the CPack.SGTZ_Header.sh.in to ask for the full install directory - as opposed to the parent dir and appending EnergyPlus-X.Y.Z - as I think it make a lot more sense anyways. To illustrate:

Before:

input_dir is asked to user, defaults to /usr/local
Installs into input_dir/EnergyPlus-X.Y.Z (because the embedded tar.gz had that extra level)

Now:

input_dir is asked to user, defaults to /usr/local/EnergyPlus-X.Y.Z
Installs into input_dir (because the embedded tar.gz does not have that extra level)

I haven't tested mac and Windows but they should be unaffected. I made one extra commit at the end that might need reversing in case it doesn't work (mac is more at risk in that case)

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.

…ECTORY globally Mac should default to the same as currently harcoded.

@Myoldmopar

@Myoldmopar there might be a good reason I'm not seeing though

Myoldmopar

General comments right now. Honestly the actual fix for the Linux installer is fine, it's the extra stuff that worries me.

Myoldmopar · 2019-05-28T11:14:10Z

cmake/CPack.STGZ_Header.sh.in

+
+# Reset
+Color_Off='\033[0m'				# Text Reset
+URed='\033[4;31m'					# Red


Are these valid for non-bash shells? This script's shebang line is attempting to run sh not bash. I have been searching for non-bash color output codes and literally everything is focusing on bash terminals. This could be because most people think of bash as the only terminal on Linux, so the answers are confusing online, or it's because the color codes really are specific to bash. Anyway, please verify. If we aren't sure, then I'm inclined to turn the color back off to avoid breaking other shells.

It works for sh (dash) on ubuntu 16.04 at least as I tested the installer, but see a MCVE below.

URed='\033[4;31m' Color_Off='\033[0m' sh -c "echo 'coucou ${URed}COUCOU${Color_Off}'"

I don't care if you want me to remove them though, just let me know.

After doing some research, it looks like we are on the up and up. According to Debian policy, any script that includes a shebang line with sh must simply be POSIX-compliant, and the color codes are POSIX-fine. So that's good. If anything were to change, I'd consider making the red code not underlined because it looks like it is a hyperlink. I'd go with bold red (1;31m).

nside dash directly:

Done, replaced with

BRed='\033[1;31m' # Bold Red

Will push after testing on mac.

(FYI I have a handy colors.sh that you can find on OpenStudio-resources here)

Myoldmopar · 2019-05-28T11:14:56Z

cmake/CPack.STGZ_Header.sh.in

+				exit 1;;
+		esac
+
+default_install_directory="/usr/local/${package_name}"


And this is the actual fix, OK.

Myoldmopar · 2019-05-28T11:17:30Z

cmake/CPack.STGZ_Header.sh.in

+		rm -f "${link_directory}/BasementGHT.idd"
+		rm -f "${link_directory}/convertESOMTR"
+		rm -f "${link_directory}/EnergyPlus"
+		rm -f "${link_directory}/energyplus"


Myoldmopar · 2019-05-28T11:20:49Z

cmake/Install.cmake

-  set(CPACK_PACKAGE_VERSION "${CPACK_PACKAGE_VERSION_MAJOR}.${CPACK_PACKAGE_VERSION_MINOR}.${CPACK_PACKAGE_VERSION_PATCH}")
-  set(CPACK_IFW_TARGET_DIRECTORY "/Applications/${CMAKE_PROJECT_NAME}-${CPACK_PACKAGE_VERSION_MAJOR}-${CPACK_PACKAGE_VERSION_MINOR}-${CPACK_PACKAGE_VERSION_PATCH}")
+  # CPACK_IFW_TARGET_DIRECTORY defaults to “@ApplicationsDir@/CPACK_PACKAGE_INSTALL_DIRECTORY”
+  #set(CPACK_IFW_TARGET_DIRECTORY "/Applications/${CMAKE_PROJECT_NAME}-${CPACK_PACKAGE_VERSION_MAJOR}-${CPACK_PACKAGE_VERSION_MINOR}-${CPACK_PACKAGE_VERSION_PATCH}")


Since Windows is working, and we had to add code in here for IFW to work, I was hoping to see Linux-only changes in this PR. It was really annoying to get IFW working with the CMake logic we have in here. I'm curious what this and the CPACK_PACKAGE_INSTALL_DIRECTORY line above are effectively changing.

When doing CPack stuff, there are variables that are defined for all Cpack generators, this being one. I think it makes more sense to use these instead of harcoding generator-specific variables, because it would help with consistency, and be more future proof.

That being said in our case it's actually a change that should have no effect as best as I can tell:

On mac: The CPACK_IFW_TARGET_DIRECTORY will be set based on this on mac, which will match the current explicit setting. (cf the block on which you commented above)

On Windows NSIS will override it anyways (https://github.com/jmarrec/EnergyPlus/blob/develop/cmake/CMakeCPackOptions.cmake.in#L6:L9).

On Linux it will not be used because we use the STGZ header script. It would be used if we used the .deb package (and would be correct...)

If that makes you uneasy I can just revert it, or least harcode the CPACK_IFW_TARGET_DIRECTORY for mac. Let me know

cf "Variables common to all CPack generators": specifically https://cmake.org/cmake/help/v3.2/module/CPack.html#variable:CPACK_PACKAGE_INSTALL_DIRECTORY

Have you tested building a package on a Mac? If not, I can try from this branch to see what happens. As long as it works, I'm OK with the cleanup, I'm just nervous from trying to get it to work with IFW in the first place.

I just kicked one on mac a minute ago (after needed to install QtIFW and stuff) to test it out.

I reverted back to harcoded CPACK_IFW_TARGET_DIRECTORY because it would default to “@ApplicationsDir@/CPACK_PACKAGE_INSTALL_DIRECTORY” which ends up being /Users/julien/Applications/EnergyPlus-9-2-0 and not /Applications/EnergyPlus-9-2-0 like I had expected.

…to PR_opened/Fix_7256_Installer

Myoldmopar · 2019-05-29T02:07:59Z

cmake/Install.cmake

@@ -241,7 +244,6 @@ endif()

 if( APPLE )
  set(CPACK_PACKAGE_DEFAULT_LOCATION "/Applications")
-  set(CPACK_PACKAGE_VERSION "${CPACK_PACKAGE_VERSION_MAJOR}.${CPACK_PACKAGE_VERSION_MINOR}.${CPACK_PACKAGE_VERSION_PATCH}")


Is this variable not picked up by anyone else later in the IFW build? It looks to be unused, but I'm still really nervous about touching this since it works perfectly fine right now.

It's already defined on line 10: https://github.com/NREL/EnergyPlus/pull/7313/files/cfbb86e3d7fd51e3c80dcf5102a1ab6937fd8121#diff-41362486261ba172ec8a6127c1785ccfR10

Only difference is that it's defined with the build sha too:

set(CPACK_PACKAGE_VERSION "${CPACK_PACKAGE_VERSION_MAJOR}.${CPACK_PACKAGE_VERSION_MINOR}.${CPACK_PACKAGE_VERSION_PATCH}-${CPACK_PACKAGE_VERSION_BUILD}")

I'll revert that too.

Myoldmopar · 2019-05-29T02:09:36Z

cmake/CompilerFlags.cmake

+    # On Mac with Ninja (kitware binary for fortran support) and brew gfortran, I get build errors due to this flag.
+    if (NOT BUILD_FORTRAN)
+      AddFlagIfSupported(-fcolor-diagnostics COMPILER_SUPPORTS_fdiagnostics_color)
+    endif()
  endif()


Interesting, I've never had any issues. Is there any chance we should protect this more specifically for your corner case so that it still works OK for other builds?

I've never had any issues with it either until today, and I think I had built fortran on mac with ninja before. I don't think it matters that much if no one bumps into this anyways, but there should be a way here to use add_compile_options with generator expressions to limit to everything BUT fortran instead of disabling for everyone.

Yeah add_compile_options($<$<NOT:$<COMPILE_LANGUAGE:Fortran>>:${flag}>) works

Myoldmopar · 2019-05-29T02:09:47Z

cmake/CPack.STGZ_Header.sh.in

-      rm -f "${link_directory}/SlabGHT.idd"
-      rm -f "${link_directory}/Transition-V8-2-0-to-V8-3-0"
-      rm -f "${link_directory}/V8-2-0-Energy+.idd"
-      rm -f "${link_directory}/V8-3-0-Energy+.idd"


Yeah good call.

…if passed.

This reverts commit cff2c95.

Myoldmopar · 2019-05-31T02:27:04Z

Everything looks to be in great shape now. I think that diff should be unrelated to the work here. I'll build an installer and test it out and this will drop in if everything is good. Thanks @jmarrec

Myoldmopar · 2019-06-01T19:28:49Z

Fetched latest develop just now and merged it in and built an installer. Verified that the latest develop build is wonky and that this new one provides a proper install. We should add an install note that the install path now accepts the full path to the E+ install folder, not the parent folder.

I'm building on Mac now and will verify that the install package over there is fine.

Myoldmopar · 2019-06-03T21:17:07Z

My IFW package build using this branch on Mac seemed to have trouble. I'm verifying my develop build works OK before any further commentary.

Myoldmopar · 2019-06-03T21:19:28Z

OK, my develop build is funny too. @jmarrec can you confirm your package build is OK on Mac? Like the actual artifact is a full package?

If it looks OK to you then I'll probably merge this in and immediately try a CI package build. If CI can also build it OK then I'll just figure out what's up with my personal build sometime later. If CI can't build it either then we'll need to address it right away.

…d/Fix_7256_Installer * commit '2eb3f96cc3cbfdcd2da70adc3be6f740ae1a297c': (35 commits) Encode degree signs properly as UTF-8 Convert DataStringGlobals.in.cc to UTF-8 Fix check_non_utf8.py script Make E+ fixture use DataStringGlobals::clear_state Add a clear_state function to DataStringGlobals.in.cc Revert "Merge pull request NREL#7317 from NREL/HotFixFiniteDiffTestDisable" Fix IDD unit and validation script Make sure file key is in json output for decent CI Fix licensetext for Python 3 support Fixup change_version script for Python 3 Remove unused 179D script, clean up validate_idd_units and add to custom_check Fix license text python 3 support Fix non-utf8 character check for Python 3, remove unused script Temporarily disable finite diff tests to clean up unit test issue Remove unneeded old python scripts, continuing to test others Minor tidying up in fluid properties code - should be no functional change; Move transition support files to PreProcess subdir and add all recent older versions of support files - fixes NREL#7248 Add protection and clarification for check_pr_labels Change to code and unit test Update to test files. Zone is now fully controlled by single reference point. Remove commented out "using" directive in test ...

jmarrec · 2019-06-04T09:02:44Z

@Myoldmopar merged develop in, and rebuilt on mac. Definitely a full package, same exact size (59.7MB) as regular develop, installs just fine.

Myoldmopar · 2019-06-04T10:19:40Z

Sounds good, thanks @jmarrec. I'll merge this in shortly and trigger a CI package build.

jmarrec added 4 commits May 27, 2019 16:51

retab (whitespace change only)

530a1cb

Fix the linux install script (sh)

1c5c04e

Found a typo in ParametricPreprocessor directory name

b590728

Optional: Tiny change in Install.cmake: set CPACK_PACKAGE_INSTALL_DIR…

04be6de

…ECTORY globally Mac should default to the same as currently harcoded.

jmarrec requested a review from Myoldmopar May 27, 2019 15:52

Delete some stuff that doesn't make much sense to me

e300e9b

@Myoldmopar there might be a good reason I'm not seeing though

jmarrec added the Defect Includes code to repair a defect in EnergyPlus label May 27, 2019

Myoldmopar reviewed May 28, 2019

View reviewed changes

jmarrec added 4 commits May 28, 2019 15:38

Replace with bold red instead of underline red

e1e4d9e

Revert to hardcoded CPACK_IFW_TARGET_DIRECTORY

957e92f

Disable fcolor-diagnostics when building with Ninja and fortran on mac

17be559

Merge remote-tracking branch 'origin/PR_opened/Fix_7256_Installer' in…

cfbb86e

…to PR_opened/Fix_7256_Installer

Myoldmopar reviewed May 29, 2019

View reviewed changes

jmarrec added 4 commits May 29, 2019 12:19

Put back CPACK_PACKAGE_VERSION override to exclude build sha for IFW

00498dd

Use generator expressions to exclude fortran only

346d501

Safer: Explictly check for fortran and cxx flag and enable only them …

cff2c95

…if passed.

Revert previous commit as it actually doesn't work on my mac

4e0c902

This reverts commit cff2c95.

Myoldmopar merged commit cbf96ed into NREL:develop Jun 4, 2019

jmarrec deleted the PR_opened/Fix_7256_Installer branch June 4, 2019 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #7256: Linux sh installer is broken in 9.1.0 #7313

Fix #7256: Linux sh installer is broken in 9.1.0 #7313

jmarrec commented May 27, 2019

Myoldmopar left a comment

Myoldmopar May 28, 2019

jmarrec May 28, 2019

Myoldmopar May 28, 2019

jmarrec May 28, 2019

jmarrec May 28, 2019 •

edited

Loading

Myoldmopar May 28, 2019

Myoldmopar May 28, 2019

Myoldmopar May 28, 2019

jmarrec May 28, 2019

jmarrec May 28, 2019

Myoldmopar May 28, 2019

jmarrec May 28, 2019

jmarrec May 28, 2019

Myoldmopar May 29, 2019

jmarrec May 29, 2019

Myoldmopar May 29, 2019

jmarrec May 29, 2019

jmarrec May 29, 2019

Myoldmopar May 29, 2019

Myoldmopar commented May 31, 2019

Myoldmopar commented Jun 1, 2019

Myoldmopar commented Jun 3, 2019

Myoldmopar commented Jun 3, 2019

jmarrec commented Jun 4, 2019

Myoldmopar commented Jun 4, 2019

Fix #7256: Linux sh installer is broken in 9.1.0 #7313

Fix #7256: Linux sh installer is broken in 9.1.0 #7313

Conversation

jmarrec commented May 27, 2019

Pull request overview

Work Checklist

Review Checklist

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrec May 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented May 31, 2019

Myoldmopar commented Jun 1, 2019

Myoldmopar commented Jun 3, 2019

Myoldmopar commented Jun 3, 2019

jmarrec commented Jun 4, 2019

Myoldmopar commented Jun 4, 2019

jmarrec May 28, 2019 •

edited

Loading