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

Cannot execute spago from cygwin terminal #783

Closed
cofad opened this issue Apr 27, 2021 · 9 comments · Fixed by #786
Closed

Cannot execute spago from cygwin terminal #783

cofad opened this issue Apr 27, 2021 · 9 comments · Fixed by #786
Labels

Comments

@cofad
Copy link
Contributor

cofad commented Apr 27, 2021

Issue:

I installed spago using NPM, and when I try to run spago from a cygwin terminal, I get the following:

$ spago --version
/cygdrive/c/Users/user/AppData/Roaming/npm/spago: line 8: C:\Users\user\AppData\Roaming\npm/node_modules/spago/spago: cannot execute binary file: Exec format error
/cygdrive/c/Users/user/AppData/Roaming/npm/spago: line 8: exec: C:\Users\user\AppData\Roaming\npm/node_modules/spago/spago: cannot execute: Permission denied

I can circumvent the issue by manually modifying the "C:\Users\user\AppData\Roaming\npm/node_modules/spago/spago" file to use the "spago.exe" instead of "spago".

I believe the root issue is that the "C:\Users\user\AppData\Roaming\npm/node_modules/spago/spago" file is an ELF file and Cygwin cannot execute that.

Desired Behavior:

Spago can be installed using NPM and run from a Cygwin terminal without modification.

@cofad cofad changed the title Cygwin cannot execute "spago" from cygwin terminal Cannot execute "spago" from cygwin terminal Apr 27, 2021
@cofad cofad changed the title Cannot execute "spago" from cygwin terminal Cannot execute spago from cygwin terminal Apr 27, 2021
@f-f
Copy link
Member

f-f commented Apr 27, 2021

Thanks for the report @cofad! Unfortunately I'm not a Windows expert so I am not quite sure how we'd fix this. Maybe the npm installer could do some detection along these lines, but as far as I know that might break things for other users.

@f-f f-f added the windows label Apr 27, 2021
@cofad
Copy link
Contributor Author

cofad commented Apr 28, 2021

I also tested it in GitBash and it has the same error.

I'm not too familiar with how all the npm scripts get generated, but it seems like it would be possible to add a check somewhere and set the appropriate binary to execute. The current npm script is already checking for cygwin to set the directory base path.

#!/bin/sh
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")

case `uname` in
    *CYGWIN*|*MINGW*|*MSYS*) basedir=`cygpath -w "$basedir"`;;
esac

exec "$basedir/node_modules/spago/spago"   "$@"

I'll do some research to see if I can find an existing node package that handles this scenario.

@f-f
Copy link
Member

f-f commented Apr 28, 2021

@cofad this is the script that currently decides which binary should be downloaded:

spago/npm/install.js

Lines 3 to 9 in ec0a8c3

const request = require("request");
const tar = require("tar");
const version = "PACKAGE_VERSION";
const platform = { win32: "Windows", darwin: "macOS" }[process.platform] || "Linux";
const url = `https://github.com/purescript/spago/releases/download/${version}/${platform}.tar.gz`
request.get(url).pipe(tar.x({"C": './'}));

@cofad
Copy link
Contributor Author

cofad commented Apr 29, 2021

@f-f That install script seems to be working. I can locate the "spago.exe" in the "node_modules/spago" folder and everything looks good with it.

What I think may be the issue is that this npm spago file gets overwritten with the Linux binary. When I replace the Linux binary "spago" file with the npm "spago" file, it works in Cygwin, GitBash, and CMD (PowerShell executes the file in a CMD prompt that closes immediately).

I'm not sure what's causing the overwrite, but I'm going to look into that as possible fix.

@f-f
Copy link
Member

f-f commented Apr 29, 2021

@cofad it's the install script above that does the overwrite, when it detects macOS or Linux

@cofad
Copy link
Contributor Author

cofad commented Apr 29, 2021

I just ran a fresh npm install -g spago using Windows CMD, and the timestamp for "spago.exe" matches the date on the releases page. So that file is definitely being downloaded which is what the install script above does.

The "spago" file has the same timestamp as the rest of the files which makes me think it is being included when the project is published to NPM. I can't find anything which would include that file though, but I'm not familiar with the make files and the build process.

image

I double checked the Linux release on the releases page, and it would have a different timestamp if it were being downloaded from GitHub. I even manually edited the "install.js" file to force the Linux download and confirmed that the timestamp would be different.

@cofad
Copy link
Contributor Author

cofad commented Apr 30, 2021

I also ran npm pack spago to get the tar file that's stored in NPM. It contains the "spago" file that is a Linux binary.

@cofad
Copy link
Contributor Author

cofad commented May 1, 2021

@f-f It appears the Linux binary is being uploaded to NPM instead of this spago script. The change occurred with version 0.19.0. If the correct "spago" file is uploaded to NPM this issue will be resolved.

I can't find the code for npm publish, so I'm not sure what to change to correct this issue.

@cofad
Copy link
Contributor Author

cofad commented May 3, 2021

This issue can be resolved by removing the npm install command in the GitHub Actions releases workflow. What is happening is that the npm postinstall calls install.js which will download the Linux binary and overwrite the spago file. I can't think of any reason to install the packages before publishing; therefore, removing that line will fix the issue without breaking anything.

  npm_publish:
    name: Publish package on npm
    needs: build_release
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - uses: actions/setup-node@v2
        with:
          node-version: 14
      - name: Publish to NPM
        shell: bash
        env:
          NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}
        run: |
          sed -e "s/NPM_VERSION/$(./scripts/get-version)/g" -i npm/package.json
          sed -e "s/PACKAGE_VERSION/$(./scripts/get-version)/g" -i npm/install.js
          cd npm
          cp ../README.md ./README.md
          cp ../CONTRIBUTING.md ./CONTRIBUTING.md
          cp ../LICENSE ./LICENSE
--        npm install
          npm publish --non-interactive --access public

I forked the repo and was able to reproduce the issue. I then made the above change and confirmed that it corrects the issue. I'll get PR submitted to make the change.

There's also another issue with the npm publishing due to using the spago.cabal file instead of package.yml. I'll submit a separate PR to correct that issue as well.

cofad added a commit to cofad/spago that referenced this issue May 3, 2021
@f-f f-f closed this as completed in #786 May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants