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

fix (node): require failure with shebang interpreter #782

Merged
merged 1 commit into from
Feb 13, 2020

Conversation

erisu
Copy link
Member

@erisu erisu commented Feb 12, 2020

Motivation and Context

Fix failing tests.
Bin files that contains the shebang interpreter fails when being required.

Example Error:

/Users/travis/build/apache/cordova-ios/bin/templates/scripts/cordova/lib/list-devices:1
(function (exports, require, module, __filename, __dirname) { #!/usr/bin/env node
^
SyntaxError: Invalid or unexpected token

https://travis-ci.org/apache/cordova-ios/jobs/649198385#L1474

Description

  • Keep the bin files with the shebang interpreter but pull out all the code into a separate JS file. The bin files require the newly created JS files and execute them as usual.

  • Update everywhere else that use to require the bin but now require the newly created JS files which do not contain the shebang interpreter.

Additional Notes:

The version bin file, which contains the platform version, is updated by coho during release. Coho should now update the version in Api.js instead. It would be better if the version is pulled from package.json but the platform-centered workflow does not have this file. Additional investigation to improve this should be done but preferably outside of this PR.

Testing

  • npm t

Checklist

  • I've run the tests to see all new and existing tests pass

@erisu erisu added the bug label Feb 12, 2020
@erisu erisu added this to the 6.0.0 milestone Feb 12, 2020
@erisu erisu requested review from dpogue and jcesarmobile February 12, 2020 03:40
@codecov-io
Copy link

Codecov Report

Merging #782 into master will increase coverage by 0.28%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #782      +/-   ##
==========================================
+ Coverage   74.26%   74.55%   +0.28%     
==========================================
  Files          11       13       +2     
  Lines        1815     1835      +20     
==========================================
+ Hits         1348     1368      +20     
  Misses        467      467
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/versions.js 90.24% <ø> (ø) ⬆️
bin/templates/scripts/cordova/lib/run.js 21.42% <0%> (ø) ⬆️
bin/templates/scripts/cordova/Api.js 71.42% <100%> (+0.3%) ⬆️
bin/templates/scripts/cordova/lib/listDevices.js 100% <100%> (ø)
...emplates/scripts/cordova/lib/listEmulatorImages.js 100% <100%> (ø)
bin/templates/scripts/cordova/lib/build.js 50.76% <33.33%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59d0dd7...4aaa143. Read the comment docs.

@erisu
Copy link
Member Author

erisu commented Feb 13, 2020

Additional Feedback:

After more investigation the issue seems to be a combination of using the new node with the 2 year old rewire npm package.

I was able to recreate the issue here: https://github.com/erisu/shebang-test

@erisu erisu merged commit 14a5728 into apache:master Feb 13, 2020
@erisu erisu deleted the refactor/node-12.16.0-fix branch February 13, 2020 01:52
@erisu
Copy link
Member Author

erisu commented Feb 13, 2020

I went ahead and merged this PR to unblock other PRs from the failing tests. Because of rewire has not been maintained or updated in the past 2 years, it would be best to look in alternatives or how to test without rewire.

@NiklasMerz
Copy link
Member

I went ahead and merged this PR to unblock other PRs from the failing tests. Because of rewire has not been maintained or updated in the past 2 years, it would be best to look in alternatives or how to test without rewire.

Should we create an issue for that?

NiklasMerz pushed a commit to GEDYSIntraWare/cordova-ios that referenced this pull request Feb 24, 2020
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 this pull request may close these issues.

4 participants