Skip to content

Conversation

rouson
Copy link
Member

@rouson rouson commented Sep 2, 2017

Avg response time coverage on master
Issue Stats Codecov branch

Summary of changes

Updated the definition of opencoarrays_version in windows-install.sh to reflect a months-old change in version numbering.

Rationale for changes

Build on Windows Subsystem for Linux was failing.

Additional info and certifications

This pull request (PR) is a:

  • Bug fix
  • Feature addition
  • Other, Please describe:

I certify that

  • I reviewed and followed the contributing guidelines, including
    - Increasing test coverage for all feature-addition PRs
    - Increasing test coverage for all bug-fix PRs for which there
    does not already exist a related test that failed before the PR
    - At least maintaining test coverage for all other PRs
    - Ensuring that all tests pass when run locally
    - Naming PR to indicate work in progress (WIP) and to attach the PR
    to the appropriate bug report or feature request issue
    - White space (no trailing white space or white space errors may
    be introduced)
    - Commenting code where it is non-obvious and non-trivial
    - Logically atomic, self consistent and coherent commits
    - Commit message content
    - Waiting 24 hours before self-approving the PR to give another
    OpenCoarrays developer a chance to review my proposed code

@CLAassistant
Copy link

CLAassistant commented Sep 2, 2017

CLA assistant check
All committers have signed the CLA.

@rouson
Copy link
Member Author

rouson commented Sep 2, 2017

@zbeekman I went back to commits as old as late April and found that the Windows installer has been broken for a rather long time. This caused me to realize that we could actually be testing this script in Travis-CI. Because the script runs under Windows Subsystem for Linux, it's actually just a bash script that expects to run on Ubuntu 16.04 or later. There is nothing in the script specific to Windows. Please set up for Travis-CI to test this script if it's easy to do so. Let me know if I should create an issue for this.

@codecov
Copy link

codecov bot commented Sep 2, 2017

Codecov Report

Merging #441 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #441   +/-   ##
=======================================
  Coverage   35.29%   35.29%           
=======================================
  Files           3        3           
  Lines        2105     2105           
  Branches      350      350           
=======================================
  Hits          743      743           
  Misses       1154     1154           
  Partials      208      208

@zbeekman
Copy link
Collaborator

zbeekman commented Sep 2, 2017

ah, sorry, this was my fault. I made the change that originally caused this and neglected to update the windows script.

LGTM

Approved with PullApprove

@zbeekman
Copy link
Collaborator

zbeekman commented Sep 2, 2017

The shebang at the start of the file is wrong: https://github.com/sourceryinstitute/OpenCoarrays/pull/441/files#diff-761c977a48b8e2a4ee665b286772ca5dR1

missing a ! after the leading #

also, shellcheck has many warnings, most common of which is to use $( ... ) instead of

` ... `

@zbeekman
Copy link
Collaborator

zbeekman commented Sep 2, 2017

also style issues:

$ ./developer-scripts/style.pl windows-install.sh
windows-install.sh[112]: [ -z "${LOG_LEVEL:-}" ] && emergency "Cannot continue without LOG_LEVEL. "
windows-install.sh[115]: if [[ "${arg_d}" == "${__flag_present}" ]]; then[ ]
windows-install.sh[117]:    if [ "$(( LOG_LEVEL < print_debug_only ))" -ne 0 ]; then
windows-install.sh[147]: export this_script="$(basename "$0")"
windows-install.sh[161]:   opencoarrays_version=$text_before_language_keyword
windows-install.sh[166]: export build_path="${OPENCOARRAYS_SRC_DIR%/}"/prerequisites/builds/opencoarrays/$opencoarrays_version
windows-install.sh[197]:   if [[ $major_release -lt 16  ]]; then
windows-install.sh[199]:   elif [[ $major_release -eq 16  ]]; then
windows-install.sh[200]:     if [[ $minor_release -lt "04"  ]]; then
windows-install.sh[209]:     info "$opencoarrays_version"
windows-install.sh[230]: [ ]
windows-install.sh[233]:   if ! type "$CMAKE" >& /dev/null; then
windows-install.sh[236]:   if ! type "$CXX" >& /dev/null; then
windows-install.sh[239]:   if ! type "$FC" >& /dev/null; then
windows-install.sh[242]: [  ]
windows-install.sh[271]: [  ]
windows-install.sh[274]:   if [[ -d "$build_path" ]]; then
windows-install.sh[275]:     rm -rf "$build_path"
windows-install.sh[277]:   mkdir -p "$build_path"
windows-install.sh[278]:   cd "$build_path"
windows-install.sh[280]:   info "FC=\"$FC\" CC=\"$CC\"  \"$CMAKE\" \"$OPENCOARRAYS_SRC_DIR\" -DCMAKE_INSTALL_PREFIX=\"$install_prefix\""
windows-install.sh[281]:   FC="$FC" CC="$CC" "$CMAKE" "$OPENCOARRAYS_SRC_DIR" -DCMAKE_INSTALL_PREFIX="$install_prefix"
windows-install.sh[283]:   info "make -j $arg_j"
windows-install.sh[284]:   make -j $arg_j
windows-install.sh[288]:   if [[ -f "$install_prefix"/lib/libcaf_mpi.a && -f "${install_prefix}/bin/caf"  && -f "${install_prefix}/bin/cafrun"  ]]; then
windows-install.sh[290]:     info "$install_prefix"
windows-install.sh[293]:     emergency "$install_prefix"
windows-install.sh[296]:   loopback_line=`grep $NAME /etc/hosts`
windows-install.sh[299]:     info "127.0.0.1 $NAME"

They show up in red when you actually run the script... the ASCII control characters to produce the red don't copy here on Github...

@zbeekman
Copy link
Collaborator

zbeekman commented Sep 2, 2017

@rouson hmmm it also appears that git wasn't configured correctly, so it didn't record the author info correctly. As a consequence, it thinks that the CLA needs to be signed again. I can probably forcibly fix this myself.

@zbeekman
Copy link
Collaborator

zbeekman commented Sep 5, 2017

yeah, it looks like you didn't configure git properly in a new environment:

Author:  <rouson@DESKTOP-S9SB06H.localdomain>
 Date:   Fri Sep 1 17:54:31 2017 -0700

@zbeekman zbeekman force-pushed the fix-windows-installer branch from 45171d0 to 61de5e4 Compare September 5, 2017 16:03
@zbeekman
Copy link
Collaborator

zbeekman commented Sep 6, 2017

LGTM

let's merge after tests run & pass

Approved with PullApprove

@rouson rouson merged commit d649a2f into master Sep 7, 2017
@rouson rouson deleted the fix-windows-installer branch September 7, 2017 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants